[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]