[libvirt] [PATCH] vbox: Handle different IID representation in Version 2.2 on Windows

[Matthias Bolte]

2010/12/23 Eric Blake <eblake at redhat.com>:
> On 12/23/2010 01:38 PM, Matthias Bolte wrote:
>> On Windows IID's are represented as GUID by value, instead of nsID
>> by reference on non-Windows platforms.
>>
>> Patch the vbox_CAPI_v2_2.h header to deal with this difference.
>>
>> Rewrite vboxIID abstraction that deals with the different IID
>> representations. Add support for the GUID representation. Also unify
>> the four context dependent free functions for vboxIIDs
>>
>>   vboxIIDUnalloc, vboxIIDFree, vboxIIDUtf8Free, vboxIIDUtf16Free
>>
>> into vboxIIDUnalloc that is now safe to be called (even multiple
>> times) on a vboxIID independent of the source and context of the
>> vboxIID.
>
> Nice - and it cuts down on a lot of #ifdefs in the code base, as an
> added bonus.
>
>>
>> The new vboxIID is designed to be used as a stack allocated variable.
>> It has a value member that represents the actual IID value.
>
>> +++ b/src/vbox/vbox_CAPI_v2_2.h
>> @@ -57,8 +57,12 @@
>>
>>  #  ifdef WIN32
>>  #   define PR_COM_METHOD __stdcall
>> +#   define PR_IID_IN_TYPE GUID
>> +#   define PR_IID_OUT_TYPE GUID *
>>  #  else
>>  #   define PR_COM_METHOD
>> +#   define PR_IID_IN_TYPE const nsID *
>> +#   define PR_IID_OUT_TYPE nsID **
>>  #  endif
>
> How annoying, but this macro looks like the right way to solve it, and
> the bulk of the patch to this file is mechanical.  Also, your (three!)
> wrappers in vbox_tmpl.c look sane, and everything after the hunk for
> line 1044 of the original file looks pretty mechanical.  I assume you
> compiled for both Windows and Linux for both 2.2 and 3.0+ versions.

Yes, this is compile tested on Linux and Windows. I did some runtime
tests on Windows and Linux for 2.2 and 3.2. So I'm quite confident
that I didn't break it :)

>> @@ -7051,7 +6951,7 @@ static virNetworkPtr vboxNetworkDefineCreateXML(virConnectPtr conn, const char *
>>       */
>>
>>  #if VBOX_API_VERSION == 2002
>> -    if STREQ(def->name, "vboxnet0") {
>> +    if (STREQ(def->name, "vboxnet0")) {
>
> Wow - how long has that been under-parenthesized (I guess since STREQ
> parenthesizes its results, it still compiled)?
>
>> @@ -8032,13 +7914,27 @@ static int vboxStorageVolDelete(virStorageVolPtr vol,
>>              vboxArrayGet(&machineIds, hardDisk, hardDisk->vtbl->GetMachineIds);
>>  #endif /* VBOX_API_VERSION >= 3001 */
>>
>> +#if VBOX_API_VERSION == 2002 && defined WIN32
>> +            /* VirtualBox 2.2 on Windows represents IIDs as GUIDs and the
>> +             * machineIds array contains direct instances of the GUID struct
>> +             * instead of pointers to the actual struct instances. But there
>> +             * is no 128bit width simple item type for a SafeArray to fit a
>> +             * GUID in. The largest simple type it 64bit width and VirtualBox
>> +             * uses two of this 64bit items to represents one GUID. Therefore,
>> +             * we devide the size of the SafeArray by two, to compensate for
>> +             * this workaround in VirtualBox */
>> +            machineIds.count /= 2;
>> +#endif /* VBOX_API_VERSION >= 2002 */
>
> OK, so that hunk wasn't mechanical, like most of them.  But overall, it
> looks good.

This one took quite a while digging the VirtualBox source code until I
figured out why deleting a volume didn't work :)

> ACK.
>

Thanks, pushed.

Matthias