[libvirt] [PATCH v2 10/11] libxl: Grab modify job for changing domain XML

[Nikolay Shirokovskiy]

On 05.06.2019 12:09, Michal Privoznik wrote:
> The reasoning here is the same as in qemu driver fixed in
> previous commit. Long story short, changing an XML of a domain
> requires modify job to be acquired.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/libxl/libxl_domain.c    | 57 +++++++++++++++++++++++++++++++++++++
>  src/libxl/libxl_domain.h    |  7 +++++
>  src/libxl/libxl_driver.c    | 24 ++++------------
>  src/libxl/libxl_migration.c | 14 +++------
>  4 files changed, 73 insertions(+), 29 deletions(-)
> 
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 2d8569e592..f2e1af52e5 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -65,6 +65,63 @@ libxlDomainObjPrivateOnceInit(void)
>  
>  VIR_ONCE_GLOBAL_INIT(libxlDomainObjPrivate);
>  
> +
> +/**
> + * libxlDomainObjListAdd:
> + * @driver: libxl driver
> + * @def: domain definition
> + * @oldDef: previous domain definition
> + * @live: whether @def is live definition
> + * @flags: an bitwise-OR of virDomainObjListAdd flags
> + *
> + * Add a domain onto the list of domain object and sets its
> + * definition. If @oldDef is not NULL and there was pre-existing
> + * definition it's returned in @oldDef.
> + *
> + * In addition to that, if definition of an existing domain is
> + * changed a MODIFY job is acquired prior to that.
> + *
> + * Returns: domain object pointer on success,
> + *          NULL otherwise.
> + */
> +virDomainObjPtr
> +libxlDomainObjListAdd(libxlDriverPrivatePtr driver,
> +                      virDomainDefPtr def,
> +                      virDomainDefPtr *oldDef,
> +                      bool live,
> +                      unsigned int flags)
> +{
> +    virDomainObjPtr vm = NULL;
> +
> +    if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, flags)))
> +        return NULL;
> +
> +    /* At this point, @vm is locked. If it doesn't have any
> +     * definition set, then the call above just added it and
> +     * there can't be anybody else using the object. It's safe to
> +     * just set the definition without acquiring job. */
> +    if (!vm->def) {
> +        virDomainObjAssignDef(vm, def, live, oldDef);
> +        VIR_RETURN_PTR(vm);
> +    }
> +
> +    /* Bad luck. Domain was pre-existing and this call is trying
> +     * to update its definition. Modify job MUST be acquired. */
> +    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
> +        if (!vm->persistent)
> +            virDomainObjListRemove(driver->domains, vm);
> +        virDomainObjEndAPI(&vm);
> +        return NULL;
> +    }
> +
> +    virDomainObjAssignDef(vm, def, live, oldDef);
> +
> +    libxlDomainObjEndJob(driver, vm);
> +
> +    return vm;
> +}
> +
> +
>  static int
>  libxlDomainObjInitJob(libxlDomainObjPrivatePtr priv)
>  {
> diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h
> index 769ee8ec4d..b5d5127e91 100644
> --- a/src/libxl/libxl_domain.h
> +++ b/src/libxl/libxl_domain.h
> @@ -83,6 +83,13 @@ extern const struct libxl_event_hooks ev_hooks;
>  int
>  libxlDomainObjPrivateInitCtx(virDomainObjPtr vm);
>  
> +virDomainObjPtr
> +libxlDomainObjListAdd(libxlDriverPrivatePtr driver,
> +                      virDomainDefPtr def,
> +                      virDomainDefPtr *oldDef,
> +                      bool live,
> +                      unsigned int flags);
> +
>  int
>  libxlDomainObjBeginJob(libxlDriverPrivatePtr driver,
>                         virDomainObjPtr obj,
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 809d298ac1..9c9a30bb90 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -601,12 +601,8 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
>      if (virUUIDParse("00000000-0000-0000-0000-000000000000", def->uuid) < 0)
>          goto cleanup;
>  
> -    if (!(vm = virDomainObjListAdd(driver->domains, def,
> -                                   driver->xmlopt,
> -                                   0)))
> +    if (!(vm = libxlDomainObjListAdd(driver, def, &oldDef, false, 0)))
>          goto cleanup;
> -
> -    virDomainObjAssignDef(vm, def, false, &oldDef);
>      def = NULL;
>  
>      vm->persistent = 1;
> @@ -1030,12 +1026,9 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml,
>      if (virDomainCreateXMLEnsureACL(conn, def) < 0)
>          goto cleanup;
>  
> -    if (!(vm = virDomainObjListAdd(driver->domains, def,
> -                                   driver->xmlopt,
> -                                   VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
> +    if (!(vm = libxlDomainObjListAdd(driver, def, NULL, true,
> +                                     VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
>          goto cleanup;
> -
> -    virDomainObjAssignDef(vm, def, true, NULL);
>      def = NULL;
>  
>      if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
> @@ -1950,12 +1943,9 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from,
>      if (virDomainRestoreFlagsEnsureACL(conn, def) < 0)
>          goto cleanup;
>  
> -    if (!(vm = virDomainObjListAdd(driver->domains, def,
> -                                   driver->xmlopt,
> +    if (!(vm = libxlDomainObjListAdd(driver, def, NULL, true,
>                                     VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))

just indentation, so

Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>