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

On Mon, Feb 04, 2013 at 04:58:25PM +0100, Jiri Denemark wrote: Yep, I believe this new code is correct 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 :|