On 12/15/2011 03:50 AM, Hu Tao wrote: > --- > src/qemu/qemu_driver.c | 334 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 334 insertions(+), 0 deletions(-) > > + > + isActive = virDomainObjIsActive(vm); > + > + if (flags == VIR_DOMAIN_AFFECT_CURRENT) { > + if (isActive) > + flags = VIR_DOMAIN_AFFECT_LIVE; > + else > + flags = VIR_DOMAIN_AFFECT_CONFIG; > + } This can be simplified by the code in commit ae523427. > + > + if (flags & VIR_DOMAIN_AFFECT_LIVE) { > + if (vm->def->numatune.memory.mode != VIR_DOMAIN_NUMATUNE_MEM_STRICT) { > + qemuReportError(VIR_ERR_INVALID_ARG, "%s", > + _("change of nodeset for running domain is supported only when numa mode is strict")); > + ret = -1; I think OPERATION_INVALID fits better here than INVALID_ARG, since once the domain goes offline, the same argument will then start working. Also, the message was long. > + continue; > + } > + rc = virCgroupSetCpusetMems(group, params[i].value.s); > + if (rc != 0) { > + virReportSystemError(-rc, "%s", > + _("unable to set memory hard_limit tunable")); Too much copy and paste. > + ret = -1; > + continue; > + } > + } > + > + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { > + char *oldnodemask = strdup(persistentDef->numatune.memory.nodemask); strdup is wrong; the CPU mask can contain NUL bytes in the middle. Also, you have a memory leak. Using stack allocation and memcpy solves both issues. > + if (!oldnodemask) { > + virReportOOMError(); > + ret = -1; > + continue; > + } > + if (virDomainCpuSetParse(params[i].value.s, > + 0, Indentation. > + } else if (STREQ(param->field, VIR_DOMAIN_NUMA_MODE)) { > + if (param->type != VIR_TYPED_PARAM_ULLONG) { > + qemuReportError(VIR_ERR_INVALID_ARG, "%s", > + _("invalid type for numa strict tunable, expected a 'ullong'")); Why a ullong? That's awfully wide, when domain_conf only stores numatune.memory.mode as an int, and that in turn only maps to an enum valued between 0 and 2 inclusive (see my tweaks to 3/8). Not to mention you used value.i later on, instead of value.ul. > + ret = -1; > + continue; > + } > + > + if (flags & VIR_DOMAIN_AFFECT_LIVE) { > + qemuReportError(VIR_ERR_INVALID_ARG, _("can't change numa mode for running domain")); Missing "%s". Again, OPERATION_INVALID fits better here. > + > +static int qemuDomainGetNumaParameters(virDomainPtr dom, > + virTypedParameterPtr params, > + int *nparams, > + unsigned int flags) Style-wise, we aren't yet consistent in libvirt (let alone in the qemu driver), but we are starting to converge on a style where new code lists the return type on one line and function name on column 1 of the next. > +{ > + struct qemud_driver *driver = dom->conn->privateData; > + int i; > + virCgroupPtr group = NULL; > + virDomainObjPtr vm = NULL; > + virDomainDefPtr persistentDef = NULL; > + char *nodeset = NULL; > + int ret = -1; > + int rc; > + bool isActive; Again, you don't need this. > + > + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | > + VIR_DOMAIN_AFFECT_CONFIG | > + VIR_TYPED_PARAM_STRING_OKAY, -1); > + > + qemuDriverLock(driver); > + > + flags &= ~VIR_TYPED_PARAM_STRING_OKAY; A comment helps here. > + > + if (virCgroupForDomain(driver->cgroup, vm->def->name, &group, 0) != 0) { > + qemuReportError(VIR_ERR_INTERNAL_ERROR, > + _("cannot find cgroup for domain %s"), vm->def->name); > + goto cleanup; > + } > + } > + > + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { > + if (!vm->persistent) { > + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", > + _("cannot change persistent config of a transient domain")); > + goto cleanup; > + } > + if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm))) > + goto cleanup; > + } > + > + if ((*nparams) == 0) { > + *nparams = QEMU_NB_NUMA_PARAM; > + ret = 0; > + goto cleanup; > + } Do this check earlier, to avoid time doing a wasted cgroup lookup. > + > + if ((*nparams) < QEMU_NB_NUMA_PARAM) { > + qemuReportError(VIR_ERR_INVALID_ARG, > + "%s", _("Invalid parameter count")); > + goto cleanup; > + } Drop this check. It needlessly limits the user. > + > + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { Rather than making this the outer condition, and repeating the loop twice, I find it simpler to make it the inner condition only in the places where it matters within a single loop. > + for (i = 0; i < *nparams; i++) { Cap this loop to the max number of parameters we know about. > + virMemoryParameterPtr param = ¶ms[i]; > + param->value.ul = 0; > + param->type = VIR_TYPED_PARAM_ULLONG; Wrong. One param is string, and the other int (given my earlier tweaks). > + > + switch (i) { > + case 0: /* fill numa nodeset here */ > + if (virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_NODESET) == NULL) { > + qemuReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", _("Field numa nodeset too long for destination")); > + goto cleanup; > + } > + if (persistentDef->numatune.memory.nodemask) { > + nodeset = virDomainCpuSetFormat(persistentDef->numatune.memory.nodemask, > + VIR_DOMAIN_CPUMASK_LEN); > + if (!nodeset) { > + qemuReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", _("failed to format nodeset for NUMA memory tuning")); > + goto cleanup; > + } > + param->value.s = nodeset; > + nodeset = NULL; > + } else { > + param->value.s = strdup(""); Check for allocation failure. > + } > + param->type = VIR_TYPED_PARAM_STRING; > + break; > + > + case 1: /* fill numa mode here */ I find it confusing listing nodeset before mode here, when the XML lists it in the reverse direction. For consistency, I also swapped the order in the setter routine (but there you still have the issue that the user can pass in parameters out of order, and result in changing the numaset prior to incorrectly requesting a mode change, and we don't roll back the numaset change in that case - oh well). > + if (virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_MODE) == NULL) { > + qemuReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", _("Field numa mode too long for destination")); > + goto cleanup; > + } > + param->value.i = persistentDef->numatune.memory.mode; > + break; > + > + default: > + break; > + /* should not hit here */ > + } > + } > + goto out; > + } > + > + for (i = 0; i < QEMU_NB_NUMA_PARAM; i++) { Wrong - you just did a buffer overflow if the user passed *nparams = 1 (well once you take in my earlier comment that requires a minimum *nparams). > + virTypedParameterPtr param = ¶ms[i]; > + param->value.ul = 0; > + param->type = VIR_TYPED_PARAM_ULLONG; > + > + /* Coverity does not realize that if we get here, group is set. */ > + sa_assert(group); I'd rather omit this until we know for sure what Coverity complains about. > + > + switch (i) { > + case 0: /* fill numa nodeset here */ > + rc = virCgroupGetCpusetMems(group, &nodeset); Hmm - here, we are reading cgroups in preference to trusting what is in domain_conf. But earlier, in the set routine, we didn't update vm->def (just cgroups). That means if we alter the nodeset at runtim, then run 'virsh dumpxml', the XML will be wrong, even though the virsh query of the numatune parameters will be correct. The fix is either to make setting _also_ update vm->def in addition to cgroups (but if we do that, then why query cgroups - just read off vm->def); or to make sure that virsh dumpxml _always_ reads cgroups rather than trusting vm->def. I'm not sure which approach is better, so for now, I added a FIXME and am hoping we get it resolved soon (I think you're not the first person to fall prey to this design bug; my recollection is that the per-device blkiotune parameters added in 0.9.8 have the same bug, so cleaning all instances up in a separate patch later makes sense anyways).; > + if (rc != 0) { > + virReportSystemError(-rc, "%s", > + _("unable to get numa nodeset")); > + goto cleanup; > + } > +out: > + *nparams = QEMU_NB_NUMA_PARAM; > + ret = 0; If the user passed *nparams as 1, then this causes the user to buffer overrun. > + > +cleanup: > + if (group) > + virCgroupFree(&group); Useless if before free. Wow - I had to really touch this up. src/qemu/qemu_driver.c | 282 ++++++++++++++++++------------------------------ 1 files changed, 105 insertions(+), 177 deletions(-) diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 24f3047..12235ef 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -6606,10 +6606,11 @@ cleanup: return ret; } -static int qemuDomainSetNumaParameters(virDomainPtr dom, - virTypedParameterPtr params, - int nparams, - unsigned int flags) +static int +qemuDomainSetNumaParameters(virDomainPtr dom, + virTypedParameterPtr params, + int nparams, + unsigned int flags) { struct qemud_driver *driver = dom->conn->privateData; int i; @@ -6617,7 +6618,6 @@ static int qemuDomainSetNumaParameters(virDomainPtr dom, virCgroupPtr group = NULL; virDomainObjPtr vm = NULL; int ret = -1; - bool isActive; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -6632,22 +6632,11 @@ static int qemuDomainSetNumaParameters(virDomainPtr dom, goto cleanup; } - isActive = virDomainObjIsActive(vm); - - if (flags == VIR_DOMAIN_AFFECT_CURRENT) { - if (isActive) - flags = VIR_DOMAIN_AFFECT_LIVE; - else - flags = VIR_DOMAIN_AFFECT_CONFIG; - } + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, + &persistentDef) < 0) + goto cleanup; if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (!isActive) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); - goto cleanup; - } - if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPUSET)) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cgroup cpuset controller is not mounted")); @@ -6656,84 +6645,80 @@ static int qemuDomainSetNumaParameters(virDomainPtr dom, if (virCgroupForDomain(driver->cgroup, vm->def->name, &group, 0) != 0) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot find cgroup for domain %s"), vm->def->name); - goto cleanup; - } - } - - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (!vm->persistent) { - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot change persistent config of a transient domain")); + _("cannot find cgroup for domain %s"), + vm->def->name); goto cleanup; } - if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm))) - goto cleanup; } ret = 0; for (i = 0; i < nparams; i++) { virTypedParameterPtr param = ¶ms[i]; - if (STREQ(param->field, VIR_DOMAIN_NUMA_NODESET)) { + if (STREQ(param->field, VIR_DOMAIN_NUMA_MODE)) { + if (param->type != VIR_TYPED_PARAM_INT) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("invalid type for numa strict tunable, " + "expected an 'int'")); + ret = -1; + continue; + } + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("can't change numa mode for running domain")); + ret = -1; + goto cleanup; + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + persistentDef->numatune.memory.mode = params[i].value.i; + } + } else if (STREQ(param->field, VIR_DOMAIN_NUMA_NODESET)) { int rc; if (param->type != VIR_TYPED_PARAM_STRING) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", - _("invalid type for numa nodeset tunable, expected a 'string'")); + _("invalid type for numa nodeset tunable, " + "expected a 'string'")); ret = -1; continue; } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (vm->def->numatune.memory.mode != VIR_DOMAIN_NUMATUNE_MEM_STRICT) { - qemuReportError(VIR_ERR_INVALID_ARG, "%s", - _("change of nodeset for running domain is supported only when numa mode is strict")); + if (vm->def->numatune.memory.mode != + VIR_DOMAIN_NUMATUNE_MEM_STRICT) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("change of nodeset for running domain " + "requires strict numa mode")); ret = -1; continue; } rc = virCgroupSetCpusetMems(group, params[i].value.s); if (rc != 0) { virReportSystemError(-rc, "%s", - _("unable to set memory hard_limit tunable")); + _("unable to set numa tunable")); ret = -1; continue; } + /* XXX FIXME - should we also be updating vm->def + * here? If not, then we need to fix dumpxml to always + * read from cgroups rather than trusting vm->def. */ } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - char *oldnodemask = strdup(persistentDef->numatune.memory.nodemask); - if (!oldnodemask) { - virReportOOMError(); - ret = -1; - continue; - } + char oldnodemask[VIR_CPU_MAPLEN(VIR_DOMAIN_CPUMASK_LEN)]; + memcpy(oldnodemask, persistentDef->numatune.memory.nodemask, + VIR_DOMAIN_CPUMASK_LEN); if (virDomainCpuSetParse(params[i].value.s, - 0, - persistentDef->numatune.memory.nodemask, - VIR_DOMAIN_CPUMASK_LEN) < 0) { - VIR_FREE(persistentDef->numatune.memory.nodemask); - persistentDef->numatune.memory.nodemask = oldnodemask; + 0, + persistentDef->numatune.memory.nodemask, + VIR_DOMAIN_CPUMASK_LEN) < 0) { + memcpy(persistentDef->numatune.memory.nodemask, + oldnodemask, VIR_DOMAIN_CPUMASK_LEN); ret = -1; continue; } } - } else if (STREQ(param->field, VIR_DOMAIN_NUMA_MODE)) { - if (param->type != VIR_TYPED_PARAM_ULLONG) { - qemuReportError(VIR_ERR_INVALID_ARG, "%s", - _("invalid type for numa strict tunable, expected a 'ullong'")); - ret = -1; - continue; - } - - if (flags & VIR_DOMAIN_AFFECT_LIVE) { - qemuReportError(VIR_ERR_INVALID_ARG, _("can't change numa mode for running domain")); - ret = -1; - goto cleanup; - } - - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - persistentDef->numatune.memory.mode = params[i].value.i; - } } else { qemuReportError(VIR_ERR_INVALID_ARG, _("Parameter `%s' not supported"), param->field); @@ -6754,10 +6739,11 @@ cleanup: return ret; } -static int qemuDomainGetNumaParameters(virDomainPtr dom, - virTypedParameterPtr params, - int *nparams, - unsigned int flags) +static int +qemuDomainGetNumaParameters(virDomainPtr dom, + virTypedParameterPtr params, + int *nparams, + unsigned int flags) { struct qemud_driver *driver = dom->conn->privateData; int i; @@ -6767,7 +6753,6 @@ static int qemuDomainGetNumaParameters(virDomainPtr dom, char *nodeset = NULL; int ret = -1; int rc; - bool isActive; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | @@ -6775,6 +6760,9 @@ static int qemuDomainGetNumaParameters(virDomainPtr dom, qemuDriverLock(driver); + /* We blindly return a string, and let libvirt.c and + * remote_driver.c do the filtering on behalf of older clients + * that can't parse it. */ flags &= ~VIR_TYPED_PARAM_STRING_OKAY; vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -6785,22 +6773,17 @@ static int qemuDomainGetNumaParameters(virDomainPtr dom, goto cleanup; } - isActive = virDomainObjIsActive(vm); + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, + &persistentDef) < 0) + goto cleanup; - if (flags == VIR_DOMAIN_AFFECT_CURRENT) { - if (isActive) - flags = VIR_DOMAIN_AFFECT_LIVE; - else - flags = VIR_DOMAIN_AFFECT_CONFIG; + if ((*nparams) == 0) { + *nparams = QEMU_NB_NUMA_PARAM; + ret = 0; + goto cleanup; } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (!isActive) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); - goto cleanup; - } - if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_MEMORY)) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cgroup memory controller is not mounted")); @@ -6809,112 +6792,58 @@ static int qemuDomainGetNumaParameters(virDomainPtr dom, if (virCgroupForDomain(driver->cgroup, vm->def->name, &group, 0) != 0) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot find cgroup for domain %s"), vm->def->name); - goto cleanup; - } - } - - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (!vm->persistent) { - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot change persistent config of a transient domain")); + _("cannot find cgroup for domain %s"), + vm->def->name); goto cleanup; } - if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm))) - goto cleanup; } - if ((*nparams) == 0) { - *nparams = QEMU_NB_NUMA_PARAM; - ret = 0; - goto cleanup; - } - - if ((*nparams) < QEMU_NB_NUMA_PARAM) { - qemuReportError(VIR_ERR_INVALID_ARG, - "%s", _("Invalid parameter count")); - goto cleanup; - } - - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - for (i = 0; i < *nparams; i++) { - virMemoryParameterPtr param = ¶ms[i]; - param->value.ul = 0; - param->type = VIR_TYPED_PARAM_ULLONG; - - switch (i) { - case 0: /* fill numa nodeset here */ - if (virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_NODESET) == NULL) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field numa nodeset too long for destination")); - goto cleanup; - } - if (persistentDef->numatune.memory.nodemask) { - nodeset = virDomainCpuSetFormat(persistentDef->numatune.memory.nodemask, - VIR_DOMAIN_CPUMASK_LEN); - if (!nodeset) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("failed to format nodeset for NUMA memory tuning")); - goto cleanup; - } - param->value.s = nodeset; - nodeset = NULL; - } else { - param->value.s = strdup(""); - } - param->type = VIR_TYPED_PARAM_STRING; - break; - - case 1: /* fill numa mode here */ - if (virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_MODE) == NULL) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field numa mode too long for destination")); - goto cleanup; - } - param->value.i = persistentDef->numatune.memory.mode; - break; - - default: - break; - /* should not hit here */ - } - } - goto out; - } - - for (i = 0; i < QEMU_NB_NUMA_PARAM; i++) { - virTypedParameterPtr param = ¶ms[i]; - param->value.ul = 0; - param->type = VIR_TYPED_PARAM_ULLONG; - - /* Coverity does not realize that if we get here, group is set. */ - sa_assert(group); + for (i = 0; i < QEMU_NB_NUMA_PARAM && i < *nparams; i++) { + virMemoryParameterPtr param = ¶ms[i]; switch (i) { - case 0: /* fill numa nodeset here */ - rc = virCgroupGetCpusetMems(group, &nodeset); - if (rc != 0) { - virReportSystemError(-rc, "%s", - _("unable to get numa nodeset")); - goto cleanup; - } - if (virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_NODESET) == NULL) { + case 0: /* fill numa mode here */ + if (!virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_MODE)) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field numa nodeset too long for destination")); - VIR_FREE(nodeset); + _("Field '%s' too long for destination"), + VIR_DOMAIN_NUMA_MODE); goto cleanup; } - param->value.s = nodeset; - param->type = VIR_TYPED_PARAM_STRING; + param->type = VIR_TYPED_PARAM_INT; + if (flags & VIR_DOMAIN_AFFECT_CONFIG) + param->value.i = persistentDef->numatune.memory.mode; + else + param->value.i = vm->def->numatune.memory.mode; break; - case 1: /* file numa mode here */ - if (virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_MODE) == NULL) { + case 1: /* fill numa nodeset here */ + if (!virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_NODESET)) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field numa exclusive too long for destination")); + _("Field '%s' too long for destination"), + VIR_DOMAIN_NUMA_NODESET); + goto cleanup; + } + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + char *mask = persistentDef->numatune.memory.nodemask; + if (mask) + nodeset = virDomainCpuSetFormat(mask, + VIR_DOMAIN_CPUMASK_LEN); + else + nodeset = strdup(""); + } else { + rc = virCgroupGetCpusetMems(group, &nodeset); + if (rc != 0) { + virReportSystemError(-rc, "%s", + _("unable to get numa nodeset")); + goto cleanup; + } + } + if (!nodeset) { + virReportOOMError(); goto cleanup; } - param->value.ul = vm->def->numatune.memory.mode; + param->type = VIR_TYPED_PARAM_STRING; + param->value.s = nodeset; break; default: @@ -6923,13 +6852,12 @@ static int qemuDomainGetNumaParameters(virDomainPtr dom, } } -out: - *nparams = QEMU_NB_NUMA_PARAM; + if (*nparams > QEMU_NB_NUMA_PARAM) + *nparams = QEMU_NB_NUMA_PARAM; ret = 0; cleanup: - if (group) - virCgroupFree(&group); + virCgroupFree(&group); if (vm) virDomainObjUnlock(vm); qemuDriverUnlock(driver); -- Eric Blake eblake@xxxxxxxxxx +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list