9:58 a.m.
On Fri, Feb 01, 2013 at 11:18:26 +0000, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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