This fixes cross-building in some scenarios.

Specifically, when building for armv7l on x86_64, has_header()
will see the x86_64 version of the header and consider it usable.
Later, when an attempt is made to actually use it, the compiler
will quickly realize that things can't quite work.

The reason why we haven't hit this in our CI is that we only ever
install the foreign version of header files. When building the
Debian package, however, some of the Debian-specific tooling will
bring in the native version of the Linux headers in addition to
the foreign one, causing Meson to misreport the header's
availability status.

Checking for its actual usability, as opposed to mere presence,
is enough to make things work correctly in all cases.

https://bugs.debian.org/1024504

Suggested-by: Helmut Grohne <helmut@subdivi.de>
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 meson.build | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index d35d5e076b..21a4bd5b37 100644
--- a/meson.build
+++ b/meson.build
@@ -614,7 +614,6 @@ headers = [
   'asm/hwcap.h',
   'ifaddrs.h',
   'libtasn1.h',
-  'linux/kvm.h',
   'mntent.h',
   'net/ethernet.h',
   'net/if.h',
@@ -635,12 +634,24 @@ if host_machine.system() == 'freebsd'
   headers += 'libutil.h'
 endif
 
+# headers for which we need to check actual usability. in most
+# cases, checking for presence is enough (and it's way faster)
+check_headers = [
+  'linux/kvm.h',
+]
+
 foreach name : headers
   if cc.has_header(name)
     conf.set('WITH_@0@'.format(name.underscorify().to_upper()), 1)
   endif
 endforeach
 
+foreach name : check_headers
+  if cc.check_header(name)
+    conf.set('WITH_@0@'.format(name.underscorify().to_upper()), 1)
+  endif
+endforeach
+
 # check for kernel header required by src/util/virnetdevbridge.c
 if host_machine.system() == 'linux'
   if not cc.has_header('linux/sockios.h')
-- 
2.40.0

On 4/27/23 11:41, Andrea Bolognani wrote:
> This fixes cross-building in some scenarios.
> 
> Specifically, when building for armv7l on x86_64, has_header()
> will see the x86_64 version of the header and consider it usable.
> Later, when an attempt is made to actually use it, the compiler
> will quickly realize that things can't quite work.
> 
> The reason why we haven't hit this in our CI is that we only ever
> install the foreign version of header files. When building the
> Debian package, however, some of the Debian-specific tooling will
> bring in the native version of the Linux headers in addition to
> the foreign one, causing Meson to misreport the header's
> availability status.
> 
> Checking for its actual usability, as opposed to mere presence,
> is enough to make things work correctly in all cases.
> 
> https://bugs.debian.org/1024504
> 
> Suggested-by: Helmut Grohne <helmut@subdivi.de>
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  meson.build | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/meson.build b/meson.build
> index d35d5e076b..21a4bd5b37 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -614,7 +614,6 @@ headers = [
>    'asm/hwcap.h',
>    'ifaddrs.h',
>    'libtasn1.h',
> -  'linux/kvm.h',
>    'mntent.h',
>    'net/ethernet.h',
>    'net/if.h',
> @@ -635,12 +634,24 @@ if host_machine.system() == 'freebsd'
>    headers += 'libutil.h'
>  endif
>  
> +# headers for which we need to check actual usability. in most
> +# cases, checking for presence is enough (and it's way faster)
> +check_headers = [
> +  'linux/kvm.h',
> +]
> +
>  foreach name : headers
>    if cc.has_header(name)
>      conf.set('WITH_@0@'.format(name.underscorify().to_upper()), 1)
>    endif
>  endforeach
>  
> +foreach name : check_headers
> +  if cc.check_header(name)
> +    conf.set('WITH_@0@'.format(name.underscorify().to_upper()), 1)
> +  endif
> +endforeach
> +
>  # check for kernel header required by src/util/virnetdevbridge.c
>  if host_machine.system() == 'linux'
>    if not cc.has_header('linux/sockios.h')


One could argue, that the semantics of has_header() is broken then. What
good is there to check if the host has a header file when compiler
rejects it subsequently?

But leaving meson aside, shouldn't we just use check_header() for every
header file then? I mean, this fixes this particular instance of the
problem, but can't we hit it again with say ifaddrs.h or any other
header file on the list?

Michal

On Thu, Apr 27, 2023 at 11:50:05AM +0200, Michal Prívozník wrote:
> On 4/27/23 11:41, Andrea Bolognani wrote:
> > +# headers for which we need to check actual usability. in most
> > +# cases, checking for presence is enough (and it's way faster)
> > +check_headers = [
> > +  'linux/kvm.h',
> > +]
> > +
> > +foreach name : check_headers
> > +  if cc.check_header(name)
> > +    conf.set('WITH_@0@'.format(name.underscorify().to_upper()), 1)
> > +  endif
> > +endforeach
>
> One could argue, that the semantics of has_header() is broken then. What
> good is there to check if the host has a header file when compiler
> rejects it subsequently?
>
> But leaving meson aside, shouldn't we just use check_header() for every
> header file then? I mean, this fixes this particular instance of the
> problem, but can't we hit it again with say ifaddrs.h or any other
> header file on the list?

We could. The Meson documentation recommends not doing this, as
check_header() is (understandably) slower than has_header().

I haven't tried to see how much slower we're actually talking...
Maybe for the 20-ish headers that we care about, it wouldn't make a
measurable difference? Especially since the results of the check are
cached between Meson runs.

Anyway, I believe the reason why only linux/kvm.h is proving to be
problematic here is that KVM support might or might not be present
depending on the architecture. All the other stuff is going to be
affected by whether or not we're targeting Linux, not the specific
Linux architecture.

-- 
Andrea Bolognani / Red Hat / Virtualization

On Thu, Apr 27, 2023 at 12:00:59PM +0200, Andrea Bolognani wrote:
> On Thu, Apr 27, 2023 at 11:50:05AM +0200, Michal Prívozník wrote:
> > On 4/27/23 11:41, Andrea Bolognani wrote:
> > > +# headers for which we need to check actual usability. in most
> > > +# cases, checking for presence is enough (and it's way faster)
> > > +check_headers = [
> > > +  'linux/kvm.h',
> > > +]
> > > +
> > > +foreach name : check_headers
> > > +  if cc.check_header(name)
> > > +    conf.set('WITH_@0@'.format(name.underscorify().to_upper()), 1)
> > > +  endif
> > > +endforeach
> >
> > One could argue, that the semantics of has_header() is broken then. What
> > good is there to check if the host has a header file when compiler
> > rejects it subsequently?
> >
> > But leaving meson aside, shouldn't we just use check_header() for every
> > header file then? I mean, this fixes this particular instance of the
> > problem, but can't we hit it again with say ifaddrs.h or any other
> > header file on the list?
> 
> We could. The Meson documentation recommends not doing this, as
> check_header() is (understandably) slower than has_header().
> 
> I haven't tried to see how much slower we're actually talking...
> Maybe for the 20-ish headers that we care about, it wouldn't make a
> measurable difference? Especially since the results of the check are
> cached between Meson runs.

I tried testing:

diff --git a/meson.build b/meson.build
index c15003ce02..3ae02ce4df 100644
--- a/meson.build
+++ b/meson.build
@@ -633,14 +633,14 @@ if host_machine.system() == 'freebsd'
 endif
 
 foreach name : headers
-  if cc.has_header(name)
+  if cc.check_header(name)
     conf.set('WITH_@0@'.format(name.underscorify().to_upper()), 1)
   endif
 endforeach
 
 # check for kernel header required by src/util/virnetdevbridge.c
 if host_machine.system() == 'linux'
-  if not cc.has_header('linux/sockios.h')
+  if not cc.check_header('linux/sockios.h')
     error('You must install kernel-headers in order to compile libvirt with QEMU or LXC support')
   endif
 endif


and could not measure any deterministic change in execution
time before/after


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 :|

On Thu, Apr 27, 2023 at 11:28:36AM +0100, Daniel P. Berrangé wrote:
> On Thu, Apr 27, 2023 at 12:00:59PM +0200, Andrea Bolognani wrote:
> > On Thu, Apr 27, 2023 at 11:50:05AM +0200, Michal Prívozník wrote:
> > > But leaving meson aside, shouldn't we just use check_header() for every
> > > header file then? I mean, this fixes this particular instance of the
> > > problem, but can't we hit it again with say ifaddrs.h or any other
> > > header file on the list?
> >
> > We could. The Meson documentation recommends not doing this, as
> > check_header() is (understandably) slower than has_header().
> >
> > I haven't tried to see how much slower we're actually talking...
> > Maybe for the 20-ish headers that we care about, it wouldn't make a
> > measurable difference? Especially since the results of the check are
> > cached between Meson runs.
>
> I tried testing:
>
> diff --git a/meson.build b/meson.build
> index c15003ce02..3ae02ce4df 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -633,14 +633,14 @@ if host_machine.system() == 'freebsd'
>  endif
>
>  foreach name : headers
> -  if cc.has_header(name)
> +  if cc.check_header(name)
>      conf.set('WITH_@0@'.format(name.underscorify().to_upper()), 1)
>    endif
>  endforeach
>
>  # check for kernel header required by src/util/virnetdevbridge.c
>  if host_machine.system() == 'linux'
> -  if not cc.has_header('linux/sockios.h')
> +  if not cc.check_header('linux/sockios.h')
>      error('You must install kernel-headers in order to compile libvirt with QEMU or LXC support')
>    endif
>  endif
>
>
> and could not measure any deterministic change in execution
> time before/after

I've just tried the same on a 4th generation low-voltage laptop CPU,
with a patch that goes further and completely replaces all uses of
has_header() with check_header(), and haven't been able to detect any
meaningful change either. Let's go with the simpler and more reliable
approach then :)

-- 
Andrea Bolognani / Red Hat / Virtualization