[libvirt] [PATCH v2 03/15] vbox: Cleanup partially-defined VM on failure

[Dawid Zamirski]

On Tue, 2017-10-24 at 15:35 -0400, Dawid Zamirski wrote:
> Since the VBOX API requires to register an initial VM before
> proceeding
> to attach any remaining devices to it, any failure to attach such
> devices should result in automatic cleanup of the initially
> registered
> VM so that the state of VBOX registry remains clean without any
> leftover
> "aborted" VMs in it. Failure to cleanup of such partial VMs results
> in a
> warning log so that actual define error stays on the top of the error
> stack.
> ---
>  src/vbox/vbox_common.c | 41 +++++++++++++++++++++++++++-------------
> -
>  1 file changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> index 92ee37164..812c940e6 100644
> --- a/src/vbox/vbox_common.c
> +++ b/src/vbox/vbox_common.c
> @@ -1853,6 +1853,8 @@ vboxDomainDefineXMLFlags(virConnectPtr conn,
> const char *xml, unsigned int flags
>      char uuidstr[VIR_UUID_STRING_BUFLEN];
>      virDomainPtr ret = NULL;
>      unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
> +    bool machineReady = false;
> +
>  
>      virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
>  
> @@ -1862,12 +1864,12 @@ vboxDomainDefineXMLFlags(virConnectPtr conn,
> const char *xml, unsigned int flags
>      if (!data->vboxObj)
>          return ret;
>  
> -    VBOX_IID_INITIALIZE(&mchiid);
>      if (!(def = virDomainDefParseString(xml, data->caps, data-
> >xmlopt,
>                                          NULL, parse_flags))) {
> -        goto cleanup;
> +        return ret;
>      }
>  
> +    VBOX_IID_INITIALIZE(&mchiid);
>      virUUIDFormat(def->uuid, uuidstr);
>  
>      rc = gVBoxAPI.UIVirtualBox.CreateMachine(data, def, &machine,
> uuidstr);
> @@ -1959,30 +1961,41 @@ vboxDomainDefineXMLFlags(virConnectPtr conn,
> const char *xml, unsigned int flags
>      vboxAttachUSB(def, data, machine);
>      vboxAttachSharedFolder(def, data, machine);
>  
> -    /* Save the machine settings made till now and close the
> -     * session. also free up the mchiid variable used.
> +    machineReady = true;
> +
> + cleanup:
> +    /* Save the machine settings made till now, even when jumped
> here on error,
> +     * as otherwise unregister won't cleanup properly. For example,
> it won't
> +     * close media that were partially attached. The VBOX SDK docs
> say that
> +     * unregister implicitly calls saveSettings but evidently it's
> not so...
>       */
>      rc = gVBoxAPI.UIMachine.SaveSettings(machine);

There's one more code path in this function that causes a segfault here
due to machine == NULL (e.g. when CreateMachine fails). I already have
patched this but I'll hold off with sending V3 until this series is
reviewed and feedback is available.

>