[libvirt] [PATCH 1/4] fix problem with MKDIR_P vs mkdir_p
From: Matthias Bolte <photon@upb.de> configure defines mkdir_p but src/Makefile.am uses MKDIR_P --- src/Makefile.am | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 9d934b4..eff9039 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -589,6 +589,8 @@ endif endif EXTRA_DIST += $(LXC_CONTROLLER_SOURCES) +MKDIR_P = $(mkdir_p) + install-exec-local: $(MKDIR_P) "$(DESTDIR)$(localstatedir)/cache/libvirt" $(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/images" -- 1.5.6.5
The function 'brProbeVnetHdr()' added in commit b14bf853b4b0c3479d13c4842cea6faa3414c834 does not use the 'tapfd' parameter. gcc does not like that. Make it happy. Signed-off-by: Maximilian Wilhelm <max@rfc2324.org> --- src/bridge.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/bridge.c b/src/bridge.c index 990a567..960db91 100644 --- a/src/bridge.c +++ b/src/bridge.c @@ -423,7 +423,7 @@ static int brSetInterfaceMtu(brControl *ctl, * Returns 0 in case of success or an errno code in case of failure. */ static int -brProbeVnetHdr(int tapfd) +brProbeVnetHdr(int tapfd ATTRIBUTE_UNUSED) { #if defined(IFF_VNET_HDR) && defined(TUNGETFEATURES) && defined(TUNGETIFF) unsigned int features; -- 1.5.6.5
The 'asprintf' -> 'virAsprintf' transition made in commit 9e5d9950a1c5069d3cc908a7316d86a2d501d8a9 introducted a build error for 'driver.c' as 'util.h' has to be included. Signed-off-by: Maximilian Wilhelm <max@rfc2324.org> --- src/driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/driver.c b/src/driver.c index fda64dd..f9f9c7c 100644 --- a/src/driver.c +++ b/src/driver.c @@ -27,6 +27,7 @@ #include "driver.h" #include "memory.h" #include "logging.h" +#include "util.h" #define DEFAULT_DRIVER_DIR LIBDIR "/libvirt/drivers" -- 1.5.6.5
The 'getVer' fix introducted in 02a72b420670718640d9abf0e07f2b1cca7b4d2b breaks compiling libvirt with loadable module support. Work around this to get it building again. Signed-off-by: Maximilian Wilhelm <max@rfc2324.org> --- src/libvirt.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index ae076d1..038a1ac 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -803,7 +803,13 @@ virGetVersion(unsigned long *libVer, const char *type, if (typeVer != NULL) { if (type == NULL) type = "Xen"; + +/* FIXME: Add _proper_ type version handling for loadable driver modules... */ +#ifdef WITH_DRIVER_MODULES + *typeVer = LIBVIR_VERSION_NUMBER; +#else *typeVer = 0; + #if WITH_XEN if (STRCASEEQ(type, "Xen")) *typeVer = xenUnifiedVersion(); @@ -836,6 +842,7 @@ virGetVersion(unsigned long *libVer, const char *type, virLibConnError(NULL, VIR_ERR_NO_SUPPORT, type); return (-1); } +#endif /* WITH_DRIVER_MODULES */ } return (0); } -- 1.5.6.5
Maximilian Wilhelm <max@rfc2324.org> wrote:
The 'getVer' fix introducted in 02a72b420670718640d9abf0e07f2b1cca7b4d2b breaks compiling libvirt with loadable module support. Work around this to get it building again.
Signed-off-by: Maximilian Wilhelm <max@rfc2324.org> --- src/libvirt.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index ae076d1..038a1ac 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -803,7 +803,13 @@ virGetVersion(unsigned long *libVer, const char *type, if (typeVer != NULL) { if (type == NULL) type = "Xen"; + +/* FIXME: Add _proper_ type version handling for loadable driver modules... */ +#ifdef WITH_DRIVER_MODULES + *typeVer = LIBVIR_VERSION_NUMBER; +#else *typeVer = 0; + #if WITH_XEN if (STRCASEEQ(type, "Xen")) *typeVer = xenUnifiedVersion(); @@ -836,6 +842,7 @@ virGetVersion(unsigned long *libVer, const char *type, virLibConnError(NULL, VIR_ERR_NO_SUPPORT, type); return (-1); } +#endif /* WITH_DRIVER_MODULES */ } return (0); }
Thanks for the patch. I note that this link failure happens when you configure --with-driver-modules. As you suggest with the FIXME comment, there's room for improvement in this area. For example, with more than one of the various modules, we'd actually want all of their version strings, not just the one that happens to be listed last in the code. Or perhaps, a method to obtain a list of loaded/loadable modules, and another to query a module for its version string. #if WITH_XEN if (STRCASEEQ(type, "Xen")) *typeVer = xenUnifiedVersion(); #endif #if WITH_TEST if (STRCASEEQ(type, "Test")) *typeVer = LIBVIR_VERSION_NUMBER; #endif #if WITH_QEMU if (STRCASEEQ(type, "QEMU")) *typeVer = LIBVIR_VERSION_NUMBER; #endif #if WITH_LXC if (STRCASEEQ(type, "LXC")) *typeVer = LIBVIR_VERSION_NUMBER; #endif #if WITH_OPENVZ if (STRCASEEQ(type, "OpenVZ")) *typeVer = LIBVIR_VERSION_NUMBER; #endif #if WITH_UML if (STRCASEEQ(type, "UML")) *typeVer = LIBVIR_VERSION_NUMBER; #endif #if WITH_REMOTE if (STRCASEEQ(type, "Remote")) *typeVer = remoteVersion(); #endif However, even with your patch, the latest sources don't link for me. I get this: /c/libvirt/qemud/qemud.c:840: undefined reference to `virDriverLoadModule' The patch below fixes that:
From 3c6565f6e78e4a0da898248c07a59033aa111ef7 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 16 Feb 2009 12:57:28 +0100 Subject: [PATCH] Avoid link failure when configured --with-driver-modules
* qemud/Makefile.am (WITH_DRIVER_MODULES): Link with libvirt_driver.la and libvirt_util.la. Indent for readability. --- qemud/Makefile.am | 41 ++++++++++++++++++++++------------------- 1 files changed, 22 insertions(+), 19 deletions(-) diff --git a/qemud/Makefile.am b/qemud/Makefile.am index a0c161a..498f05c 100644 --- a/qemud/Makefile.am +++ b/qemud/Makefile.am @@ -104,30 +104,33 @@ libvirtd_LDADD = \ $(SASL_LIBS) \ $(POLKIT_LIBS) -if ! WITH_DRIVER_MODULES -if WITH_QEMU -libvirtd_LDADD += ../src/libvirt_driver_qemu.la -endif +if WITH_DRIVER_MODULES + libvirtd_LDADD += ../src/libvirt_driver.la + libvirtd_LDADD += ../src/libvirt_util.la +else + if WITH_QEMU + libvirtd_LDADD += ../src/libvirt_driver_qemu.la + endif -if WITH_LXC -libvirtd_LDADD += ../src/libvirt_driver_lxc.la -endif + if WITH_LXC + libvirtd_LDADD += ../src/libvirt_driver_lxc.la + endif -if WITH_UML -libvirtd_LDADD += ../src/libvirt_driver_uml.la -endif + if WITH_UML + libvirtd_LDADD += ../src/libvirt_driver_uml.la + endif -if WITH_STORAGE_DIR -libvirtd_LDADD += ../src/libvirt_driver_storage.la -endif + if WITH_STORAGE_DIR + libvirtd_LDADD += ../src/libvirt_driver_storage.la + endif -if WITH_NETWORK -libvirtd_LDADD += ../src/libvirt_driver_network.la -endif + if WITH_NETWORK + libvirtd_LDADD += ../src/libvirt_driver_network.la + endif -if WITH_NODE_DEVICES -libvirtd_LDADD += ../src/libvirt_driver_nodedev.la -endif + if WITH_NODE_DEVICES + libvirtd_LDADD += ../src/libvirt_driver_nodedev.la + endif endif libvirtd_LDADD += ../src/libvirt.la -- 1.6.2.rc0.264.g60787
Jim Meyering <jim@meyering.net> wrote:
Maximilian Wilhelm <max@rfc2324.org> wrote:
The 'getVer' fix introducted in 02a72b420670718640d9abf0e07f2b1cca7b4d2b breaks compiling libvirt with loadable module support. Work around this to get it building again.
Signed-off-by: Maximilian Wilhelm <max@rfc2324.org> --- src/libvirt.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-) ...
Thanks for the patch. I note that this link failure happens when you configure --with-driver-modules.
As you suggest with the FIXME comment, there's room for improvement in this area. For example, with more than one of the various modules, we'd actually want all of their version strings, not just the one that happens to be listed last in the code. Or perhaps, a method to obtain a list of loaded/loadable modules, and another to query a module for its version string.
...
However, even with your patch, the latest sources don't link for me. I get this:
/c/libvirt/qemud/qemud.c:840: undefined reference to `virDriverLoadModule'
The patch below fixes that:
From 3c6565f6e78e4a0da898248c07a59033aa111ef7 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 16 Feb 2009 12:57:28 +0100 Subject: [PATCH] Avoid link failure when configured --with-driver-modules
* qemud/Makefile.am (WITH_DRIVER_MODULES): Link with libvirt_driver.la and libvirt_util.la. Indent for readability. ...
I've pushed both changes.
Maximilian Wilhelm <max@rfc2324.org> wrote:
The 'asprintf' -> 'virAsprintf' transition made in commit 9e5d9950a1c5069d3cc908a7316d86a2d501d8a9 introducted a build error for 'driver.c' as 'util.h' has to be included.
Signed-off-by: Maximilian Wilhelm <max@rfc2324.org> --- src/driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/driver.c b/src/driver.c index fda64dd..f9f9c7c 100644 --- a/src/driver.c +++ b/src/driver.c @@ -27,6 +27,7 @@ #include "driver.h" #include "memory.h" #include "logging.h" +#include "util.h"
Thanks. I'll apply this shortly. I note that it's required when you've configured with --with-driver-modules
Maximilian Wilhelm <max@rfc2324.org> wrote:
The 'asprintf' -> 'virAsprintf' transition made in commit 9e5d9950a1c5069d3cc908a7316d86a2d501d8a9 introducted a build error for 'driver.c' as 'util.h' has to be included.
Signed-off-by: Maximilian Wilhelm <max@rfc2324.org> --- src/driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/driver.c b/src/driver.c index fda64dd..f9f9c7c 100644 --- a/src/driver.c +++ b/src/driver.c @@ -27,6 +27,7 @@ #include "driver.h" #include "memory.h" #include "logging.h" +#include "util.h"
Committed.
Maximilian Wilhelm <max@rfc2324.org> wrote:
The function 'brProbeVnetHdr()' added in commit b14bf853b4b0c3479d13c4842cea6faa3414c834 does not use the 'tapfd' parameter. gcc does not like that. Make it happy.
Signed-off-by: Maximilian Wilhelm <max@rfc2324.org> --- src/bridge.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/bridge.c b/src/bridge.c index 990a567..960db91 100644 --- a/src/bridge.c +++ b/src/bridge.c @@ -423,7 +423,7 @@ static int brSetInterfaceMtu(brControl *ctl, * Returns 0 in case of success or an errno code in case of failure. */ static int -brProbeVnetHdr(int tapfd) +brProbeVnetHdr(int tapfd ATTRIBUTE_UNUSED) { #if defined(IFF_VNET_HDR) && defined(TUNGETFEATURES) && defined(TUNGETIFF) unsigned int features;
Thanks. While it is unused in the #else block, it *is* used in the #if-block, so marking it unused like that is misleading and might even provoke its own warning some day. I'll fix it like this instead:
From 569c419001111a28772f4db75e5f086cdd08c70e Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 16 Feb 2009 12:23:27 +0100 Subject: [PATCH] avoid compiler warning about unused parameter
* src/bridge.c (brProbeVnetHdr) [IFF_VNET_HDR && TUNGETFEATURES && TUNGETIFF]: Use a "(void)" case to mark the parameter as unused. Reported by Maximilian Wilhelm in http://thread.gmane.org/gmane.comp.emulators.libvirt/11918/focus=11917 --- src/bridge.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/bridge.c b/src/bridge.c index fc11429..668dcf0 100644 --- a/src/bridge.c +++ b/src/bridge.c @@ -454,6 +454,7 @@ brProbeVnetHdr(int tapfd) return 1; #else + (void) tapfd; VIR_INFO0(_("Not enabling IFF_VNET_HDR; disabled at build time")); return 0; #endif -- 1.6.2.rc0.264.g60787
On Mon, Feb 16, 2009 at 12:28:31PM +0100, Jim Meyering wrote:
diff --git a/src/bridge.c b/src/bridge.c index fc11429..668dcf0 100644 --- a/src/bridge.c +++ b/src/bridge.c @@ -454,6 +454,7 @@ brProbeVnetHdr(int tapfd)
return 1; #else + (void) tapfd; VIR_INFO0(_("Not enabling IFF_VNET_HDR; disabled at build time")); return 0; #endif
I prefer it if we move the conditional block outside the method decl to stub out the whole block eg #if defined(IFF_VNET_HDR) && defined(TUNGETFEATURES) && defined(TUNGETIFF) static int brProbeVnetHdr(int tapfd) { unsigned int features; ... } #else static int brProbeVnetHdr(int tapfd ATTRIBUTE_UNUSED ) { return 0; } #endif Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Maximilian Wilhelm <max@rfc2324.org> wrote:
configure defines mkdir_p but src/Makefile.am uses MKDIR_P --- src/Makefile.am | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am index 9d934b4..eff9039 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -589,6 +589,8 @@ endif endif EXTRA_DIST += $(LXC_CONTROLLER_SOURCES)
+MKDIR_P = $(mkdir_p) + install-exec-local: $(MKDIR_P) "$(DESTDIR)$(localstatedir)/cache/libvirt" $(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/images"
This is just a symptom of your using a version of automake that is too old. I see that MKDIR_P *is* automatically substituted: $ grep @MKDIR_P config.status s&@MKDIR_P@&$ac_MKDIR_P&;t t $ grep MKDIR_P.= Makefile.in MKDIR_P = @MKDIR_P@
Anno domini 2009 Jim Meyering scripsit:
Maximilian Wilhelm <max@rfc2324.org> wrote:
configure defines mkdir_p but src/Makefile.am uses MKDIR_P --- src/Makefile.am | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am index 9d934b4..eff9039 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -589,6 +589,8 @@ endif endif EXTRA_DIST += $(LXC_CONTROLLER_SOURCES)
+MKDIR_P = $(mkdir_p) + install-exec-local: $(MKDIR_P) "$(DESTDIR)$(localstatedir)/cache/libvirt" $(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/images"
This is just a symptom of your using a version of automake that is too old.
Oh thanks for the hint. We had to downgrade autoake to 1.9 for building software for other systems... The usual auto*-fun. Ciao Max -- Follow the white penguin.
Maximilian Wilhelm <max@rfc2324.org> wrote:
Anno domini 2009 Jim Meyering scripsit:
Maximilian Wilhelm <max@rfc2324.org> wrote:
configure defines mkdir_p but src/Makefile.am uses MKDIR_P --- src/Makefile.am | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am index 9d934b4..eff9039 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -589,6 +589,8 @@ endif endif EXTRA_DIST += $(LXC_CONTROLLER_SOURCES)
+MKDIR_P = $(mkdir_p) + install-exec-local: $(MKDIR_P) "$(DESTDIR)$(localstatedir)/cache/libvirt" $(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/images"
This is just a symptom of your using a version of automake that is too old.
Oh thanks for the hint. We had to downgrade autoake to 1.9 for building software for other systems...
Be aware that in using versions of automake and autoconf that are too old, you are in effect reintroducing bugs that have long since been fixed. Be especially careful not to distribute for general use any distribution tarballs you make based on those older versions. And note that even for your own use, relying on automake-generated Makefiles is risky. For example, automake-1.9 has a bug (fixed in 1.10) that can make parallel builds thread-unsafe -- coincidentally, due to problems with detecting a working "mkdir -p", which is sometimes used for locking.
The usual auto*-fun.
Be sure to prod the maintainers of any packages that require anything older than the latest upstream stable releases of automake or autoconf.
On Sat, Feb 14, 2009 at 03:03:34PM +0100, Maximilian Wilhelm wrote:
Anno domini 2009 Jim Meyering scripsit:
Maximilian Wilhelm <max@rfc2324.org> wrote:
configure defines mkdir_p but src/Makefile.am uses MKDIR_P --- src/Makefile.am | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am index 9d934b4..eff9039 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -589,6 +589,8 @@ endif endif EXTRA_DIST += $(LXC_CONTROLLER_SOURCES)
+MKDIR_P = $(mkdir_p) + install-exec-local: $(MKDIR_P) "$(DESTDIR)$(localstatedir)/cache/libvirt" $(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/images"
This is just a symptom of your using a version of automake that is too old.
Oh thanks for the hint. We had to downgrade autoake to 1.9 for building software for other systems... The usual auto*-fun.
That should still be OK. Libvirt is intended to use automake 1.9.x and autoconf autoconf 2.59 or later. Specifically the versions from RHEL-5 or newer Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Sun, Feb 15, 2009 at 11:37:12AM +0000, Daniel P. Berrange wrote:
On Sat, Feb 14, 2009 at 03:03:34PM +0100, Maximilian Wilhelm wrote:
Anno domini 2009 Jim Meyering scripsit:
Maximilian Wilhelm <max@rfc2324.org> wrote:
configure defines mkdir_p but src/Makefile.am uses MKDIR_P --- src/Makefile.am | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am index 9d934b4..eff9039 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -589,6 +589,8 @@ endif endif EXTRA_DIST += $(LXC_CONTROLLER_SOURCES)
+MKDIR_P = $(mkdir_p) + install-exec-local: $(MKDIR_P) "$(DESTDIR)$(localstatedir)/cache/libvirt" $(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/images"
This is just a symptom of your using a version of automake that is too old.
Oh thanks for the hint. We had to downgrade autoake to 1.9 for building software for other systems... The usual auto*-fun.
That should still be OK. Libvirt is intended to use automake 1.9.x and autoconf autoconf 2.59 or later. Specifically the versions from RHEL-5 or newer
I've verified that this patch is *not* neccessary with automake 1.9.6 that is in RHEL-5 Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (3)
-
Daniel P. Berrange -
Jim Meyering -
Maximilian Wilhelm