Re: [libvirt] [PATCH 1/6] vbox: Close media when undefining domains.
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(a)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.
> 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.
> + /* 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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list