[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 04/13] Merge virDomainObjListIsDuplicate into virDomainObjListAdd



On Fri, Feb 01, 2013 at 11:18:26 +0000, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange redhat com>
> 
> The duplicate VM checking should be done atomically with
> virDomainObjListAdd, so shoud not be a separate function.
> Instead just use flags to indicate what kind of checks are
> required.
> 
...
> This pair, used in virDomainDefinXML:

s/Defin/Define/

> 
>    if (virDomainObjListIsDuplicate(privconn->domains, def, 0) < 0)
>      goto cleanup;
>    if (!(dom = virDomainObjListAdd(privconn->domains,
>                                    privconn->caps,
>                                    def, false)))
>      goto cleanup;
> 
> Changes to
> 
>    if (!(dom = virDomainObjListAdd(privconn->domains,
>                                    privconn->caps,
>                                    def,
>                                    0, NULL)))
>      goto cleanup;
...
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 3861e68..79da5eb 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1871,30 +1871,91 @@ void virDomainObjAssignDef(virDomainObjPtr domain,
>      }
>  }
>  
> +
> +
> +/*
> + *
> + * If flags & VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE then
> + * this will refuse updating an existing def if the
> + * current def is Live

It would be cool to have VIR_DOMAIN_OBJ_LIST_ADD_LIVE documented here
too :-)

> + *
> + */
>  virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
>                                      virCapsPtr caps,
>                                      const virDomainDefPtr def,
> -                                    bool live)
> +                                    unsigned int flags,
> +                                    virDomainDefPtr *oldDef)
>  {
> -    virDomainObjPtr domain;
> +    virDomainObjPtr vm;
>      char uuidstr[VIR_UUID_STRING_BUFLEN];
> +    if (oldDef)
> +        *oldDef = false;
>  
> -    if ((domain = virDomainObjListFindByUUID(doms, def->uuid))) {
> -        virDomainObjAssignDef(domain, def, live);
> -        return domain;
> -    }
> +    /* See if a VM with matching UUID already exists */
> +    if ((vm = virDomainObjListFindByUUID(doms, def->uuid))) {
> +        /* UUID matches, but if names don't match, refuse it */
> +        if (STRNEQ(vm->def->name, def->name)) {
> +            virUUIDFormat(vm->def->uuid, uuidstr);
> +            virReportError(VIR_ERR_OPERATION_FAILED,
> +                           _("domain '%s' is already defined with uuid %s"),
> +                           vm->def->name, uuidstr);

    virObjectUnlock(vm);
    vm = NULL;

> +            goto cleanup;
> +        }
>  
> -    if (!(domain = virDomainObjNew(caps)))
> -        return NULL;
> -    domain->def = def;
> +        if (flags & VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE) {
> +            /* UUID & name match, but if VM is already active, refuse it */
> +            if (virDomainObjIsActive(vm)) {
> +                virReportError(VIR_ERR_OPERATION_INVALID,
> +                               _("domain is already active as '%s'"),
> +                               vm->def->name);

    virObjectUnlock(vm);
    vm = NULL;

Given that you need to repeat the same code in three places in this
function, it might be worth a dedicated "duplicate" label.

> +                goto cleanup;
> +            }
> +        }
>  
> -    virUUIDFormat(def->uuid, uuidstr);
> -    if (virHashAddEntry(doms->objs, uuidstr, domain) < 0) {
> -        VIR_FREE(domain);
> -        return NULL;
> -    }

So the following if statement is effectively copying
virDomainObjAssignDef with a small addition which allows for returning
the replaced def in *oldDef. Given how hard to read the code is (mainly
because newDef is not a very good name) I'd prefer to keep it only once
and call enhanced virDomainObjAssignDef from here.

> +        if (virDomainObjIsActive(vm)) {
> +            if (oldDef)
> +                *oldDef = vm->newDef;
> +            else
> +                virDomainDefFree(vm->newDef);
> +            vm->newDef = def;
> +        } else {
> +            if (flags & VIR_DOMAIN_OBJ_LIST_ADD_LIVE) {
> +                if (!vm->newDef)
> +                    vm->newDef = vm->def;
> +                else
> +                    virDomainDefFree(vm->newDef);

This else branch is not in virDomainObjAssignDef and seems to be wrong.
If you call virDomainObjListAdd with VIR_DOMAIN_OBJ_LIST_ADD_LIVE twice
in a row, you will lose the original def (in vm->newDef). It looks like
virDomainObjAssignDef is not correct either as we either store vm->def
in vm->newDef or replace it without freeing. So I think you wanted to
virDomainDefFree vm->def rather than vm->newDef and also add this branch
to virDomainObjAssignDef. Another argument against copying the code
here.

> +            } else {
> +                if (oldDef)
> +                    *oldDef = vm->def;
> +                else
> +                    virDomainDefFree(vm->def);
> +            }
> +            vm->def = def;
> +        }
> +    } else {
> +        /* UUID does not match, but if a name matches, refuse it */
> +        if ((vm = virDomainObjListFindByName(doms, def->name))) {
> +            virUUIDFormat(vm->def->uuid, uuidstr);
> +            virReportError(VIR_ERR_OPERATION_FAILED,
> +                           _("domain '%s' already exists with uuid %s"),
> +                           def->name, uuidstr);
> +            virObjectUnlock(vm);
> +            vm = NULL;
> +            goto cleanup;
> +        }
>  
> -    return domain;
> +        if (!(vm = virDomainObjNew(caps)))
> +            goto cleanup;
> +        vm->def = def;
> +
> +        virUUIDFormat(def->uuid, uuidstr);
> +        if (virHashAddEntry(doms->objs, uuidstr, vm) < 0) {
> +            VIR_FREE(vm);

Shouldn't this be virObjectUnref(vm)?

> +            return NULL;
> +        }
> +    }
> +cleanup:
> +    return vm;
>  }
>  
>  /*
...
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 3ad1173..fa13e24 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1968,10 +1968,15 @@ virDomainChrDefPtr virDomainChrDefNew(void);
>  
>  /* live == true means def describes an active domain (being migrated or
>   * restored) as opposed to a new persistent configuration of the domain */

