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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list