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

[John Ferlan]

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

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

>      rc = vboxArrayGetWithUintArg(&media, *machine, (*machine)->vtbl->Unregister,
> -                                 CleanupMode_DetachAllReturnNone);
> +                                 CleanupMode_DetachAllReturnHardDisksOnly);
> +
> +    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.

> +        /* 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;
>  }
>