On 01/07/2013 08:24 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> Add checks for existance of GNUTLS and automatically disable
s/existance/existence/
> it if not found.
>
> Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> ---
> configure.ac | 70 ++++++++++++++++++++++++++++++++-----------
> daemon/libvirtd.c | 41 ++++++++++++++++++-------
> daemon/remote.c | 2 ++
> src/Makefile.am | 8 ++++-
> src/libvirt.c | 17 ++++++++---
> src/locking/lock_daemon.c | 12 ++++++--
> src/lxc/lxc_controller.c | 6 ++--
> src/qemu/qemu_migration.c | 15 ++++++++--
> src/remote/remote_driver.c | 15 ++++++++++
> src/rpc/virnetclient.c | 20 ++++++++++---
> src/rpc/virnetclient.h | 8 ++++-
> src/rpc/virnetserver.c | 6 ++++
> src/rpc/virnetserver.h | 6 +++-
> src/rpc/virnetserverclient.c | 63 ++++++++++++++++++++++++++++++++++----
> src/rpc/virnetserverclient.h | 4 +++
> src/rpc/virnetserverservice.c | 31 ++++++++++++++-----
> src/rpc/virnetserverservice.h | 20 +++++++++----
> src/rpc/virnetsocket.c | 17 ++++++++++-
> src/rpc/virnetsocket.h | 6 +++-
> tests/Makefile.am | 11 ++++++-
> 20 files changed, 311 insertions(+), 67 deletions(-)
Touches quite a bit, but hopefully for the better. What platform are
you targeting where you were unwilling to require gnutls as a prereq?
> +++ b/configure.ac
> @@ -1025,30 +1025,62 @@ CFLAGS="$old_cflags"
> LIBS="$old_libs"
>
> dnl GnuTLS library
> -GNUTLS_CFLAGS=
> -GNUTLS_LIBS=
> -GNUTLS_FOUND=no
> -if test -x "$PKG_CONFIG" ; then
> - PKG_CHECK_MODULES(GNUTLS, gnutls >= $GNUTLS_REQUIRED,
> - [GNUTLS_FOUND=yes], [GNUTLS_FOUND=no])
> -fi
> -if test "$GNUTLS_FOUND" = "no"; then
> +AC_ARG_WITH([gnutls],
> + AC_HELP_STRING([--with-gnutls], [use GNUTLS for encryption @<:@default=check@:>@]),
> + [],
> + [with_gnutls=check])
> +
> +
> +if test "x$with_gnutls" != "xno"; then
> + if test "x$with_gnutls" != "xyes" && test "x$with_gnutls" != "xcheck"; then
> + GNUTLS_CFLAGS="-I$with_gnutls/include"
> + GNUTLS_LIBS="-L$with_gnutls/lib"
> + fi
> fail=0
> + old_cflags="$CFLAGS"
> old_libs="$LIBS"
> - AC_CHECK_HEADER([gnutls/gnutls.h], [], [fail=1])
> - AC_CHECK_LIB([gnutls], [gnutls_handshake],[], [fail=1], [-lgcrypt])
> + CFLAGS="$CFLAGS $GNUTLS_CFLAGS"
> + LIBS="$LIBS $GNUTLS_LIBS"
>
> - test $fail = 1 &&
> - AC_MSG_ERROR([You must install the GnuTLS library in order to compile and run libvirt])
> + GNUTLS_FOUND=no
> + if test -x "$PKG_CONFIG" ; then
> + PKG_CHECK_MODULES(GNUTLS, gnutls >= $GNUTLS_REQUIRED,
> + [GNUTLS_FOUND=yes], [GNUTLS_FOUND=no])
> + fi
> + if test "$GNUTLS_FOUND" = "no"; then
> + fail=0
> + AC_CHECK_HEADER([gnutls/gnutls.h], [], [fail=1])
> + AC_CHECK_LIB([gnutls], [gnutls_handshake],[], [fail=1], [-lgcrypt])
> +
> + test $fail = 0 && GNUTLS_FOUND=yes
> +
> + GNUTLS_LIBS="$GNUTLS_LIBS -lgnutls"
It looks fishy requiring -lgcrypt for the probe, but not using it here...
> + fi
> + if test "$GNUTLS_FOUND" = "no"; then
> + if test "$with_gnutls" = "check"; then
> + with_gnutls=no
> + GNUTLS_LIBS=
> + GNUTLS_CFLAGS=
> + else
> + AC_MSG_ERROR([You must install the GnuTLS library in order to compile and run libvirt])
> + fi
> + else
> + dnl Not all versions of gnutls include -lgcrypt, and so we add
> + dnl it explicitly for the calls to gcry_control/check_version
> + GNUTLS_LIBS="$GNUTLS_LIBS -lgcrypt"
...but then you unconditionally add it in later. I guess that's okay.
> +++ b/daemon/libvirtd.c
> @@ -449,7 +449,9 @@ static int daemonSetupNetworking(virNetServerPtr srv,
> virNetServerServicePtr svc = NULL;
> virNetServerServicePtr svcRO = NULL;
> virNetServerServicePtr svcTCP = NULL;
> +#if HAVE_GNUTLS
> virNetServerServicePtr svcTLS = NULL;
> +#endif
This makes for a lot of #ifdef'd code, and I'm worried that we might get
out of sync depending on whether gnutls is present or not. Would it be
any easier to always declare this variable, but always have it NULL when
tls is not present?
> gid_t unix_sock_gid = 0;
> int unix_sock_ro_mask = 0;
> int unix_sock_rw_mask = 0;
> @@ -474,9 +476,11 @@ static int daemonSetupNetworking(virNetServerPtr srv,
> unix_sock_rw_mask,
> unix_sock_gid,
> config->auth_unix_rw,
> +#if HAVE_GNUTLS
> + NULL,
> +#endif
Likewise, having different signatures based on the #ifdef seems awkward,
compared to making the conditional code just ignore the parameter when
tls is not present.
That said, this is not the first time we've done this (the code for
HAVE_SASL is equally if-def'd), so I guess I can live with it.
>
> #if HAVE_SASL
> if (config->auth_unix_rw == REMOTE_AUTH_SASL ||
> config->auth_unix_ro == REMOTE_AUTH_SASL ||
> - config->auth_tcp == REMOTE_AUTH_SASL ||
> - config->auth_tls == REMOTE_AUTH_SASL) {
> +# if HAVE_GNUTLS
> + config->auth_tls == REMOTE_AUTH_SASL ||
> +# endif
Indeed, the fact that this code is nested #ifdefs explains why you are
forced to follow the same style as HAVE_SASL for lots of conditional
signature.
> +++ b/src/locking/lock_daemon.c
> @@ -654,7 +654,11 @@ virLockDaemonSetupNetworkingSystemD(virNetServerPtr srv)
>
> /* Systemd passes FDs, starting immediately after stderr,
> * so the first FD we'll get is '3'. */
> - if (!(svc = virNetServerServiceNewFD(3, 0, false, 1, NULL)))
> + if (!(svc = virNetServerServiceNewFD(3, 0,
> +#if HAVE_GNUTLS
> + NULL,
> +#endif
> + false, 1)))
Still, I'm not a fan of mid-function-call #ifdefs. It does not play
nicely with turning a function call into a macro call. And you're
forced to reflect your #ifdef choice outside of src/rpc, which feels
like a leaky abstraction. Are you sure you want the conditional signature?
> +++ b/src/qemu/qemu_migration.c
> @@ -23,8 +23,10 @@
> #include <config.h>
>
> #include <sys/time.h>
> -#include <gnutls/gnutls.h>
> -#include <gnutls/x509.h>
> +#ifdef HAVE_GNUTLS
> +# include <gnutls/gnutls.h>
> +# include <gnutls/x509.h>
> +#endif
Also, can we wrap this inside a "virtls.h" convenience header, so that
all other files outside of src/rpc (or src/util) can unconditionally
include one header, instead of having to worry about HAVE_GNUTLS?
Overall, your patch looks sane, and you have a 'weak ACK' - that is, I'm
willing to look the other way and let this patch go in, if you don't
think it is worth even more refactoring to avoid quite so much leaky
#ifdef throughout the code base.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list