This commend would deserve some modifications :-)

> +enum {
> +    VIR_DOMAIN_OBJ_LIST_ADD_LIVE = (1 << 0),
> +    VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE = (1 << 1),
> +};
>  virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
>                                      virCapsPtr caps,
>                                      const virDomainDefPtr def,
> -                                    bool live);
> +                                    unsigned int flags,
> +                                    virDomainDefPtr *oldDef);
>  void virDomainObjAssignDef(virDomainObjPtr domain,
>                             const virDomainDefPtr def,
>                             bool live);
...
> diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
> index 6eacbca..84ba3d6 100644
> --- a/src/openvz/openvz_conf.c
> +++ b/src/openvz/openvz_conf.c
> @@ -642,17 +642,13 @@ int openvzLoadDomains(struct openvz_driver *driver) {
>          openvzReadMemConf(def, veid);
>  
>          virUUIDFormat(def->uuid, uuidstr);
> -        if (virDomainObjListIsDuplicate(driver->domains, def, true)) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR,
> -                           _("Duplicate container UUID %s detected for %d"),
> -                           uuidstr,
> -                           veid);
> -            goto cleanup;
> -        }
>          if (!(dom = virDomainObjListAdd(driver->domains,
>                                          driver->caps,
>                                          def,
> -                                        STRNEQ(status, "stopped"))))
> +                                        VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE |
> +                                        (STRNEQ(status, "stopped") ?
> +                                         VIR_DOMAIN_OBJ_LIST_ADD_LIVE : 0),

It might be easier to read to compute the flags separately and just use
the result here.

