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