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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
