block/meson.build | 2 +- io/meson.build | 2 +- meson.build | 5 +++-- storage-daemon/meson.build | 2 +- tests/meson.build | 6 +++--- ui/meson.build | 2 +- 6 files changed, 10 insertions(+), 9 deletions(-)
crypto/tlscreds.h includes GnuTLS headers if CONFIG_GNUTLS is set, but
GNUTLS_CFLAGS, that describe include path, are not propagated
transitively to all users of crypto and build fails if GnuTLS headers
reside in non-standard directory (which is a case for homebrew on Apple
Silicon).
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
block/meson.build | 2 +-
io/meson.build | 2 +-
meson.build | 5 +++--
storage-daemon/meson.build | 2 +-
tests/meson.build | 6 +++---
ui/meson.build | 2 +-
6 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/block/meson.build b/block/meson.build
index 5dcc1e5cce..61fc5e5955 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -39,7 +39,7 @@ block_ss.add(files(
'vmdk.c',
'vpc.c',
'write-threshold.c',
-), zstd, zlib)
+), zstd, zlib, gnutls)
softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('blkreplay.c'))
diff --git a/io/meson.build b/io/meson.build
index bcd8b1e737..bbcd3c53a4 100644
--- a/io/meson.build
+++ b/io/meson.build
@@ -12,4 +12,4 @@ io_ss.add(files(
'dns-resolver.c',
'net-listener.c',
'task.c',
-))
+), gnutls)
diff --git a/meson.build b/meson.build
index 372576f82c..d39fc018f4 100644
--- a/meson.build
+++ b/meson.build
@@ -1567,7 +1567,7 @@ blockdev_ss.add(files(
'blockdev-nbd.c',
'iothread.c',
'job-qmp.c',
-))
+), gnutls)
# os-posix.c contains POSIX-specific functions used by qemu-storage-daemon,
# os-win32.c does not
@@ -1723,6 +1723,7 @@ qmp = declare_dependency(link_whole: [libqmp])
libchardev = static_library('chardev', chardev_ss.sources() + genh,
name_suffix: 'fa',
+ dependencies: [gnutls],
build_by_default: false)
chardev = declare_dependency(link_whole: libchardev)
@@ -1941,7 +1942,7 @@ if have_tools
qemu_io = executable('qemu-io', files('qemu-io.c'),
dependencies: [block, qemuutil], install: true)
qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'),
- dependencies: [blockdev, qemuutil], install: true)
+ dependencies: [blockdev, qemuutil, gnutls], install: true)
subdir('storage-daemon')
subdir('contrib/rdmacm-mux')
diff --git a/storage-daemon/meson.build b/storage-daemon/meson.build
index c5adce81c3..68852f3d25 100644
--- a/storage-daemon/meson.build
+++ b/storage-daemon/meson.build
@@ -1,6 +1,6 @@
qsd_ss = ss.source_set()
qsd_ss.add(files('qemu-storage-daemon.c'))
-qsd_ss.add(blockdev, chardev, qmp, qom, qemuutil)
+qsd_ss.add(blockdev, chardev, qmp, qom, qemuutil, gnutls)
subdir('qapi')
diff --git a/tests/meson.build b/tests/meson.build
index 1fa068f27b..29ebaba48d 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -159,11 +159,11 @@ if have_block
'CONFIG_POSIX' in config_host
tests += {
'test-crypto-tlscredsx509': ['crypto-tls-x509-helpers.c', 'pkix_asn1_tab.c',
- tasn1, crypto],
+ tasn1, crypto, gnutls],
'test-crypto-tlssession': ['crypto-tls-x509-helpers.c', 'pkix_asn1_tab.c', 'crypto-tls-psk-helpers.c',
- tasn1, crypto],
+ tasn1, crypto, gnutls],
'test-io-channel-tls': ['io-channel-helpers.c', 'crypto-tls-x509-helpers.c', 'pkix_asn1_tab.c',
- tasn1, io, crypto]}
+ tasn1, io, crypto, gnutls]}
endif
if 'CONFIG_AUTH_PAM' in config_host
tests += {'test-authz-pam': [authz]}
diff --git a/ui/meson.build b/ui/meson.build
index 013258a01c..e6655c94a6 100644
--- a/ui/meson.build
+++ b/ui/meson.build
@@ -29,7 +29,7 @@ vnc_ss.add(files(
'vnc-ws.c',
'vnc-jobs.c',
))
-vnc_ss.add(zlib, png, jpeg)
+vnc_ss.add(zlib, png, jpeg, gnutls)
vnc_ss.add(when: sasl, if_true: files('vnc-auth-sasl.c'))
softmmu_ss.add_all(when: vnc, if_true: vnc_ss)
softmmu_ss.add(when: vnc, if_false: files('vnc-stubs.c'))
--
2.29.2
On Sat, 2 Jan 2021 at 12:54, Roman Bolshakov <r.bolshakov@yadro.com> wrote:
>
> crypto/tlscreds.h includes GnuTLS headers if CONFIG_GNUTLS is set, but
> GNUTLS_CFLAGS, that describe include path, are not propagated
> transitively to all users of crypto and build fails if GnuTLS headers
> reside in non-standard directory (which is a case for homebrew on Apple
> Silicon).
>
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
Ah, this is https://bugs.launchpad.net/qemu/+bug/1909256
-- thanks for finding a fix.
> ---
> block/meson.build | 2 +-
> io/meson.build | 2 +-
> meson.build | 5 +++--
> storage-daemon/meson.build | 2 +-
> tests/meson.build | 6 +++---
> ui/meson.build | 2 +-
> 6 files changed, 10 insertions(+), 9 deletions(-)
> diff --git a/ui/meson.build b/ui/meson.build
> index 013258a01c..e6655c94a6 100644
> --- a/ui/meson.build
> +++ b/ui/meson.build
> @@ -29,7 +29,7 @@ vnc_ss.add(files(
> 'vnc-ws.c',
> 'vnc-jobs.c',
> ))
> -vnc_ss.add(zlib, png, jpeg)
> +vnc_ss.add(zlib, png, jpeg, gnutls)
> vnc_ss.add(when: sasl, if_true: files('vnc-auth-sasl.c'))
> softmmu_ss.add_all(when: vnc, if_true: vnc_ss)
> softmmu_ss.add(when: vnc, if_false: files('vnc-stubs.c'))
Question to Paolo -- it seems pretty fragile to have to explicitly
list "these source files need these extra CFLAGS" in half a dozen
meson.build files, because it's pretty non-obvious that adding
eg '#include "block/nbd.h"' to a .c file means that you also
need to update the meson.build file to say "and now it needs these
extra CFLAGS". Isn't there some way we can just have the CFLAGS
added more globally so that if we use gnutls.h directly or
indirectly from more .c files in future it Just Works ?
If the build failed for the common Linux case then it would be
at least more obvious that you needed to update the meson.build
files. I think it's better to avoid "you need to do this special
thing that you'll only notice you're missing if you happen to test
on a somewhat obscure host configuration" where we can.
(We don't want to link helper binaries etc against gnutls if
they don't need it, but that's LDFLAGS, not CFLAGS.)
thanks
-- PMM
On 02/01/21 14:25, Peter Maydell wrote:
> Question to Paolo -- it seems pretty fragile to have to explicitly
> list "these source files need these extra CFLAGS" in half a dozen
> meson.build files, because it's pretty non-obvious that adding
> eg '#include "block/nbd.h"' to a .c file means that you also
> need to update the meson.build file to say "and now it needs these
> extra CFLAGS". Isn't there some way we can just have the CFLAGS
> added more globally so that if we use gnutls.h directly or
> indirectly from more .c files in future it Just Works ?
>
> If the build failed for the common Linux case then it would be
> at least more obvious that you needed to update the meson.build
> files. I think it's better to avoid "you need to do this special
> thing that you'll only notice you're missing if you happen to test
> on a somewhat obscure host configuration" where we can.
>
> (We don't want to link helper binaries etc against gnutls if
> they don't need it, but that's LDFLAGS, not CFLAGS.)
The gnutls dependency will already propagate from
if 'CONFIG_GNUTLS' in config_host
crypto_ss.add(gnutls)
endif
to
libcrypto = static_library('crypto', crypto_ss.sources() + genh,
dependencies: [crypto_ss.dependencies()], ...)
crypto = declare_dependency(link_whole: libcrypto,
dependencies: [authz, qom])
That is, Meson does know that everything that needs crypto needs gnutls
(see get_dependencies in mesonbuild/build.py if you're curious).
I think the issue is that dependencies are listed too late---in the
declare_dependency rather than the static_library. Take io/ for example:
libio = static_library('io', io_ss.sources() + genh,
dependencies: [io_ss.dependencies()],
link_with: libqemuutil,
name_suffix: 'fa',
build_by_default: false)
io = declare_dependency(link_whole: libio, dependencies: [crypto, qom])
Listing "crypto" in io's declare_dependency is enough to propagate the
gnutls LDFLAGS down to the executables, but it does not add the CFLAGS
to io/ files itself. So for the io/ files we aren't telling meson that
they need crypto (and thus in turn gnutls on the include path).
The fix should be pretty simple and localized to the "Library
dependencies" section of meson.build. For the two libraries above, the
fixed version would look like:
crypto_ss.add(authz, qom)
libcrypto = ... # same as above
crypto = declare_dependency(link_whole: libcrypto)
io_ss.add(crypto, qom)
...
libio = ... # same as above
io = declare_dependency(link_whole: libio)
(Roman, feel free to plunder the above if you want to turn it into a
commit message, and if it's correct of course).
Thanks,
Paolo
On Sat, Jan 02, 2021 at 01:25:07PM +0000, Peter Maydell wrote:
> On Sat, 2 Jan 2021 at 12:54, Roman Bolshakov <r.bolshakov@yadro.com> wrote:
> >
> > crypto/tlscreds.h includes GnuTLS headers if CONFIG_GNUTLS is set, but
> > GNUTLS_CFLAGS, that describe include path, are not propagated
> > transitively to all users of crypto and build fails if GnuTLS headers
> > reside in non-standard directory (which is a case for homebrew on Apple
> > Silicon).
> >
> > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
>
> Ah, this is https://bugs.launchpad.net/qemu/+bug/1909256
> -- thanks for finding a fix.
>
> > ---
> > block/meson.build | 2 +-
> > io/meson.build | 2 +-
> > meson.build | 5 +++--
> > storage-daemon/meson.build | 2 +-
> > tests/meson.build | 6 +++---
> > ui/meson.build | 2 +-
> > 6 files changed, 10 insertions(+), 9 deletions(-)
>
> > diff --git a/ui/meson.build b/ui/meson.build
> > index 013258a01c..e6655c94a6 100644
> > --- a/ui/meson.build
> > +++ b/ui/meson.build
> > @@ -29,7 +29,7 @@ vnc_ss.add(files(
> > 'vnc-ws.c',
> > 'vnc-jobs.c',
> > ))
> > -vnc_ss.add(zlib, png, jpeg)
> > +vnc_ss.add(zlib, png, jpeg, gnutls)
> > vnc_ss.add(when: sasl, if_true: files('vnc-auth-sasl.c'))
> > softmmu_ss.add_all(when: vnc, if_true: vnc_ss)
> > softmmu_ss.add(when: vnc, if_false: files('vnc-stubs.c'))
>
> Question to Paolo -- it seems pretty fragile to have to explicitly
> list "these source files need these extra CFLAGS" in half a dozen
> meson.build files, because it's pretty non-obvious that adding
> eg '#include "block/nbd.h"' to a .c file means that you also
> need to update the meson.build file to say "and now it needs these
> extra CFLAGS". Isn't there some way we can just have the CFLAGS
> added more globally so that if we use gnutls.h directly or
> indirectly from more .c files in future it Just Works ?
The actual usage of gnutls should be confined to the crypto/ code.
The rest of QEMU should only ever be using QEMU's TLS abstractions
and not directly be tied to GNUTLS. So ideally the gnutls flags
should only ever be added in the crypto/meson.build, and anything
which depends on that should then get the flags indirectly.
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 04/01/21 13:21, Daniel P. Berrangé wrote: > The actual usage of gnutls should be confined to the crypto/ code. > > The rest of QEMU should only ever be using QEMU's TLS abstractions > and not directly be tied to GNUTLS. So ideally the gnutls flags > should only ever be added in the crypto/meson.build, and anything > which depends on that should then get the flags indirectly. Right, see my reply. Paolo
On Mon, 4 Jan 2021 at 12:22, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > Question to Paolo -- it seems pretty fragile to have to explicitly
> > list "these source files need these extra CFLAGS" in half a dozen
> > meson.build files, because it's pretty non-obvious that adding
> > eg '#include "block/nbd.h"' to a .c file means that you also
> > need to update the meson.build file to say "and now it needs these
> > extra CFLAGS". Isn't there some way we can just have the CFLAGS
> > added more globally so that if we use gnutls.h directly or
> > indirectly from more .c files in future it Just Works ?
>
> The actual usage of gnutls should be confined to the crypto/ code.
>
> The rest of QEMU should only ever be using QEMU's TLS abstractions
> and not directly be tied to GNUTLS. So ideally the gnutls flags
> should only ever be added in the crypto/meson.build, and anything
> which depends on that should then get the flags indirectly.
Unfortunately include/crypto/tlscreds.h leaks this implementation
detail, because it defines:
struct QCryptoTLSCreds {
Object parent_obj;
char *dir;
QCryptoTLSCredsEndpoint endpoint;
#ifdef CONFIG_GNUTLS
gnutls_dh_params_t dh_params;
#endif
bool verifyPeer;
char *priority;
};
So every file that uses the tlscreds.h header must be
compiled with the GNUTLS flags, even if it doesn't itself
care about the underlying implementation.
(Maybe there's a way to keep the internals of the QCryptoTLSCreds
struct private to the crypto/ source files ?)
thanks
-- PMM
On 04/01/21 14:21, Peter Maydell wrote:
>> The rest of QEMU should only ever be using QEMU's TLS abstractions
>> and not directly be tied to GNUTLS. So ideally the gnutls flags
>> should only ever be added in the crypto/meson.build, and anything
>> which depends on that should then get the flags indirectly.
> Unfortunately include/crypto/tlscreds.h leaks this implementation
> detail
That is not a problem. As Daniel said, anything depending on crypto can
still get the gnutls flags _indirectly_.
In my proposed solution to the bug you'd get that via
io_ss.add(crypto, qom)
libio = static_library(..., dependencies: io_ss.dependencies())
for example.
Paolo
On Mon, 4 Jan 2021 at 14:40, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 04/01/21 14:21, Peter Maydell wrote: > >> The rest of QEMU should only ever be using QEMU's TLS abstractions > >> and not directly be tied to GNUTLS. So ideally the gnutls flags > >> should only ever be added in the crypto/meson.build, and anything > >> which depends on that should then get the flags indirectly. > > Unfortunately include/crypto/tlscreds.h leaks this implementation > > detail > > That is not a problem. As Daniel said, anything depending on crypto can > still get the gnutls flags _indirectly_. > > In my proposed solution to the bug you'd get that via > > io_ss.add(crypto, qom) > libio = static_library(..., dependencies: io_ss.dependencies()) Does this work recursively? For instance monitor/qmp-cmds.c needs the gnutls CFLAGS because: * qmp-cmds.c includes ui/vnc.h * ui/vnc.h includes include/crypto/tlssession.h * include/crypto/tlssession.h includes gnutls.h I don't see anything in monitor/meson.build that says "qmp-cmds.c needs whatever ui's dependencies are". thanks -- PMM
On 04/01/21 16:19, Peter Maydell wrote: > On Mon, 4 Jan 2021 at 14:40, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> On 04/01/21 14:21, Peter Maydell wrote: >>>> The rest of QEMU should only ever be using QEMU's TLS abstractions >>>> and not directly be tied to GNUTLS. So ideally the gnutls flags >>>> should only ever be added in the crypto/meson.build, and anything >>>> which depends on that should then get the flags indirectly. >>> Unfortunately include/crypto/tlscreds.h leaks this implementation >>> detail >> >> That is not a problem. As Daniel said, anything depending on crypto can >> still get the gnutls flags _indirectly_. >> >> In my proposed solution to the bug you'd get that via >> >> io_ss.add(crypto, qom) >> libio = static_library(..., dependencies: io_ss.dependencies()) > > Does this work recursively? For instance monitor/qmp-cmds.c > needs the gnutls CFLAGS because: > * qmp-cmds.c includes ui/vnc.h > * ui/vnc.h includes include/crypto/tlssession.h > * include/crypto/tlssession.h includes gnutls.h > > I don't see anything in monitor/meson.build that says "qmp-cmds.c > needs whatever ui's dependencies are". Hmm, I would have thought it would be handled, but Roman said otherwise. I'll look into it, at worst we can fix Meson and temporarily apply Roman's patch until it makes it into a release. Paolo
On Mon, 4 Jan 2021 at 17:49, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 04/01/21 16:19, Peter Maydell wrote: > > Does this work recursively? For instance monitor/qmp-cmds.c > > needs the gnutls CFLAGS because: > > * qmp-cmds.c includes ui/vnc.h > > * ui/vnc.h includes include/crypto/tlssession.h > > * include/crypto/tlssession.h includes gnutls.h > > > > I don't see anything in monitor/meson.build that says "qmp-cmds.c > > needs whatever ui's dependencies are". > > Hmm, I would have thought it would be handled, but Roman said otherwise. > I'll look into it, at worst we can fix Meson and temporarily apply > Roman's patch until it makes it into a release. NB that for qmp-cmds.c in particular https://patchew.org/QEMU/20210104161200.15068-1-peter.maydell@linaro.org/ will avoid the accidental inclusion of <gnutls.h>, though I guess in principle the monitor still needs to say it depends on ui... thanks -- PMM
© 2016 - 2025 Red Hat, Inc.