Re: [libvirt] [PATCHv3 6/8] Implement virDomain{G, S}etNumaParameters for the qemu driver

On 12/15/2011 03:50 AM, Hu Tao wrote: This can be simplified by the code in commit ae523427. 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. Too much copy and paste. 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. Indentation. 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. Missing "%s". Again, OPERATION_INVALID fits better here. 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. Again, you don't need this. A comment helps here. Do this check earlier, to avoid time doing a wasted cgroup lookup. Drop this check. It needlessly limits the user. 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. Cap this loop to the max number of parameters we know about. Wrong. One param is string, and the other int (given my earlier tweaks). Check for allocation failure. 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). 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). I'd rather omit this until we know for sure what Coverity complains about. 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 the user passed *nparams as 1, then this causes the user to buffer overrun. 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 = &params[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 = &params[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 = &params[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 = &params[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(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org