virhostcpu: Fix build with clang and newest kernel headers

[PATCH] virhostcpu: Fix build with clang and newest kernel headers

Posted by Peter Krempa 1 year, 8 months ago

The most recent environment e.g. present in our Fedora Rawhide builds
fail to build the tree with clang with the following error:

../src/util/virhostcpu.c:1291:25: error: field 'header' with variable sized type 'struct kvm_msrs' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
        struct kvm_msrs header;
                        ^

The problem seems to be that clang doesn't like the new way the
'entries' field in struct kvm_msrs is declared.

To work around the issue we can simply allocate the variable dynamically
and use the 'entries' member as it was intended to to access the
members.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/util/virhostcpu.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index 54d0166b85..c1e8dc8078 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -1287,25 +1287,22 @@ virHostCPUGetMSRFromKVM(unsigned long index,
                         uint64_t *result)
 {
     VIR_AUTOCLOSE fd = -1;
-    struct {
-        struct kvm_msrs header;
-        struct kvm_msr_entry entry;
-    } msr = {
-        .header = { .nmsrs = 1 },
-        .entry = { .index = index },
-    };
+    g_autofree struct kvm_msrs *msr = g_malloc0(sizeof(struct kvm_msrs) +
+                                                sizeof(struct kvm_msr_entry));
+    msr->nmsrs = 1;
+    msr->entries[0].index = index;

     if ((fd = open(KVM_DEVICE, O_RDONLY)) < 0) {
         virReportSystemError(errno, _("Unable to open %s"), KVM_DEVICE);
         return -1;
     }

-    if (ioctl(fd, KVM_GET_MSRS, &msr) < 0) {
+    if (ioctl(fd, KVM_GET_MSRS, msr) < 0) {
         VIR_DEBUG("Cannot get MSR 0x%lx from KVM", index);
         return 1;
     }

-    *result = msr.entry.data;
+    *result = msr->entries[0].data;
     return 0;
 }

-- 
2.37.1

Re: [PATCH] virhostcpu: Fix build with clang and newest kernel headers

Posted by Daniel P. Berrangé 1 year, 8 months ago

On Tue, Aug 23, 2022 at 04:15:54PM +0200, Peter Krempa wrote:
> The most recent environment e.g. present in our Fedora Rawhide builds
> fail to build the tree with clang with the following error:
> 
> ../src/util/virhostcpu.c:1291:25: error: field 'header' with variable sized type 'struct kvm_msrs' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
>         struct kvm_msrs header;
>                         ^
> 
> The problem seems to be that clang doesn't like the new way the
> 'entries' field in struct kvm_msrs is declared.
> 
> To work around the issue we can simply allocate the variable dynamically
> and use the 'entries' member as it was intended to to access the
> members.

We explicitly only support GCC and CLang, and intentionally rely on
many GNU extensions to C. CLang is trying to be helpful to people
who need fully portable C code, but we don't care about that. So
why not just turn off the warning by adding

  -Wno-gnu-variable-sized-style-not-at-end

better than making our code less understandable IMHO

> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/util/virhostcpu.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
> index 54d0166b85..c1e8dc8078 100644
> --- a/src/util/virhostcpu.c
> +++ b/src/util/virhostcpu.c
> @@ -1287,25 +1287,22 @@ virHostCPUGetMSRFromKVM(unsigned long index,
>                          uint64_t *result)
>  {
>      VIR_AUTOCLOSE fd = -1;
> -    struct {
> -        struct kvm_msrs header;
> -        struct kvm_msr_entry entry;
> -    } msr = {
> -        .header = { .nmsrs = 1 },
> -        .entry = { .index = index },
> -    };
> +    g_autofree struct kvm_msrs *msr = g_malloc0(sizeof(struct kvm_msrs) +
> +                                                sizeof(struct kvm_msr_entry));
> +    msr->nmsrs = 1;
> +    msr->entries[0].index = index;
> 
>      if ((fd = open(KVM_DEVICE, O_RDONLY)) < 0) {
>          virReportSystemError(errno, _("Unable to open %s"), KVM_DEVICE);
>          return -1;
>      }
> 
> -    if (ioctl(fd, KVM_GET_MSRS, &msr) < 0) {
> +    if (ioctl(fd, KVM_GET_MSRS, msr) < 0) {
>          VIR_DEBUG("Cannot get MSR 0x%lx from KVM", index);
>          return 1;
>      }
> 
> -    *result = msr.entry.data;
> +    *result = msr->entries[0].data;
>      return 0;
>  }
> 
> -- 
> 2.37.1
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] virhostcpu: Fix build with clang and newest kernel headers

Posted by Jiri Denemark 1 year, 8 months ago

On Tue, Aug 23, 2022 at 15:42:38 +0100, Daniel P. Berrangé wrote:
> On Tue, Aug 23, 2022 at 04:15:54PM +0200, Peter Krempa wrote:
> > The most recent environment e.g. present in our Fedora Rawhide builds
> > fail to build the tree with clang with the following error:
> > 
> > ../src/util/virhostcpu.c:1291:25: error: field 'header' with variable sized type 'struct kvm_msrs' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> >         struct kvm_msrs header;
> >                         ^
> > 
> > The problem seems to be that clang doesn't like the new way the
> > 'entries' field in struct kvm_msrs is declared.
> > 
> > To work around the issue we can simply allocate the variable dynamically
> > and use the 'entries' member as it was intended to to access the
> > members.
> 
> We explicitly only support GCC and CLang, and intentionally rely on
> many GNU extensions to C. CLang is trying to be helpful to people
> who need fully portable C code, but we don't care about that. So
> why not just turn off the warning by adding
> 
>   -Wno-gnu-variable-sized-style-not-at-end
> 
> better than making our code less understandable IMHO

Well I guess mostly because the new code is actually easier to
understand than the old one :-)

