This function sets the running domain definition as transient, by reparsing the persistent config and assigning it to newDef. This ensures that any changes made to the running definition and not the persistent config are discarded when the VM is shutdown. Signed-off-by: Cole Robinson <crobinso(a)redhat.com&gt; --- src/conf/domain_conf.c | 44 +++++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 2 ++ src/libvirt_private.syms | 1 + 3 files changed, 46 insertions(+), 1 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 30c27db..11a6280 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -352,6 +352,9 @@ VIR_ENUM_IMPL(virDomainTimerMode, VIR_DOMAIN_TIMER_MODE_LAST, virReportErrorHelper(NULL, VIR_FROM_DOMAIN, code, __FILE__, \ __FUNCTION__, __LINE__, __VA_ARGS__) +#define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE +#define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE + int virDomainObjListInit(virDomainObjListPtr doms) { doms->objs = virHashCreate(50); @@ -966,6 +969,45 @@ virDomainObjPtr virDomainAssignDef(virCapsPtr caps, } /* + * Mark the running VM config as transient. Ensures transient hotplug + * operations do not persist past shutdown. + * + * @param caps pointer to capabilities info + * @param domain domain object pointer + * @return 0 on success, -1 on failure + */ +int +virDomainObjSetDefTransient(virCapsPtr caps, + virDomainObjPtr domain) +{ + int ret = -1; + char *xml = NULL; + virDomainDefPtr newDef = NULL; + + if (!virDomainObjIsActive(domain)) + return 0; + + if (!domain->persistent) + return 0; + + if (domain->newDef) + return 0; + + if (!(xml = virDomainDefFormat(domain->def, VIR_DOMAIN_XML_WRITE_FLAGS))) + goto out; + + if (!(newDef = virDomainDefParseString(caps, xml, + VIR_DOMAIN_XML_READ_FLAGS))) + goto out; + + domain->newDef = newDef; + ret = 0; +out: + VIR_FREE(xml); + return ret; +} + +/* * The caller must hold a lock on the driver owning 'doms', * and must also have locked 'dom', to ensure no one else * is either waiting for 'dom' or still usingn it @@ -7256,7 +7298,7 @@ int virDomainSaveConfig(const char *configDir, char *xml; if (!(xml = virDomainDefFormat(def, - VIR_DOMAIN_XML_SECURE))) + VIR_DOMAIN_XML_WRITE_FLAGS))) goto cleanup; if (virDomainSaveXML(configDir, def, xml)) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 7d2d6f5..392e052 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1092,6 +1092,8 @@ virDomainObjPtr virDomainAssignDef(virCapsPtr caps, void virDomainObjAssignDef(virDomainObjPtr domain, const virDomainDefPtr def, bool live); +int virDomainObjSetDefTransient(virCapsPtr caps, + virDomainObjPtr domain); void virDomainRemoveInactive(virDomainObjListPtr doms, virDomainObjPtr dom); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 50eceba..0b62cf1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -224,6 +224,7 @@ virDomainMemballoonModelTypeToString; virDomainNetDefFree; virDomainNetTypeToString; virDomainObjAssignDef; +virDomainObjSetDefTransient; virDomainObjIsDuplicate; virDomainObjListDeinit; virDomainObjListGetActiveIDs; -- 1.7.3.2

The current semantics of non-persistent hotplug/update are confusing: the changes will persist as long as the in memory domain definition isn't overwritten. This means hotplug changes stay around until the domain is redefined or libvirtd is restarted. Call virDomainObjSetDefTransient at VM startup, so that we properly discard hotplug changes when the VM is shutdown. Signed-off-by: Cole Robinson <crobinso(a)redhat.com&gt; --- src/lxc/lxc_driver.c | 3 +++ src/qemu/qemu_driver.c | 5 +++++ src/test/test_driver.c | 42 ++++++++++++++++++++++++------------------ src/uml/uml_driver.c | 5 ++++- 4 files changed, 36 insertions(+), 19 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index f5014cb..f6ba137 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1532,6 +1532,9 @@ static int lxcVmStart(virConnectPtr conn, if (virDomainSaveConfig(driver->stateDir, vm->def) < 0) goto cleanup; + if (virDomainObjSetDefTransient(driver->caps, vm) < 0) + goto cleanup; + rc = 0; cleanup: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ed1ea6b..18b3f16 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4235,6 +4235,11 @@ static int qemudStartVMDaemon(virConnectPtr conn, if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) goto cleanup; + /* Do this last, since it depends on domain being active */ + DEBUG0("Setting running domain def as transient"); + if (virDomainObjSetDefTransient(driver->caps, vm) < 0) + goto cleanup; + VIR_FORCE_CLOSE(logfile); return 0; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 5693d7a..5e737e8 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -457,6 +457,22 @@ cleanup: return ret; } +static void +testDomainShutdownState(virDomainPtr domain, + virDomainObjPtr privdom) +{ + if (privdom->newDef) { + virDomainDefFree(privdom->def); + privdom->def = privdom->newDef; + privdom->newDef = NULL; + } + + privdom->state = VIR_DOMAIN_SHUTOFF; + privdom->def->id = -1; + if (domain) + domain->id = -1; +} + /* Set up domain runtime state */ static int testDomainStartState(virConnectPtr conn, @@ -468,30 +484,20 @@ testDomainStartState(virConnectPtr conn, if (testDomainUpdateVCPUs(conn, dom, dom->def->vcpus, 1) < 0) goto cleanup; - /* Set typical run state */ dom->state = VIR_DOMAIN_RUNNING; dom->def->id = privconn->nextDomID++; + if (virDomainObjSetDefTransient(privconn->caps, dom) < 0) { + goto cleanup; + } + ret = 0; cleanup: + if (ret < 0) + testDomainShutdownState(NULL, dom); return ret; } -static void -testDomainShutdownState(virDomainPtr domain, - virDomainObjPtr privdom) -{ - if (privdom->newDef) { - virDomainDefFree(privdom->def); - privdom->def = privdom->newDef; - privdom->newDef = NULL; - } - - privdom->state = VIR_DOMAIN_SHUTOFF; - privdom->def->id = -1; - domain->id = -1; -} - static int testOpenDefault(virConnectPtr conn) { int u; struct timeval tv; @@ -558,12 +564,12 @@ static int testOpenDefault(virConnectPtr conn) { goto error; domdef = NULL; + domobj->persistent = 1; if (testDomainStartState(conn, domobj) < 0) { virDomainObjUnlock(domobj); goto error; } - domobj->persistent = 1; virDomainObjUnlock(domobj); if (!(netdef = virNetworkDefParseString(defaultNetworkXML))) @@ -918,12 +924,12 @@ static int testOpenFromFile(virConnectPtr conn, goto error; } + dom->persistent = 1; if (testDomainStartState(conn, dom) < 0) { virDomainObjUnlock(dom); goto error; } - dom->persistent = 1; virDomainObjUnlock(dom); } VIR_FREE(domains); diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index ca4e7be..eaa3509 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -916,7 +916,11 @@ static int umlStartVMDaemon(virConnectPtr conn, VIR_EXEC_CLEAR_CAPS, NULL, NULL, NULL); VIR_FORCE_CLOSE(logfd); + if (ret < 0) + goto cleanup; + ret = virDomainObjSetDefTransient(driver->caps, vm); +cleanup: /* * At the moment, the only thing that populates keepfd is * umlBuildCommandLineChr. We want to close every fd it opens. @@ -940,7 +944,6 @@ static int umlStartVMDaemon(virConnectPtr conn, umlCleanupTapDevices(conn, vm); } - /* NB we don't mark it running here - we do that async with inotify */ /* XXX what if someone else tries to start it again -- 1.7.3.2

Signed-off-by: Cole Robinson <crobinso(a)redhat.com&gt; --- src/conf/domain_conf.c | 11 +++++++++-- 1 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 11a6280..045934d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4569,7 +4569,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, def->maxvcpus = 1; } else { def->maxvcpus = count; - if (def->maxvcpus != count || count == 0) { + if (count == 0) { virDomainReportError(VIR_ERR_XML_ERROR, _("invalid maxvcpus %lu"), count); goto error; @@ -4585,11 +4585,18 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, def->vcpus = def->maxvcpus; } else { def->vcpus = count; - if (def->vcpus != count || count == 0 || def->maxvcpus < count) { + if (count == 0) { virDomainReportError(VIR_ERR_XML_ERROR, _("invalid current vcpus %lu"), count); goto error; } + + if (def->maxvcpus < count) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("maxvcpus must not be less than current vcpus (%d < %lu)"), + def->maxvcpus, count); + goto error; + } } tmp = virXPathString("string(./vcpu[1]/@cpuset)", ctxt); -- 1.7.3.2

On 11/22/2010 02:35 PM, Cole Robinson wrote: At first glance, I was about to complain: Since def->maxvcpus is an unsigned short but count is an int, someone calling setvcpus 0x10001 will silently overflow and end up setting def->maxvcpus == 1. In other words, you just deleted the 'def->maxvcpus != count' overflow check... ...but this new code is an equally effective overflow check. No complaint after all; def is local, so it doesn't matter if we changed def->maxvcpus to an invalid value before detecting overflow. Thanks for cleaning this up for me. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

Doing 'virsh setvcpus $vm --config 10' doesn't check the value against the domains maxvcpus value. A larger value for example will prevent the guest from starting. Also make a similar change to the test driver. Signed-off-by: Cole Robinson <crobinso(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 3 +-- src/test/test_driver.c | 5 +++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 18b3f16..058a8f0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6322,8 +6322,7 @@ qemudDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, goto endjob; } - if ((flags & (VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_VCPU_LIVE)) == - VIR_DOMAIN_VCPU_LIVE && vm->def->maxvcpus < max) { + if (!(flags & VIR_DOMAIN_VCPU_MAXIMUM) && vm->def->maxvcpus < max) { max = vm->def->maxvcpus; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 5e737e8..d32568f 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2134,9 +2134,10 @@ testDomainSetVcpusFlags(virDomainPtr domain, unsigned int nrCpus, /* We allow more cpus in guest than host, but not more than the * domain's starting limit. */ - if ((flags & (VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_VCPU_LIVE)) == - VIR_DOMAIN_VCPU_LIVE && privdom->def->maxvcpus < maxvcpus) + if (!(flags & (VIR_DOMAIN_VCPU_MAXIMUM)) && + privdom->def->maxvcpus < maxvcpus) maxvcpus = privdom->def->maxvcpus; + if (nrCpus > maxvcpus) { testError(VIR_ERR_INVALID_ARG, "requested cpu amount exceeds maximum (%d > %d)", -- 1.7.3.2

Do this by adding a helper function to get the persistent domain config. This should be useful for other functions that may eventually want to alter the persistent domain config (attach/detach device). Also make similar changes to the test drivers setvcpus command. A caveat is that the function will return the running config for a transient domain, rather than error. This simplifies callers, as long as they use other methods to ensure the guest is persistent. Signed-off-by: Cole Robinson <crobinso(a)redhat.com&gt; --- src/conf/domain_conf.c | 21 +++++++++++++++++++++ src/conf/domain_conf.h | 3 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 44 +++++++++++++++++--------------------------- src/test/test_driver.c | 39 ++++++++++++--------------------------- 5 files changed, 54 insertions(+), 54 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 045934d..39a77c6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1008,6 +1008,27 @@ out: } /* + * Return the persistent domain configuration. If domain is transient, + * return the running config. + * + * @param caps pointer to capabilities info + * @param domain domain object pointer + * @return NULL on error, virDOmainDefPtr on success + */ +virDomainDefPtr +virDomainObjGetPersistentDef(virCapsPtr caps, + virDomainObjPtr domain) +{ + if (virDomainObjSetDefTransient(caps, domain) < 0) + return NULL; + + if (domain->newDef) + return domain->newDef; + else + return domain->def; +} + +/* * The caller must hold a lock on the driver owning 'doms', * and must also have locked 'dom', to ensure no one else * is either waiting for 'dom' or still usingn it diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 392e052..10cbded 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1094,6 +1094,9 @@ void virDomainObjAssignDef(virDomainObjPtr domain, bool live); int virDomainObjSetDefTransient(virCapsPtr caps, virDomainObjPtr domain); +virDomainDefPtr +virDomainObjGetPersistentDef(virCapsPtr caps, + virDomainObjPtr domain); void virDomainRemoveInactive(virDomainObjListPtr doms, virDomainObjPtr dom); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0b62cf1..5a97cb4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -225,6 +225,7 @@ virDomainNetDefFree; virDomainNetTypeToString; virDomainObjAssignDef; virDomainObjSetDefTransient; +virDomainObjGetPersistentDef; virDomainObjIsDuplicate; virDomainObjListDeinit; virDomainObjListGetActiveIDs; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 058a8f0..c0335c3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6264,7 +6264,7 @@ qemudDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, { struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm; - virDomainDefPtr def; + virDomainDefPtr persistentDef; const char * type; int max; int ret = -1; @@ -6309,6 +6309,12 @@ qemudDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, goto endjob; } + if (!vm->persistent && (flags & VIR_DOMAIN_VCPU_CONFIG)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot change persistent config of a transient domain")); + goto endjob; + } + if (!(type = virDomainVirtTypeToString(vm->def->virtType))) { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("unknown virt type in domain definition '%d'"), @@ -6333,36 +6339,19 @@ qemudDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, goto endjob; } + if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm))) + goto endjob; + switch (flags) { case VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_VCPU_CONFIG: - def = vm->def; - if (virDomainObjIsActive(vm)) { - if (vm->newDef) - def = vm->newDef; - else{ - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("no persistent state")); - goto endjob; - } - } - def->maxvcpus = nvcpus; - if (nvcpus < vm->newDef->vcpus) - def->vcpus = nvcpus; + persistentDef->maxvcpus = nvcpus; + if (nvcpus < persistentDef->vcpus) + persistentDef->vcpus = nvcpus; ret = 0; break; case VIR_DOMAIN_VCPU_CONFIG: - def = vm->def; - if (virDomainObjIsActive(vm)) { - if (vm->newDef) - def = vm->newDef; - else { - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("no persistent state")); - goto endjob; - } - } - def->vcpus = nvcpus; + persistentDef->vcpus = nvcpus; ret = 0; break; @@ -6372,8 +6361,9 @@ qemudDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, case VIR_DOMAIN_VCPU_LIVE | VIR_DOMAIN_VCPU_CONFIG: ret = qemudDomainHotplugVcpus(vm, nvcpus); - if (ret == 0 && vm->newDef) - vm->newDef->vcpus = nvcpus; + if (ret == 0) { + persistentDef->vcpus = nvcpus; + } break; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index d32568f..4f14606 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2095,7 +2095,7 @@ testDomainSetVcpusFlags(virDomainPtr domain, unsigned int nrCpus, { testConnPtr privconn = domain->conn->privateData; virDomainObjPtr privdom = NULL; - virDomainDefPtr def; + virDomainDefPtr persistentDef; int ret = -1, maxvcpus; virCheckFlags(VIR_DOMAIN_VCPU_LIVE | @@ -2145,36 +2145,20 @@ testDomainSetVcpusFlags(virDomainPtr domain, unsigned int nrCpus, goto cleanup; } + if (!(persistentDef = virDomainObjGetPersistentDef(privconn->caps, + privdom))) + goto cleanup; + switch (flags) { case VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_VCPU_CONFIG: - def = privdom->def; - if (virDomainObjIsActive(privdom)) { - if (privdom->newDef) - def = privdom->newDef; - else { - testError(VIR_ERR_OPERATION_INVALID, "%s", - _("no persistent state")); - goto cleanup; - } - } - def->maxvcpus = nrCpus; - if (nrCpus < def->vcpus) - def->vcpus = nrCpus; + persistentDef->maxvcpus = nrCpus; + if (nrCpus < persistentDef->vcpus) + persistentDef->vcpus = nrCpus; ret = 0; break; case VIR_DOMAIN_VCPU_CONFIG: - def = privdom->def; - if (virDomainObjIsActive(privdom)) { - if (privdom->newDef) - def = privdom->newDef; - else { - testError(VIR_ERR_OPERATION_INVALID, "%s", - _("no persistent state")); - goto cleanup; - } - } - def->vcpus = nrCpus; + persistentDef->vcpus = nrCpus; ret = 0; break; @@ -2184,8 +2168,9 @@ testDomainSetVcpusFlags(virDomainPtr domain, unsigned int nrCpus, case VIR_DOMAIN_VCPU_LIVE | VIR_DOMAIN_VCPU_CONFIG: ret = testDomainUpdateVCPUs(domain->conn, privdom, nrCpus, 0); - if (ret == 0 && privdom->newDef) - privdom->newDef->vcpus = nrCpus; + if (ret == 0) { + persistentDef->vcpus = nrCpus; + } break; } -- 1.7.3.2

Currently changes to the persistent config aren't flushed to disk, meaning they are lost if the domain is redefined or libvirtd is restarted. Signed-off-by: Cole Robinson <crobinso(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c0335c3..04cca43 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6367,6 +6367,10 @@ qemudDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, break; } + /* Save the persistent config to disk */ + if (flags & VIR_DOMAIN_VCPU_CONFIG) + ret = virDomainSaveConfig(driver->configDir, persistentDef); + endjob: if (qemuDomainObjEndJob(vm) == 0) vm = NULL; -- 1.7.3.2

On 11/23/2010 06:31 AM, Daniel P. Berrange wrote: Thanks, I've pushed this series. - Cole