4:37 a.m.
This is supposed to fix bug report which appeared on the list [1]. The driver
registration ordering is back as it was, but the subdrivers are registered
before. Unfortunately, if we don't want to dig vbox network and storage code
out, we have to compile the sources couple of times.
Michal Privoznik (3):
virdrivermoduletest: Test all the modules
virDriverLoadModule: Honor libvirt func name tranlsation
vbox: Register per partes
daemon/libvirtd.c | 16 +++++++++++++--
src/Makefile.am | 41 ++++++++++++++++++++++++++++++++++---
src/driver.c | 19 +++++++++++++++--
src/vbox/vbox_driver.c | 50 ++++++++++++++++++++++++++++++++++++++++-----
src/vbox/vbox_driver.h | 10 +++++++++
tests/virdrivermoduletest.c | 18 ++++++++++------
6 files changed, 136 insertions(+), 18 deletions(-)
--
1.8.5.5
4:37 a.m.
New subject: [libvirt] [PATCH 1/3] virdrivermoduletest: Test all the modules
Even though we kept adding new and new modules (e.g. vbox or bhyve)
the test wasn't updated. Do that now. Moreover, while it's not
crucial, it's nice to reorder test cases to match the order in which
the daemon loads the modules.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/virdrivermoduletest.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/tests/virdrivermoduletest.c b/tests/virdrivermoduletest.c
index 840fc28..d823ad9 100644
--- a/tests/virdrivermoduletest.c
+++ b/tests/virdrivermoduletest.c
@@ -71,6 +71,9 @@ mymain(void)
#else
# define USE_NETWORK NULL
#endif
+#ifdef WITH_INTERFACE
+ TEST("interface", NULL);
+#endif
#ifdef WITH_STORAGE
TEST("storage", NULL);
#endif
@@ -83,8 +86,11 @@ mymain(void)
#ifdef WITH_NWFILTER
TEST("nwfilter", NULL);
#endif
-#ifdef WITH_INTERFACE
- TEST("interface", NULL);
+#ifdef WITH_XEN
+ TEST("xen", NULL);
+#endif
+#ifdef WITH_LIBXL
+ TEST("libxl", NULL);
#endif
#ifdef WITH_QEMU
TEST("qemu", USE_NETWORK);
@@ -95,11 +101,11 @@ mymain(void)
#ifdef WITH_UML
TEST("uml", NULL);
#endif
-#ifdef WITH_XEN
- TEST("xen", NULL);
+#ifdef WITH_VBOX
+ TEST("vbox", NULL);
#endif
-#ifdef WITH_LIBXL
- TEST("libxl", NULL);
+#ifdef WITH_BHYVE
+ TEST("bhyve", NULL);
#endif
return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
--
1.8.5.5
4:37 a.m.
New subject: [libvirt] [PATCH 2/3] virDriverLoadModule: Honor libvirt func name tranlsation
There's this unwritten rule in libvirt that vir_function is translated
into virFunction when needed (e.g. in remote protocol definition,
python, ...). Up till now we ignored such translation in driver module
loading and did fine. Well, we didn't have any module with an
underscore in its name. But this will change in next commit. The
problem is, once an a module is dlopen()-ed, we derive register
function name from its name. So instead of "driver_subdriverRegister"
do some magic to turn that into "driverSubdriverRegister".
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/driver.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/src/driver.c b/src/driver.c
index 9e3a2eb..71569e6 100644
--- a/src/driver.c
+++ b/src/driver.c
@@ -23,6 +23,7 @@
#include <config.h>
#include <unistd.h>
+#include <c-ctype.h>
#include "driver.h"
#include "viralloc.h"
@@ -45,7 +46,8 @@ VIR_LOG_INIT("driver");
void *
virDriverLoadModule(const char *name)
{
- char *modfile = NULL, *regfunc = NULL;
+ char *modfile = NULL, *regfunc = NULL, *fixedname = NULL;
+ char *tmp;
void *handle = NULL;
int (*regsym)(void);
@@ -72,7 +74,18 @@ virDriverLoadModule(const char *name)
goto cleanup;
}
- if (virAsprintfQuiet(®func, "%sRegister", name) < 0) {
+ if (VIR_STRDUP_QUIET(fixedname, name) < 0) {
+ VIR_ERROR(_("out of memory"));
+ goto cleanup;
+ }
+
+ /* convert something_like_this into somethingLikeThis */
+ while ((tmp = strchr(fixedname, '_'))) {
+ memmove(tmp, tmp + 1, strlen(tmp));
+ *tmp = c_toupper(*tmp);
+ }
+
+ if (virAsprintfQuiet(®func, "%sRegister", fixedname) < 0) {
goto cleanup;
}
@@ -89,11 +102,13 @@ virDriverLoadModule(const char *name)
VIR_FREE(modfile);
VIR_FREE(regfunc);
+ VIR_FREE(fixedname);
return handle;
cleanup:
VIR_FREE(modfile);
VIR_FREE(regfunc);
+ VIR_FREE(fixedname);
if (handle)
dlclose(handle);
return NULL;
--
1.8.5.5
4:37 a.m.
New subject: [libvirt] [PATCH 3/3] vbox: Register per partes
Since times when vbox moved to the daemon (due to some licensing
issue) the subdrivers that vbox implements were registered, but not
opened since our generic subdrivers took priority. I've tried to fix
this in 65b7d553f39ff9 but it was not correct. Apparently moving
vbox driver registration upfront changes the default connection URI
which makes some users sad. So, this commit breaks vbox into pieces
and register vbox's network and storage drivers first, and vbox driver
then at the end. This way, the vbox driver is registered in the order
it always was, but its subdrivers are registered prior the generic
ones.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
daemon/libvirtd.c | 16 ++++++++++++++--
src/Makefile.am | 41 ++++++++++++++++++++++++++++++++++++++---
src/vbox/vbox_driver.c | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
src/vbox/vbox_driver.h | 10 ++++++++++
4 files changed, 107 insertions(+), 10 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 4cf78e6..c3bd2ab 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -383,7 +383,7 @@ static void daemonInitialize(void)
* is not loaded they'll get a suitable error at that point
*/
# ifdef WITH_VBOX
- virDriverLoadModule("vbox");
+ virDriverLoadModule("vbox_network");
# endif
# ifdef WITH_NETWORK
virDriverLoadModule("network");
@@ -391,6 +391,9 @@ static void daemonInitialize(void)
# ifdef WITH_INTERFACE
virDriverLoadModule("interface");
# endif
+# ifdef WITH_VBOX
+ virDriverLoadModule("vbox_storage");
+# endif
# ifdef WITH_STORAGE
virDriverLoadModule("storage");
# endif
@@ -418,12 +421,15 @@ static void daemonInitialize(void)
# ifdef WITH_UML
virDriverLoadModule("uml");
# endif
+# ifdef WITH_VBOX
+ virDriverLoadModule("vbox");
+# endif
# ifdef WITH_BHYVE
virDriverLoadModule("bhyve");
# endif
#else
# ifdef WITH_VBOX
- vboxRegister();
+ vboxNetworkRegister();
# endif
# ifdef WITH_NETWORK
networkRegister();
@@ -431,6 +437,9 @@ static void daemonInitialize(void)
# ifdef WITH_INTERFACE
interfaceRegister();
# endif
+# ifdef WITH_VBOX
+ vboxStorageRegister();
+# endif
# ifdef WITH_STORAGE
storageRegister();
# endif
@@ -458,6 +467,9 @@ static void daemonInitialize(void)
# ifdef WITH_UML
umlRegister();
# endif
+# ifdef WITH_VBOX
+ vboxRegister();
+# endif
# ifdef WITH_BHYVE
bhyveRegister();
# endif
diff --git a/src/Makefile.am b/src/Makefile.am
index 538530e..46e411e 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1135,13 +1135,27 @@ libvirt_driver_vmware_la_SOURCES = $(VMWARE_DRIVER_SOURCES)
endif WITH_VMWARE
if WITH_VBOX
-noinst_LTLIBRARIES += libvirt_driver_vbox_impl.la
+noinst_LTLIBRARIES += \
+ libvirt_driver_vbox_impl.la \
+ libvirt_driver_vbox_network_impl.la \
+ libvirt_driver_vbox_storage_impl.la
libvirt_driver_vbox_la_SOURCES =
libvirt_driver_vbox_la_LIBADD = libvirt_driver_vbox_impl.la
+libvirt_driver_vbox_network_la_SOURCES =
+libvirt_driver_vbox_network_la_LIBADD = libvirt_driver_vbox_network_impl.la
+libvirt_driver_vbox_storage_la_SOURCES =
+libvirt_driver_vbox_storage_la_LIBADD = libvirt_driver_vbox_storage_impl.la
if WITH_DRIVER_MODULES
-mod_LTLIBRARIES += libvirt_driver_vbox.la
+mod_LTLIBRARIES += \
+ libvirt_driver_vbox.la \
+ libvirt_driver_vbox_network.la \
+ libvirt_driver_vbox_storage.la
libvirt_driver_vbox_la_LIBADD += ../gnulib/lib/libgnu.la
libvirt_driver_vbox_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS)
+libvirt_driver_vbox_network_la_LIBADD += ../gnulib/lib/libgnu.la
+libvirt_driver_vbox_network_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS)
+libvirt_driver_vbox_storage_la_LIBADD += ../gnulib/lib/libgnu.la
+libvirt_driver_vbox_storage_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS)
else ! WITH_DRIVER_MODULES
noinst_LTLIBRARIES += libvirt_driver_vbox.la
# GPLv2-only license requries that it be linked into
@@ -1151,12 +1165,33 @@ endif ! WITH_DRIVER_MODULES
libvirt_driver_vbox_impl_la_CFLAGS = \
-I$(top_srcdir)/src/conf \
- $(AM_CFLAGS)
+ $(AM_CFLAGS) \
+ -DVBOX_DRIVER
libvirt_driver_vbox_impl_la_LDFLAGS = $(AM_LDFLAGS)
libvirt_driver_vbox_impl_la_LIBADD = $(DLOPEN_LIBS) \
$(MSCOM_LIBS) \
$(LIBXML_LIBS)
libvirt_driver_vbox_impl_la_SOURCES = $(VBOX_DRIVER_SOURCES)
+
+libvirt_driver_vbox_network_impl_la_CFLAGS = \
+ -I$(top_srcdir)/src/conf \
+ $(AM_CFLAGS) \
+ -DVBOX_NETWORK_DRIVER
+libvirt_driver_vbox_network_impl_la_LDFLAGS = $(AM_LDFLAGS)
+libvirt_driver_vbox_network_impl_la_LIBADD = $(DLOPEN_LIBS) \
+ $(MSCOM_LIBS) \
+ $(LIBXML_LIBS)
+libvirt_driver_vbox_network_impl_la_SOURCES = $(VBOX_DRIVER_SOURCES)
+
+libvirt_driver_vbox_storage_impl_la_CFLAGS = \
+ -I$(top_srcdir)/src/conf \
+ $(AM_CFLAGS) \
+ -DVBOX_STORAGE_DRIVER
+libvirt_driver_vbox_storage_impl_la_LDFLAGS = $(AM_LDFLAGS)
+libvirt_driver_vbox_storage_impl_la_LIBADD = $(DLOPEN_LIBS) \
+ $(MSCOM_LIBS) \
+ $(LIBXML_LIBS)
+libvirt_driver_vbox_storage_impl_la_SOURCES = $(VBOX_DRIVER_SOURCES)
endif WITH_VBOX
if WITH_XENAPI
diff --git a/src/vbox/vbox_driver.c b/src/vbox/vbox_driver.c
index 498be71..d33f78f 100644
--- a/src/vbox/vbox_driver.c
+++ b/src/vbox/vbox_driver.c
@@ -75,12 +75,15 @@ static virDriver vboxDriverDummy;
#define VIR_FROM_THIS VIR_FROM_VBOX
-int vboxRegister(void)
+static void
+vboxGetDrivers(virDriverPtr *driver_ret,
+ virNetworkDriverPtr *networkDriver_ret,
+ virStorageDriverPtr *storageDriver_ret)
{
- virDriverPtr driver;
+ virDriverPtr driver;
virNetworkDriverPtr networkDriver;
virStorageDriverPtr storageDriver;
- uint32_t uVersion;
+ uint32_t uVersion;
/*
* If the glue layer does not initialize, we register a driver
@@ -157,15 +160,52 @@ int vboxRegister(void)
VIR_DEBUG("VBoxCGlueInit failed, using dummy driver");
}
- if (virRegisterDriver(driver) < 0)
- return -1;
+ if (driver_ret)
+ *driver_ret = driver;
+ if (networkDriver_ret)
+ *networkDriver_ret = networkDriver;
+ if (storageDriver_ret)
+ *storageDriver_ret = storageDriver;
+}
+
+
+#if !defined(WITH_DRIVER_MODULES) || defined(VBOX_NETWORK_DRIVER)
+int vboxNetworkRegister(void)
+{
+ virNetworkDriverPtr networkDriver;
+
+ vboxGetDrivers(NULL, &networkDriver, NULL);
if (virRegisterNetworkDriver(networkDriver) < 0)
return -1;
+ return 0;
+}
+#endif
+
+#if !defined(WITH_DRIVER_MODULES) || defined(VBOX_STORAGE_DRIVER)
+int vboxStorageRegister(void)
+{
+ virStorageDriverPtr storageDriver;
+
+ vboxGetDrivers(NULL, NULL, &storageDriver);
+
if (virRegisterStorageDriver(storageDriver) < 0)
return -1;
+ return 0;
+}
+#endif
+
+#if !defined(WITH_DRIVER_MODULES) || defined(VBOX_DRIVER)
+int vboxRegister(void)
+{
+ virDriverPtr driver;
+
+ vboxGetDrivers(&driver, NULL, NULL);
+ if (virRegisterDriver(driver) < 0)
+ return -1;
return 0;
}
+#endif
static virDrvOpenStatus dummyConnectOpen(virConnectPtr conn,
virConnectAuthPtr auth ATTRIBUTE_UNUSED,
diff --git a/src/vbox/vbox_driver.h b/src/vbox/vbox_driver.h
index 399e4c6..ccd331a 100644
--- a/src/vbox/vbox_driver.h
+++ b/src/vbox/vbox_driver.h
@@ -31,6 +31,16 @@
# include "internal.h"
+# if !defined(WITH_DRIVER_MODULES) || defined(VBOX_NETWORK_DRIVER)
+int vboxNetworkRegister(void);
+# endif
+
+# if !defined(WITH_DRIVER_MODULES) || defined(VBOX_STORAGE_DRIVER)
+int vboxStorageRegister(void);
+# endif
+
+# if !defined(WITH_DRIVER_MODULES) || defined(VBOX_DRIVER)
int vboxRegister(void);
+# endif
#endif
--
1.8.5.5
11:37 a.m.
New subject: [libvirt] [PATCH 3/3] vbox: Register per partes
On Fri, Aug 22, 2014 at 11:37:52AM +0200, Michal Privoznik wrote:
Since times when vbox moved to the daemon (due to some licensing
issue) the subdrivers that vbox implements were registered, but not
opened since our generic subdrivers took priority. I've tried to fix
this in 65b7d553f39ff9 but it was not correct. Apparently moving
vbox driver registration upfront changes the default connection URI
which makes some users sad. So, this commit breaks vbox into pieces
and register vbox's network and storage drivers first, and vbox driver
then at the end. This way, the vbox driver is registered in the order
it always was, but its subdrivers are registered prior the generic
ones.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
daemon/libvirtd.c | 16 ++++++++++++++--
src/Makefile.am | 41 ++++++++++++++++++++++++++++++++++++++---
src/vbox/vbox_driver.c | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
src/vbox/vbox_driver.h | 10 ++++++++++
4 files changed, 107 insertions(+), 10 deletions(-)
[...]
diff --git a/src/Makefile.am b/src/Makefile.am
index 538530e..46e411e 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1135,13 +1135,27 @@ libvirt_driver_vmware_la_SOURCES = $(VMWARE_DRIVER_SOURCES)
endif WITH_VMWARE
if WITH_VBOX
-noinst_LTLIBRARIES += libvirt_driver_vbox_impl.la
+noinst_LTLIBRARIES += \
+ libvirt_driver_vbox_impl.la \
+ libvirt_driver_vbox_network_impl.la \
+ libvirt_driver_vbox_storage_impl.la
libvirt_driver_vbox_la_SOURCES =
libvirt_driver_vbox_la_LIBADD = libvirt_driver_vbox_impl.la
+libvirt_driver_vbox_network_la_SOURCES =
+libvirt_driver_vbox_network_la_LIBADD = libvirt_driver_vbox_network_impl.la
+libvirt_driver_vbox_storage_la_SOURCES =
+libvirt_driver_vbox_storage_la_LIBADD = libvirt_driver_vbox_storage_impl.la
Couldn't you just do:
libvirt_driver_vbox_network_la_LIBADD = libvirt_driver_vbox_impl.la
libvirt_driver_vbox_storage_la_LIBADD = libvirt_driver_vbox_impl.la
Or just symlink these to the original one? Of course you would export
all three register symbols, but just use the one you want for each
load. Or am I missing something why that wouldn't work?
[reorganizing the hunks so my responses follow logically]
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 4cf78e6..c3bd2ab 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -383,7 +383,7 @@ static void daemonInitialize(void)
* is not loaded they'll get a suitable error at that point
*/
# ifdef WITH_VBOX
- virDriverLoadModule("vbox");
+ virDriverLoadModule("vbox_network");
# endif
It would look a bit nicer, I guess, if we just loaded the symbols in
virDriverLoadModule() into some kind of table (or list) and then
register them later on, but I understand this is just a fix so 1.2.8
is not broken and that suggestion might be done later.
We could also do:
#define virDriverLoadModule(x) virDriverLoadModuleFull(x, 0)
and then load each driver like this, for example:
virDriverLoadModuleFull("vbox", VIR_DRIVER_NETWORK);
Anyway, back to the stuff that's relevant for 1.2.8...
Everything looks fine from my POV and I tested what I could. I,
however, have neither vbox nor xen installed to test what were the
issues in the first place, so I would like to ask whomever reported
the issue or uses those drivers to let us know before we merge this.
ACK series, but we should wait until at least rc0 to push this. If
nobody else replies, I'd merge this into rc0 to let others test it
before the actual release.
Martin
1:04 a.m.
New subject: [libvirt] [PATCH 3/3] vbox: Register per partes
On Mon, Aug 25, 2014 at 06:37:50PM +0200, Martin Kletzander wrote:
On Fri, Aug 22, 2014 at 11:37:52AM +0200, Michal Privoznik wrote:
>Since times when vbox moved to the daemon (due to some licensing
>issue) the subdrivers that vbox implements were registered, but not
>opened since our generic subdrivers took priority. I've tried to fix
>this in 65b7d553f39ff9 but it was not correct. Apparently moving
>vbox driver registration upfront changes the default connection URI
>which makes some users sad. So, this commit breaks vbox into pieces
>and register vbox's network and storage drivers first, and vbox driver
>then at the end. This way, the vbox driver is registered in the order
>it always was, but its subdrivers are registered prior the generic
>ones.
>
>Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>---
> daemon/libvirtd.c | 16 ++++++++++++++--
> src/Makefile.am | 41 ++++++++++++++++++++++++++++++++++++++---
> src/vbox/vbox_driver.c | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
> src/vbox/vbox_driver.h | 10 ++++++++++
> 4 files changed, 107 insertions(+), 10 deletions(-)
>
[...]
>diff --git a/src/Makefile.am b/src/Makefile.am
>index 538530e..46e411e 100644
>--- a/src/Makefile.am
>+++ b/src/Makefile.am
>@@ -1135,13 +1135,27 @@ libvirt_driver_vmware_la_SOURCES = $(VMWARE_DRIVER_SOURCES)
> endif WITH_VMWARE
>
> if WITH_VBOX
>-noinst_LTLIBRARIES += libvirt_driver_vbox_impl.la
>+noinst_LTLIBRARIES += \
>+ libvirt_driver_vbox_impl.la \
>+ libvirt_driver_vbox_network_impl.la \
>+ libvirt_driver_vbox_storage_impl.la
> libvirt_driver_vbox_la_SOURCES =
> libvirt_driver_vbox_la_LIBADD = libvirt_driver_vbox_impl.la
>+libvirt_driver_vbox_network_la_SOURCES =
>+libvirt_driver_vbox_network_la_LIBADD = libvirt_driver_vbox_network_impl.la
>+libvirt_driver_vbox_storage_la_SOURCES =
>+libvirt_driver_vbox_storage_la_LIBADD = libvirt_driver_vbox_storage_impl.la
Couldn't you just do:
libvirt_driver_vbox_network_la_LIBADD = libvirt_driver_vbox_impl.la
libvirt_driver_vbox_storage_la_LIBADD = libvirt_driver_vbox_impl.la
Or just symlink these to the original one? Of course you would export
all three register symbols, but just use the one you want for each
load. Or am I missing something why that wouldn't work?
[reorganizing the hunks so my responses follow logically]
>diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
>index 4cf78e6..c3bd2ab 100644
>--- a/daemon/libvirtd.c
>+++ b/daemon/libvirtd.c
>@@ -383,7 +383,7 @@ static void daemonInitialize(void)
> * is not loaded they'll get a suitable error at that point
> */
> # ifdef WITH_VBOX
>- virDriverLoadModule("vbox");
>+ virDriverLoadModule("vbox_network");
> # endif
It would look a bit nicer, I guess, if we just loaded the symbols in
virDriverLoadModule() into some kind of table (or list) and then
register them later on, but I understand this is just a fix so 1.2.8
is not broken and that suggestion might be done later.
We could also do:
#define virDriverLoadModule(x) virDriverLoadModuleFull(x, 0)
and then load each driver like this, for example:
virDriverLoadModuleFull("vbox", VIR_DRIVER_NETWORK);
Anyway, back to the stuff that's relevant for 1.2.8...
Everything looks fine from my POV and I tested what I could. I,
however, have neither vbox nor xen installed to test what were the
issues in the first place, so I would like to ask whomever reported
the issue or uses those drivers to let us know before we merge this.
ACK series, but we should wait until at least rc0 to push this. If
Of course, I meant -rc1.
nobody else replies, I'd merge this into rc0 to let others test
it
before the actual release.
I can't find the original reporter of the issue, Michal also failed to
add the link into the cover letter, so I have nobody to ask.
I'm pushing this as -rc1 is approaching, so others can test it as
early as possible.
I'll also squash in the following fix for spec-file:
diff --git i/libvirt.spec.in w/libvirt.spec.in
index 9126277..b7a26a1 100644
--- i/libvirt.spec.in
+++ w/libvirt.spec.in
@@ -2094,6 +2094,8 @@ exit 0
%files daemon-driver-vbox
%defattr(-, root, root)
%{_libdir}/%{name}/connection-driver/libvirt_driver_vbox.so
+%{_libdir}/%{name}/connection-driver/libvirt_driver_vbox_network.so
+%{_libdir}/%{name}/connection-driver/libvirt_driver_vbox_storage.so
%endif
%endif # %{with_driver_modules}
--
Martin
3993
days inactive
3998
days old
5 comments
2 participants
participants (2)
-
Martin Kletzander -
Michal Privoznik