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

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.

> 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.

> > Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; > > otherwise let me know and I'll let you repost as part of a v3 > > John >