Jirka

Re: [PATCH] virhostcpu: Fix build with clang and newest kernel headers

Posted by Michal Prívozník 1 year, 8 months ago

On 8/23/22 16:15, Peter Krempa wrote:
> The most recent environment e.g. present in our Fedora Rawhide builds
> fail to build the tree with clang with the following error:
> 
> ../src/util/virhostcpu.c:1291:25: error: field 'header' with variable sized type 'struct kvm_msrs' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
>         struct kvm_msrs header;
>                         ^
> 
> The problem seems to be that clang doesn't like the new way the
> 'entries' field in struct kvm_msrs is declared.
> 
> To work around the issue we can simply allocate the variable dynamically
> and use the 'entries' member as it was intended to to access the
> members.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/util/virhostcpu.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)

Yeah, since this code is heavily inspired by QEMU I wanted to wait a bit
to see how QEMU deals with this because allocating those few bytes
looked needless to me. But I guess there's no better solution. Anyway,
I've raised this issue with QEMU here:

https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg03128.html

Michal

Re: [PATCH] virhostcpu: Fix build with clang and newest kernel headers

Posted by Peter Krempa 1 year, 8 months ago

On Tue, Aug 23, 2022 at 16:38:46 +0200, Michal Prívozník wrote:
> On 8/23/22 16:15, Peter Krempa wrote:
> > The most recent environment e.g. present in our Fedora Rawhide builds
> > fail to build the tree with clang with the following error:
> > 
> > ../src/util/virhostcpu.c:1291:25: error: field 'header' with variable sized type 'struct kvm_msrs' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> >         struct kvm_msrs header;
> >                         ^
> > 
> > The problem seems to be that clang doesn't like the new way the
> > 'entries' field in struct kvm_msrs is declared.
> > 
> > To work around the issue we can simply allocate the variable dynamically
> > and use the 'entries' member as it was intended to to access the
> > members.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >  src/util/virhostcpu.c | 15 ++++++---------
> >  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> Yeah, since this code is heavily inspired by QEMU I wanted to wait a bit
> to see how QEMU deals with this because allocating those few bytes
> looked needless to me. But I guess there's no better solution. Anyway,

This is a usual way how we deal with such structs e.g. in
virNetDevGetEthtoolGFeatures. Additionally now it uses the intended
accessor declared in 'struct kvm_msrs'.

Also we have A LOT places where we allocate short temp strings just to
format something. I'd not worry about overhead too much.

Re: [PATCH] virhostcpu: Fix build with clang and newest kernel headers

Posted by Jiri Denemark 1 year, 8 months ago

On Tue, Aug 23, 2022 at 16:15:54 +0200, Peter Krempa wrote:
> The most recent environment e.g. present in our Fedora Rawhide builds
> fail to build the tree with clang with the following error:
> 
> ../src/util/virhostcpu.c:1291:25: error: field 'header' with variable sized type 'struct kvm_msrs' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
>         struct kvm_msrs header;
>                         ^
> 
> The problem seems to be that clang doesn't like the new way the
> 'entries' field in struct kvm_msrs is declared.
> 
> To work around the issue we can simply allocate the variable dynamically
> and use the 'entries' member as it was intended to to access the
> members.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/util/virhostcpu.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)

Reviewed-by: Jiri Denemark <jdenemar@redhat.com>