[libvirt] [PATCH v2 05/15] vbox: Cleanup vboxAttachDrives implementation

[John Ferlan]

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 at redhat.com>
>>
>> otherwise let me know and I'll let you repost as part of a v3
>>
>> John
>>

[...]