Re: [libvirt] [PATCH v2 03/15] vbox: Cleanup partially-defined VM on failure
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 03/15] vbox: Cleanup partially-defined VM on failure
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.
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 03/15] vbox: Cleanup partially-defined VM on failure
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);
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
--
2.14.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
