[libvirt] [PATCH 1/6] vbox: Close media when undefining domains.

[John Ferlan]

On 10/17/2017 05:28 PM, Dawid Zamirski wrote:
> On Tue, 2017-10-17 at 15:44 -0400, John Ferlan wrote:
>>
>> On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
>>> From: Dawid Zamirski <dzamirski at datto.com>
>>>
>>> When registering a VM we call OpenMedium on each disk image which
>>> adds it
>>> to vbox's global media registry. Therefore, we should make sure to
>>> call
>>> Close when unregistering VM so we cleanup the media registry
>>> entries
>>> after ourselves - this does not remove disk image files. This
>>> follows
>>> the behaviour of the VBoxManage unregistervm command.
>>> ---
>>>  src/vbox/vbox_tmpl.c | 24 +++++++++++++++++++-----
>>>  1 file changed, 19 insertions(+), 5 deletions(-)
>>>
>>
>> I'm not a vbox expert by any stretch, looking at this mostly because
>> it's been sitting on list unreviewed...
> 
> Thank you :-)
> 
>>
>>> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
>>> index dffeabde0..ac3b8fa00 100644
>>> --- a/src/vbox/vbox_tmpl.c
>>> +++ b/src/vbox/vbox_tmpl.c
>>> @@ -378,6 +378,8 @@ _unregisterMachine(vboxDriverPtr data, vboxIID
>>> *iid, IMachine **machine)
>>>  {
>>>      nsresult rc;
>>>      vboxArray media = VBOX_ARRAY_INITIALIZER;
>>> +    size_t i;
>>> +
>>>      rc = data->vboxObj->vtbl->FindMachine(data->vboxObj, iid-
>>>> value, machine);
>>>      if (NS_FAILED(rc)) {
>>>          virReportError(VIR_ERR_NO_DOMAIN, "%s",
>>> @@ -385,12 +387,24 @@ _unregisterMachine(vboxDriverPtr data,
>>> vboxIID *iid, IMachine **machine)
>>>          return rc;
>>>      }
>>>  
>>> -    /* We're not interested in the array returned by the
>>> Unregister method,
>>> -     * but in the side effect of unregistering the virtual
>>> machine. In order
>>> -     * to call the Unregister method correctly we need to use the
>>> vboxArray
>>> -     * wrapper here. */
>>
>> I think you should keep the second sentence here - "In order to
>> call..."
>>  - IDC either way, but that would seem to still be important.
>>
> 
> Since this patch adds a code that loops over this array to close each
> IMedium instance it's self-documenting so no there's need for code
> comments here.
> 

That's fine - for you I'm assuming it's obvious why the wrapper is
needed to call Unregister, but for me - less so.

>>>      rc = vboxArrayGetWithUintArg(&media, *machine, (*machine)-
>>>> vtbl->Unregister,
>>> -                                 CleanupMode_DetachAllReturnNone);
>>> +                                 CleanupMode_DetachAllReturnHardDi
>>> sksOnly);
>>> +
>>> +    if (NS_FAILED(rc))
>>> +        goto cleanup;
>>> +
>>> +    /* close each medium attached to VM to remove from media
>>> registry */
>>> +    for (i = 0; i < media.count; i++) {
>>> +        IMedium *medium = media.items[i];
>>> +
>>> +        if (!medium)
>>> +            continue;
>>> +
>>
>> So the vboxSnapshotRedefine and vboxDomainSnapshotDeleteMetadataOnly
>> APIs that use CleanupMode_DetachAllReturnHardDisksOnly seem to go
>> through some great pain to determine whether it's a "fake" disk or
>> not -
>> would this code need the same kind of logic?
>>
>> /me wonders how much the existing code could be made more common -
>> beyond the "isCurrent" in the latter function the code appears to be
>> the
>> same. If this code needs it, then it might be worth the effort to
>> pass
>> 'isCurrent' to some common helper and for the other caller, just pass
>> true instead... Which would be similar if this code needed that.
> 
> I intentionally tried to stay away from touching the snapshot related
> functions even though it seems that they could share some code. Since
> I've been working on this on and off between projects at work, at
> initial stages it seemed like a couple of lines of code "here and
> there" would let me set/change vbox storage controller models via
> libvirtxml. Unfortunately, as I dug deeper and tested all sorts of
> combinations more bugs (even in master branch) seemed to pop out and
> the series grew to be much bigger than I originally intended. However,
> since I'll have to reorganize the commits for V2 anyway, I'll take a
> look at those functions and see what I can do.
> 

I try to avoid snapshot too ;-)  I understand how making adjustments in
libvirt can be like peeling back layers of an onion that cause you to
cry each time you have to enter some other piece of code because either
you discover some new bug/issue or what you're doing affects some other
piece of code. That is though one reason why we encourage using "small
enough" functionality changing/fixing patches rather than some massive
patch doing many different things. Another reason for smaller patches
has to do with git bisect and being able to determine which patch broke
some functionality - you bisect back to some 800 line patch bomb and
attempt to figure out what caused the failure...  Many times what I find
myself doing when I trip across some existing issue is stashing work and
rebasing to the patch before I've stashed, generating a patch for the
issue I uncover and then dealing with the merge conflicts after that.

John

>>
>>> +        /* it's ok to ignore failure here - e.g. it may be used by
>>> another VM */
>>> +        medium->vtbl->Close(medium);
>>
>> You could place an ignore_value(); around this. Again see the two
>> API's
>> above they have a detailed explanation.
>>
>>
>> John
>>
>>> +    }
>>> +
>>> + cleanup:
>>>      vboxArrayUnalloc(&media);
>>>      return rc;
>>>  }
>>>
>>
>> --
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list