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

[John Ferlan]

On 10/24/2017 03:35 PM, 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;
>      }

Existing, but the { } here aren't necessary since there's only one
statement after the if. It escapes the syntax-check check because
there's two lines in the function call...

>  
> +    VBOX_IID_INITIALIZE(&mchiid);
>      virUUIDFormat(def->uuid, uuidstr);
>  
>      rc = gVBoxAPI.UIVirtualBox.CreateMachine(data, def, &machine, uuidstr);

And as I assume you know - having this go to cleanup is going to cause a
problem since it's designed to clean up on failure assuming that this
succeeded, right?

I would have been OK with a posted diff to squash in...

In any case, at this point only @def and @mchiid would need to be
cleaned up. If you move the mchiid generation to after this line, then
the failure scenario here could be virDomainDefFree(def) and return ret;
(or NULL).

Whichever way works - the rest of the code looks OK, but I can wait for
the next version.

Since Patches 1 & 2 are OK and for the most part Patches 4-9 are fine
and would seem to be "movable" to just before current patch 10. That
patch also adjusts vboxDomainDefineXMLFlags, so for me it became a logic
point to at least be able to push some of the patches before you post a
a v3. For the nits found in patch 4-9 - I can make the simple
adjustments noted before pushing. That way the next series is smaller.

John

> @@ -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);
>      if (NS_FAILED(rc)) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("failed no saving settings, rc=%08x"), (unsigned)rc);
> -        goto cleanup;
> +                       _("Failed to save VM settings, rc=%08x"), rc);
> +        machineReady = false;
>      }
>  
>      gVBoxAPI.UISession.Close(data->vboxSession);
> -    vboxIIDUnalloc(&mchiid);
> -
> -    ret = virGetDomain(conn, def->name, def->uuid, -1);
> -    VBOX_RELEASE(machine);
>  
> -    virDomainDefFree(def);
> +    if (machineReady) {
> +        ret = virGetDomain(conn, def->name, def->uuid, -1);
> +    } else {
> +        /* Unregister incompletely configured VM to not leave garbage behind */
> +        rc = gVBoxAPI.unregisterMachine(data, &mchiid, &machine);
>  
> -    return ret;
> +        if (NS_SUCCEEDED(rc))
> +            gVBoxAPI.deleteConfig(machine);
> +        else
> +            VIR_WARN("Could not cleanup partially created VM after failure, "
> +                     "rc=%08x", rc);
> +    }
>  
> - cleanup:
> +    vboxIIDUnalloc(&mchiid);
>      VBOX_RELEASE(machine);
>      virDomainDefFree(def);
> -    return NULL;
> +
> +    return ret;
>  }
>  
>  static virDomainPtr
>