On Fri, Feb 01, 2013 at 11:18:26 +0000, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list