Hi Daniel,
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.
Sorry, would this from next time.
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
yes, in virtualbox each user can run their own machines.
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.
Thanks
- 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
Will take care of above two points.
- 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
The reason is that this way the libvirt implementation is self contained and
VirtualBox support can be enabled by default. The second reason is that the
VirtualBox API tends to change in incompatible ways so future libvirt patches
will be able to deal with multiple versions of VirtualBox and therefore also
have multiple header files.
- 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];
In vbox nsID is implemented as:
struct nsID {
PRUint32 m0;
PRUint16 m1;
PRUint16 m2;
PRUint8 m3[8];
};
while in libvirt it is implemented as unsigned char uuid[16];
On little endian machines this creates byte swaps as shown above
and thus the workaround.
(The
http://www.ietf.org/rfc/rfc4122.txt , section 4.1.2 shows the layout and
byte order)
- 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.
Will take care of this.
- 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.
Will take care of this.
- 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.
for time being i will remove these.
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...
Thanks for such verbose review.
and yes, it is nice to work with the libvirt community, will post more when I
get things working.
Regards,
-pritesh