Re: [PATCH 02/11] libxl: use job functions in libxlVmStart

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 07.02.2014 04:53, Jim Fehlig wrote:
> Creating a large domain could potentially be time consuming.  Use the
> recently added job functions and unlock the virDomainObj while
> the create operation is in progress.
>
> Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
> ---
>   src/libxl/libxl_driver.c | 59 ++++++++++++++++++++++++++----------------------
>   1 file changed, 32 insertions(+), 27 deletions(-)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 8e4242a..7c964c5 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -593,7 +593,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>       virDomainDefPtr def = NULL;
>       virObjectEventPtr event = NULL;
>       libxlSavefileHeader hdr;
> -    int ret;
> +    int ret = -1;
>       uint32_t domid = 0;
>       char *dom_xml = NULL;
>       char *managed_save_path = NULL;
> @@ -605,14 +605,17 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>   #endif
>
>       if (libxlDomainObjPrivateInitCtx(vm) < 0)
> -        goto error;
> +        goto cleanup;
> +
> +    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
> +        goto cleanup;
>
>       /* If there is a managed saved state restore it instead of starting
>        * from scratch. The old state is removed once the restoring succeeded. */
>       if (restore_fd < 0) {
>           managed_save_path = libxlDomainManagedSavePath(driver, vm);
>           if (managed_save_path == NULL)
> -            goto error;
> +            goto endjob;
>
>           if (virFileExists(managed_save_path)) {
>
> @@ -620,7 +623,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>                                                    managed_save_path,
>                                                    &def, &hdr);
>               if (managed_save_fd < 0)
> -                goto error;
> +                goto endjob;
>
>               restore_fd = managed_save_fd;
>
> @@ -634,7 +637,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>                                  _("cannot restore domain '%s' uuid %s from a file"
>                                    " which belongs to domain '%s' uuid %s"),
>                                  vm->def->name, vm_uuidstr, def->name, def_uuidstr);
> -                goto error;
> +                goto endjob;
>               }
>
>               virDomainObjAssignDef(vm, def, true, NULL);
> @@ -652,17 +655,17 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>       libxl_domain_config_init(&d_config);
>
>       if (libxlBuildDomainConfig(driver, vm, &d_config) < 0)
> -        goto error;
> +        goto endjob;
>
>       if (cfg->autoballoon && libxlFreeMem(priv, &d_config) < 0) {
>           virReportError(VIR_ERR_INTERNAL_ERROR,
>                          _("libxenlight failed to get free memory for domain '%s'"),
>                          d_config.c_info.name);
> -        goto error;
> +        goto endjob;
>       }
>
> -    /* use as synchronous operations => ao_how = NULL and no intermediate reports => ao_progress = NULL */
> -
> +    /* Unlock virDomainObj while creating the domain */
> +    virObjectUnlock(vm);
>       if (restore_fd < 0) {
>           ret = libxl_domain_create_new(priv->ctx, &d_config,
>                                         &domid, NULL, NULL);
> @@ -676,6 +679,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>                                             restore_fd, NULL, NULL);
>   #endif
>       }
> +    virObjectLock(vm);
>
>       if (ret) {
>           if (restore_fd < 0)
> @@ -686,25 +690,25 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>               virReportError(VIR_ERR_INTERNAL_ERROR,
>                              _("libxenlight failed to restore domain '%s'"),
>                              d_config.c_info.name);
> -        goto error;
> +        goto endjob;
>       }
>
>       vm->def->id = domid;
>       if (libxlDomEventsRegister(vm) < 0)
> -        goto error;
> +        goto endjob;
>
>       if ((dom_xml = virDomainDefFormat(vm->def, 0)) == NULL)
> -        goto error;
> +        goto endjob;
>
>       if (libxl_userdata_store(priv->ctx, domid, "libvirt-xml",
>                                (uint8_t *)dom_xml, strlen(dom_xml) + 1)) {
>           virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                          _("libxenlight failed to store userdata"));
> -        goto error;
> +        goto endjob;
>       }
>
>       if (libxlDomainSetVcpuAffinities(driver, vm) < 0)
> -        goto error;
> +        goto endjob;
>
>       if (!start_paused) {
>           libxl_domain_unpause(priv->ctx, domid);
> @@ -715,7 +719,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>
>
>       if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
> -        goto error;
> +        goto endjob;
>
>       if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback)
>           driver->inhibitCallback(true, driver->inhibitOpaque);
> @@ -727,25 +731,26 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>       if (event)
>           libxlDomainEventQueue(driver, event);
>
> -    libxl_domain_config_dispose(&d_config);
> -    VIR_FREE(dom_xml);
> -    VIR_FORCE_CLOSE(managed_save_fd);
> -    virObjectUnref(cfg);
> -    return 0;
> +    ret = 0;
>
> -error:
> -    if (domid > 0) {
> -        libxl_domain_destroy(priv->ctx, domid, NULL);
> -        vm->def->id = -1;
> -        virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_FAILED);
> +endjob:
> +    libxlDomainObjEndJob(driver, vm);

See my comment in the next patch where my concern about this pattern is 
more visible IMO.

> +
> +cleanup:
> +    if (ret) {

s/ret/ret < 0/ please

> +        if (domid > 0) {
> +            libxl_domain_destroy(priv->ctx, domid, NULL);
> +            vm->def->id = -1;
> +            virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_FAILED);
> +        }
> +        virDomainDefFree(def);
>       }
>       libxl_domain_config_dispose(&d_config);
>       VIR_FREE(dom_xml);
>       VIR_FREE(managed_save_path);
> -    virDomainDefFree(def);
>       VIR_FORCE_CLOSE(managed_save_fd);
>       virObjectUnref(cfg);
> -    return -1;
> +    return ret;
>   }

ACK

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list