Re: [libvirt] VirtualBox support to libvirt

On Fri, Mar 06, 2009 at 04:27:53PM +0100, Pritesh Kothari wrote: 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. 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 :|