[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] VirtualBox support to libvirt



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]