[libvirt] [PATCH 3/3] vbox: Register per partes

[Martin Kletzander]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140827/a4db4a88/attachment-0001.sig>