On Mon, Feb 04, 2013 at 04:58:25PM +0100, Jiri Denemark wrote:
> 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:
> > @@ -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.
Yep, I believe this new code is correct
>
> Looking forward to v2 :-)
Here is the diff from v1 to v2
Daniel
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1771668..06dbe1f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1853,21 +1853,33 @@ error:
void virDomainObjAssignDef(virDomainObjPtr domain,
const virDomainDefPtr def,
- bool live)
+ bool live,
+ virDomainDefPtr *oldDef)
{
- if (!virDomainObjIsActive(domain)) {
+ *oldDef = NULL;
+ if (virDomainObjIsActive(domain)) {
+ if (oldDef)
+ *oldDef = domain->newDef;
+ else
+ virDomainDefFree(domain->newDef);
+ domain->newDef = def;
+ } else {
if (live) {
- /* save current configuration to be restored on domain shutdown */
- if (!domain->newDef)
- domain->newDef = domain->def;
+ if (domain->def) {
+ /* save current configuration to be restored on domain shutdown */
+ if (!domain->newDef)
+ domain->newDef = domain->def;
+ else
+ virDomainDefFree(domain->def);
+ }
domain->def = def;
} else {
- virDomainDefFree(domain->def);
+ if (oldDef)
+ *oldDef = domain->def;
+ else
+ virDomainDefFree(domain->def);
domain->def = def;
}
- } else {
- virDomainDefFree(domain->newDef);
- domain->newDef = def;
}
}
@@ -1879,6 +1891,10 @@ void virDomainObjAssignDef(virDomainObjPtr domain,
* this will refuse updating an existing def if the
* current def is Live
*
+ * If flags & VIR_DOMAIN_OBJ_LIST_ADD_LIVE then
+ * the @def being added is assumed to represent a
+ * live config, not a future inactive config
+ *
*/
virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
virCapsPtr caps,
@@ -1899,7 +1915,7 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
virReportError(VIR_ERR_OPERATION_FAILED,
_("domain '%s' is already defined with uuid %s"),
vm->def->name, uuidstr);
- goto cleanup;
+ goto error;
}
if (flags & VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE) {
@@ -1908,30 +1924,14 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
virReportError(VIR_ERR_OPERATION_INVALID,
_("domain is already active as '%s'"),
vm->def->name);
- goto cleanup;
+ goto error;
}
}
- 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);
- } else {
- if (oldDef)
- *oldDef = vm->def;
- else
- virDomainDefFree(vm->def);
- }
- vm->def = def;
- }
+ virDomainObjAssignDef(vm,
+ def,
+ !!(flags & VIR_DOMAIN_OBJ_LIST_ADD_LIVE),
+ oldDef);
} else {
/* UUID does not match, but if a name matches, refuse it */
if ((vm = virDomainObjListFindByName(doms, def->name))) {
@@ -1939,9 +1939,7 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
virReportError(VIR_ERR_OPERATION_FAILED,
_("domain '%s' already exists with uuid %s"),
def->name, uuidstr);
- virObjectUnlock(vm);
- vm = NULL;
- goto cleanup;
+ goto error;
}
if (!(vm = virDomainObjNew(caps)))
@@ -1950,12 +1948,17 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
virUUIDFormat(def->uuid, uuidstr);
if (virHashAddEntry(doms->objs, uuidstr, vm) < 0) {
- VIR_FREE(vm);
+ virObjectUnref(vm);
return NULL;
}
}
cleanup:
return vm;
+
+error:
+ virObjectUnlock(vm);
+ vm = NULL;
+ goto cleanup;
}
/*
@@ -14879,7 +14882,7 @@ virDomainObjListLoadConfig(virDomainObjListPtr doms,
virDomainDefPtr def = NULL;
virDomainObjPtr dom;
int autostart;
- int newVM = 1;
+ virDomainDefPtr oldDef = NULL;
if ((configFile = virDomainConfigFile(configDir, name)) == NULL)
goto error;
@@ -14893,32 +14896,15 @@ virDomainObjListLoadConfig(virDomainObjListPtr doms,
if ((autostart = virFileLinkPointsTo(autostartLink, configFile)) < 0)
goto error;
- /* if the domain is already in our hashtable, we only need to
- * update the autostart flag
- */
- if ((dom = virDomainObjListFindByUUID(doms, def->uuid))) {
- dom->autostart = autostart;
-
- if (virDomainObjIsActive(dom) &&
- !dom->newDef) {
- virDomainObjAssignDef(dom, def, false);
- } else {
- virDomainDefFree(def);
- }
-
- VIR_FREE(configFile);
- VIR_FREE(autostartLink);
- return dom;
- }
-
- if (!(dom = virDomainObjListAdd(doms, caps, def, 0, NULL)))
+ if (!(dom = virDomainObjListAdd(doms, caps, def, 0, &oldDef)))
goto error;
dom->autostart = autostart;
if (notify)
- (*notify)(dom, newVM, opaque);
+ (*notify)(dom, oldDef == NULL, opaque);
+ virDomainDefFree(oldDef);
VIR_FREE(configFile);
VIR_FREE(autostartLink);
return dom;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index fa13e24..dc411e4 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1966,8 +1966,6 @@ void virDomainDefFree(virDomainDefPtr vm);
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 */
enum {
VIR_DOMAIN_OBJ_LIST_ADD_LIVE = (1 << 0),
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE = (1 << 1),
@@ -1979,7 +1977,8 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
virDomainDefPtr *oldDef);
void virDomainObjAssignDef(virDomainObjPtr domain,
const virDomainDefPtr def,
- bool live);
+ bool live,
+ virDomainDefPtr *oldDef);
int virDomainObjSetDefTransient(virCapsPtr caps,
virDomainObjPtr domain,
bool live);
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index e0a9a8e..d11acfc 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -902,7 +902,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
goto error;
}
- virDomainObjAssignDef(vm, def, true);
+ virDomainObjAssignDef(vm, def, true, NULL);
def = NULL;
if (unlink(managed_save_path) < 0) {
@@ -3602,7 +3602,7 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
if (!ret && (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) {
ret = virDomainSaveConfig(driver->configDir, vmdef);
if (!ret) {
- virDomainObjAssignDef(vm, vmdef, false);
+ virDomainObjAssignDef(vm, vmdef, false, NULL);
vmdef = NULL;
}
}
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 5e664c7..9f636e0 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1863,7 +1863,7 @@ lxcSetSchedulerParametersFlags(virDomainPtr dom,
if (rc < 0)
goto cleanup;
- virDomainObjAssignDef(vm, vmdef, false);
+ virDomainObjAssignDef(vm, vmdef, false, NULL);
vmdef = NULL;
}
@@ -4411,7 +4411,7 @@ lxcDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
ret = virDomainSaveConfig(driver->configDir, vmdef);
if (!ret) {
- virDomainObjAssignDef(vm, vmdef, false);
+ virDomainObjAssignDef(vm, vmdef, false, NULL);
vmdef = NULL;
}
}
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
index e9724fd..3081417 100644
--- a/src/openvz/openvz_conf.c
+++ b/src/openvz/openvz_conf.c
@@ -586,6 +586,7 @@ int openvzLoadDomains(struct openvz_driver *driver) {
line = outbuf;
while (line[0] != '\0') {
+ unsigned int flags = 0;
if (virStrToLong_i(line, &status, 10, &veid) < 0 ||
*status++ != ' ' ||
(line = strchr(status, '\n')) == NULL) {
@@ -642,12 +643,14 @@ int openvzLoadDomains(struct openvz_driver *driver) {
openvzReadMemConf(def, veid);
virUUIDFormat(def->uuid, uuidstr);
+ flags = VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE;
+ if (STRNEQ(status, "stopped"))
+ flags |= VIR_DOMAIN_OBJ_LIST_ADD_LIVE;
+
if (!(dom = virDomainObjListAdd(driver->domains,
driver->caps,
def,
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE |
- (STRNEQ(status, "stopped") ?
- VIR_DOMAIN_OBJ_LIST_ADD_LIVE : 0),
+ flags,
NULL)))
goto cleanup;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d6c6af5..b0683c6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5168,7 +5168,7 @@ qemuDomainObjRestore(virConnectPtr conn,
goto cleanup;
}
- virDomainObjAssignDef(vm, def, true);
+ virDomainObjAssignDef(vm, def, true, NULL);
def = NULL;
ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, &header, path,
@@ -5611,8 +5611,7 @@ static virDomainPtr qemuDomainDefine(virConnectPtr conn, const char *xml) {
if (!(vm = virDomainObjListAdd(driver->domains,
driver->caps,
def,
- VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
+ 0,
&oldDef)))
goto cleanup;
@@ -5620,7 +5619,7 @@ static virDomainPtr qemuDomainDefine(virConnectPtr conn, const char *xml) {
if (virDomainHasDiskMirror(vm)) {
virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s",
_("domain has active block copy job"));
- virDomainObjAssignDef(vm, NULL, false);
+ virDomainObjAssignDef(vm, NULL, false, NULL);
goto cleanup;
}
vm->persistent = 1;
@@ -6532,7 +6531,7 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
ret = virDomainSaveConfig(cfg->configDir, vmdef);
if (!ret) {
- virDomainObjAssignDef(vm, vmdef, false);
+ virDomainObjAssignDef(vm, vmdef, false, NULL);
vmdef = NULL;
}
}
@@ -8045,7 +8044,7 @@ qemuSetSchedulerParametersFlags(virDomainPtr dom,
if (rc < 0)
goto cleanup;
- virDomainObjAssignDef(vm, vmdef, false);
+ virDomainObjAssignDef(vm, vmdef, false, NULL);
vmdef = NULL;
}
@@ -12181,13 +12180,13 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
goto endjob;
}
if (config)
- virDomainObjAssignDef(vm, config, false);
+ virDomainObjAssignDef(vm, config, false, NULL);
} else {
/* Transitions 2, 3 */
load:
was_stopped = true;
if (config)
- virDomainObjAssignDef(vm, config, false);
+ virDomainObjAssignDef(vm, config, false, NULL);
rc = qemuProcessStart(snapshot->domain->conn,
driver, vm, NULL, -1, NULL, snap,
@@ -12271,7 +12270,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
goto endjob;
}
if (config)
- virDomainObjAssignDef(vm, config, false);
+ virDomainObjAssignDef(vm, config, false, NULL);
if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) {
diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c
index 8684f56..b7905fa 100644
--- a/src/vmware/vmware_driver.c
+++ b/src/vmware/vmware_driver.c
@@ -338,7 +338,9 @@ vmwareDomainDefineXML(virConnectPtr conn, const char *xml)
/* assign def */
if (!(vm = virDomainObjListAdd(driver->domains,
driver->caps,
- vmdef, 0, NULL)))
+ vmdef,
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
+ NULL)))
goto cleanup;
pDomain = vm->privateData;
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list