On 11/03/2017 12:19 PM, Dawid Zamirski wrote:
> On Fri, 2017-11-03 at 09:43 -0400, John Ferlan wrote:
>>
>> On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
>>> This commit primes vboxAttachDrives for further changes so when
>>> they
>>> are made, the diff is less noisy:
>>>
>>> * move variable declarations to the top of the function
>>> * add disk variable to replace all the def->disks[i] instances
>>> * add cleanup at the end of the loop body, so it's all in one place
>>> rather than scattered through the loop body. It's purposefully
>>> called 'cleanup' rather than 'skip' or 'continue' because future
>>> commit will treat errors as hard-failures.
>>> ---
>>> src/vbox/vbox_common.c | 95 ++++++++++++++++++++++++------------
>>> --------------
>>> 1 file changed, 46 insertions(+), 49 deletions(-)
>>>
>>> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
>>> index fa8471e68..b949c4db7 100644
>>> --- a/src/vbox/vbox_common.c
>>> +++ b/src/vbox/vbox_common.c
>>> @@ -959,8 +959,17 @@ static void
>>> vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
>>> *machine)
>>> {
>>> size_t i;
>>> + int type, format;
>>> + const char *src = NULL;
>>> nsresult rc = 0;
>>> + virDomainDiskDefPtr disk = NULL;
>>> PRUnichar *storageCtlName = NULL;
>>> + IMedium *medium = NULL;
>>> + PRUnichar *mediumFileUtf16 = NULL, *mediumEmpty = NULL;
>>> + PRUint32 devicePort, deviceSlot, deviceType, accessMode;
>>> + vboxIID mediumUUID;
>>> +
>>> + VBOX_IID_INITIALIZE(&mediumUUID);
>>
>> The cleanup: label would clean this up on the first pass through the
>> loop, so it needs to move outside the last }.
>>
>
> The VBOX_IID_INITIALIZE macro is a simple struct initializer that is
> equivalent of vboxIID mediumUUID = {NULL, true}; as a matter of fact,
> src/vbox/vbox_tmpl.c uses VBOX_IID_INITIALIZER macro which is exactly
> that, but that macro is not available to vbox_common.c This is likely
> another remanent of VBOX <= 3 support that needs to be cleaned up - I
> will do so in a separate patch as it will touch to many parts of the
> driver and when you take a closer look we don't really need vboxIID
> struct at all and can simply use PRUnichar directly for VBOX UUIDs.
>
OK it really wasn't overly intuitively obvious what VBOX_IID_INITIALIZE
a/k/a gVBoxAPI.UIID.vboxIIDInitialize really did, your explanation makes
sense.
>> I can do this for you; however, I have a question - something I
>> noticed
>> while reviewing patch 7. There's a call:
>>
>> rc = gVBoxAPI.UIMedium.GetId(medium, &mediumUUID);
>>
>> for which it wasn't clear whether the mediumUUID is overwritten. If
>> it
>> is, then although existing - it probably needs some cleanup and of
>> course would change the flow a bit to doing that initializer each
>> time
>> through the loop.
>
> Yes, it is overwritten but since the cleanup: label is at the end of
> the loop body it will set mediumUIID to {NULL, true} which is the same
> as calling VBOX_IID_INITIALIZE (see _vboxIIDUnalloc in
> src/vbox/vbox_tmpl.c)
>
>>
>> Moving it does have a ripple effect on subsequent patches, but it's
>> manageable.
>>
>> If moving it to the end is acceptable, then
>
> As per above, I'd rather leave this patch "as is" as the (future)
> driver cleanup patches for vboxIID will make this clearer.
>
OK I'll leave it as is. Patches 1-2 and 4-9 are thus all R-B and will
be pushed in a little while.
John
>>
>> Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
>>
>> otherwise let me know and I'll let you repost as part of a v3
>>
>> John
>>
[...]
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