> +                                        NULL)))
>              goto cleanup;
>  
>          if (STREQ(status, "stopped")) {
...
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index a02e989..d6c6af5 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
...
> @@ -5615,26 +5608,14 @@ static virDomainPtr qemuDomainDefine(virConnectPtr conn, const char *xml) {
>      if (qemuDomainAssignAddresses(def, caps, NULL) < 0)
>          goto cleanup;
>  
> -    /* We need to differentiate two cases:
> -     * a) updating an existing domain - must preserve previous definition
> -     *                                  so we can roll back if something fails
> -     * b) defining a brand new domain - virDomainObjListAdd is just sufficient
> -     */
> -    if ((vm = virDomainObjListFindByUUID(driver->domains, def->uuid))) {
> -        if (virDomainObjIsActive(vm)) {
> -            def_backup = vm->newDef;
> -            vm->newDef = def;
> -        } else {
> -            def_backup = vm->def;
> -            vm->def = def;
> -        }
> -    } else {
> -        if (!(vm = virDomainObjListAdd(driver->domains,
> -                                       driver->caps,
> -                                       def, false))) {
> -            goto cleanup;
> -        }
> -    }
> +    if (!(vm = virDomainObjListAdd(driver->domains,
> +                                   driver->caps,
> +                                   def,
> +                                   VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
> +                                   VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,

I believe the flags should be 0 here. By using
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE you would effectively forbid calling
virDomainDefine for running transient domains. And
VIR_DOMAIN_OBJ_LIST_ADD_LIVE should not be set either since def is
domain's persistent (rather than active) definition.

> +                                   &oldDef)))
> +        goto cleanup;
> +
>      def = NULL;
>      if (virDomainHasDiskMirror(vm)) {
>          virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s",
...
> @@ -12612,9 +12593,6 @@ static virDomainPtr qemuDomainAttach(virConnectPtr conn,
>      if (!(caps = qemuCapsCacheLookup(driver->capsCache, def->emulator)))
>          goto cleanup;
>  
> -    if (virDomainObjListIsDuplicate(driver->domains, def, 1) < 0)
> -        goto cleanup;
> -
>      if (qemuCanonicalizeMachine(def, caps) < 0)
>          goto cleanup;
>  
> @@ -12623,7 +12601,10 @@ static virDomainPtr qemuDomainAttach(virConnectPtr conn,
>  
>      if (!(vm = virDomainObjListAdd(driver->domains,
>                                     driver->caps,
> -                                   def, false)))
> +                                   def,
> +                                   VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
> +                                   VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
> +                                   NULL)))

Although this could be correct, it doesn't match current code. To match
it, you'd need to use just VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE flag.

>          goto cleanup;
>  
>      def = NULL;
...
> diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c
> index b99fca3..8684f56 100644
> --- a/src/vmware/vmware_driver.c
> +++ b/src/vmware/vmware_driver.c
> @@ -320,9 +320,6 @@ vmwareDomainDefineXML(virConnectPtr conn, const char *xml)
>                                           VIR_DOMAIN_XML_INACTIVE)) == NULL)
>          goto cleanup;
>  
> -    if (virDomainObjListIsDuplicate(driver->domains, vmdef, 1) < 0)
> -        goto cleanup;
> -
>      /* generate vmx file */
>      vmx = virVMXFormatConfig(&ctx, driver->caps, vmdef, 7);
>      if (vmx == NULL)
> @@ -341,7 +338,7 @@ vmwareDomainDefineXML(virConnectPtr conn, const char *xml)
>      /* assign def */
>      if (!(vm = virDomainObjListAdd(driver->domains,
>                                     driver->caps,
> -                                   vmdef, false)))
> +                                   vmdef, 0, NULL)))

virDomainObjListIsDuplicate was called with 1 as the third argument,
thus you need to use VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE flag here.

>          goto cleanup;
>  
>      pDomain = vm->privateData;
...

Looking forward to v2 :-)

Jirka


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]