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