[libvirt] VirtualBox support to libvirt

[Daniel P. Berrange]

On Fri, Mar 06, 2009 at 04:27:53PM +0100, Pritesh Kothari wrote:
> Hi All,
> 
> I have attached a patch which when applied on the HEAD would
> allow virtualbox support in libvirt.
> 
> The steps to apply patch are:
> tar -zxvf vbox-patch.tar.gz
> git-apply vbox-patch

Minor point for next time - it'd make it a little quicker to review 
if could just attach the plain vbox.patch file directly to the mail. 
If it hits the maximum attachment size allowed by mailman, one of
us can easily approve the mail, or just split it into sections.

> The patch works very well with the VirtualBox OSE svn HEAD and
> is supposed to support VirtualBox 2.2 onwards both editions.
> 
> If anyone wants to run it on current VirtualBox OSE svn HEAD
> (2.1.51_OSE r17375) then the steps are as follows:
> 
> 1)After compiling and installing VBox OSE you need to copy the
> file out/linux.amd64/debug/bin/VBoxXPCOMC.so to the install
> directory.
> 2) export VBOX_APP_HOME=<install dir>
> 3) run virsh to connect to virtualbox as below:
> virsh -c vbox:///session

I'm not too familiar with virtualbox - am I right in thinking that each
user can run their own set of virtual machines, independantly of others
If so, then your choice of URLs vbox:///session fits perfectly :-)

I've had a quick look at the patch & it basically looks to be developing
well. I will just list a few pretty minor suggestions or questions here
for now, rather than doing a full inline patch review.

 - There's a few places using of malloc/realloc/free, rather than
   VIR_ALLOC/REALLOC_N/FREE. 

 - There's a couple of places doing sizeof(s_aSyms) / sizeof(s_aSyms[0])
   In util.h, there is a convenient macro ARRAY_CARDINAILITY(s_aSyms)
   that lets you do this
 
 - The general integration bits you've done in Makefile.am, src/libvirt.c
   configure.ac and src/virterror.c all look correct 

 - Does the src/vbox/VBoxCAPI_v2_2.h file need to be present in the
   source tree ?  I would have expected the VirtualBox-devel RPm (or
   equivalent to be providing the header file definitions for the
   API interface, but perhaps I'm mis-understanding the purpose of
   this file

 - I'm curious as to why you're changing the UUID format in nsIDtoChar,
   switching the positions of bytes in the UUID ?

     +    uuidstrdst[0]  = uuidstrsrc[6];
     +    uuidstrdst[1]  = uuidstrsrc[7];
     +    uuidstrdst[2]  = uuidstrsrc[4];
     +    uuidstrdst[3]  = uuidstrsrc[5];
     +    uuidstrdst[4]  = uuidstrsrc[2];
     +    uuidstrdst[5]  = uuidstrsrc[3];
     +    uuidstrdst[6]  = uuidstrsrc[0];
     +    uuidstrdst[7]  = uuidstrsrc[1];

 - When creating virDomainPtr objects, rather than directly allocating
   them, and then setting the id, uuid, and name fields, we have a
   helper method in src/datatypes.h you can use:

       virDomainPtr virGetDomain(virConnectPtr conn,
                                 const char *name,
                                 const unsigned char *uuid);

   This avoids the need to worry about the fine details of the ref
   counting & caching associated with these objects. So generally
   shouldn't directly access the conn->domains hash table from driver
   code.

 - The vboxGetDomainGetOSType() method shouldn't actually return the
   name of the guest operating system. This is one of the badly named
   libvirt methods. What it is actually intended for is to give the
   name of the guest operating system hypervisor ABI. Since VirtualBox
   is a fully-virtualized hypervisor, it should always just return 'hvm'
   for the OS type.

 - The vboxCapsInit() should also pass 'hvm' as the OS type field,
   rather than 'exe'which is for container based virt like OpenVZ.
   Also in that method, "sizeof(int) == 4 ? 32 : 8" - i think that 
   8 should probably be 64 :-)

 - In the vboxGetDomainDefineXML(), there's a couple of places accessing
   def->features bits - you could use the VIR_DOMAIN_FEATURE_* enums
   for those.

 - Since VirtualBox is a local machine  driver, there's probably no need 
   to register virNetworkDriver, virStorageDriver, virDeviceMonitor driver
   structs with libvirt.  Just let the generic shared implementation 
   activate - we re-use this shared network & storage impl across all our
   drivers at this time.

   Then again if the VirtualBox  API provides explicit APIs for managing
   LVM, SCSI, iSCSI storage and NAT based networks, then perhaps it 
   would be worth having an impl of these drivers for VirtualBox. I'd
   still just leave them out for now though, until the main VirtualBox
   hypervisor APIs are more developed.

All in all, it looks like you've got a pretty good understanding of
what to do with VirtualBox for libvirt driver support. Look forward 
to seeing future versions of the patch...

Regards,
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 :|