Re: [libvirt] [PATCHv3 7/8] add new command numatune to virsh

On 12/15/2011 03:50 AM, Hu Tao wrote: Hmm, this is the first case of including a file from src/conf/ from within tools/, just to translate between strings and numa modes. If the functionality needs to be common, should it be moved to live in util (cf. virKeycodeValueFromString)? Daniel (danpb or DV), is that a problem that we need to worry about? This code needs tweaking, since I fixed this to be an INT in value.i, not a ULLONG. Also, this whole case statement is redundant; we already have a helper method for formatting typed parameters. I'll submit a separate patch for starting the cleanup. Wrong type. This needs to be done per parameter. We should prefer mode before nodeset, to match the XML, and since it gives saner cleanup behavior if mode is incompatible with a runtime change in nodeset. Why strncpy here, but virStrcpy on the other field? Memory leak. params can contain strdup'd text. Sorting. Here's what I'm squashing so far, although I'd like an answer on whether we can keep the conf/domain_conf.h include before I can push anything: diff --git i/tools/virsh.c w/tools/virsh.c index ad570d3..e148f95 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -5143,10 +5143,10 @@ static const vshCmdInfo info_numatune[] = { static const vshCmdOptDef opts_numatune[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, - {"nodeset", VSH_OT_DATA, VSH_OFLAG_NONE, - N_("NUMA node to set")}, {"mode", VSH_OT_DATA, VSH_OFLAG_NONE, N_("NUMA mode, one of strict, preferred and interleave")}, + {"nodeset", VSH_OT_DATA, VSH_OFLAG_NONE, + N_("NUMA node selections to set")}, {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, @@ -5224,41 +5224,14 @@ cmdNumatune(vshControl * ctl, const vshCmd * cmd) } for (i = 0; i < nparams; i++) { - switch (params[i].type) { - case VIR_TYPED_PARAM_INT: - vshPrint(ctl, "%-15s: %d\n", params[i].field, - params[i].value.i); - break; - case VIR_TYPED_PARAM_UINT: - vshPrint(ctl, "%-15s: %u\n", params[i].field, - params[i].value.ui); - break; - case VIR_TYPED_PARAM_LLONG: - vshPrint(ctl, "%-15s: %lld\n", params[i].field, - params[i].value.l); - break; - case VIR_TYPED_PARAM_ULLONG: - if (STREQ(params[i].field, VIR_DOMAIN_NUMA_MODE)) - vshPrint(ctl, "%-15s: %s\n", params[i].field, - virDomainNumatuneMemModeTypeToString(params[i].value.ul)); - else - vshPrint(ctl, "%-15s: %llu\n", params[i].field, - params[i].value.ul); - break; - case VIR_TYPED_PARAM_DOUBLE: - vshPrint(ctl, "%-15s: %f\n", params[i].field, - params[i].value.d); - break; - case VIR_TYPED_PARAM_BOOLEAN: - vshPrint(ctl, "%-15s: %d\n", params[i].field, - params[i].value.b); - break; - case VIR_TYPED_PARAM_STRING: - vshPrint(ctl, "%-15s: %s\n", params[i].field, - params[i].value.s); - break; - default: - vshPrint(ctl, "unimplemented numa parameter type\n"); + if (params[i].type == VIR_TYPED_PARAM_INT && + STREQ(params[i].field, VIR_DOMAIN_NUMA_MODE)) { + vshPrint(ctl, "%-15s: %s\n", params[i].field, + virDomainNumatuneMemModeTypeToString(params[i].value.i)); + } else { + char *str = vshGetTypedParamValue(ctl, &params[i]); + vshPrint(ctl, "%-15s: %s\n", params[i].field, str); + VIR_FREE(str); } } @@ -5269,7 +5242,6 @@ cmdNumatune(vshControl * ctl, const vshCmd * cmd) for (i = 0; i < nparams; i++) { temp = &params[i]; - temp->type = VIR_TYPED_PARAM_ULLONG; /* * Some magic here, this is used to fill the params structure with @@ -5277,22 +5249,27 @@ cmdNumatune(vshControl * ctl, const vshCmd * cmd) * argument we purposely make them 0, so on the next pass it goes * to the next valid argument and so on. */ - if (nodeset) { - temp->value.s = vshStrdup(ctl, nodeset); - temp->type = VIR_TYPED_PARAM_STRING; - if (!virStrcpy(temp->field, VIR_DOMAIN_NUMA_NODESET, - sizeof(temp->field))) - goto cleanup; - nodeset = NULL; - } else if (mode) { + if (mode) { + /* Accept string or integer, in case server + * understands newer integer than what strings we were + * compiled with */ if ((temp->value.i = virDomainNumatuneMemModeTypeFromString(mode)) < 0) { vshError(ctl, "%s %s", _("Invalid mode"), mode); goto cleanup; } - strncpy(temp->field, VIR_DOMAIN_NUMA_MODE, - sizeof(temp->field)); + if (!virStrcpy(temp->field, VIR_DOMAIN_NUMA_MODE, + sizeof(temp->field))) + goto cleanup; + temp->type = VIR_TYPED_PARAM_INT; mode = NULL; + } else if (nodeset) { + temp->value.s = vshStrdup(ctl, nodeset); + temp->type = VIR_TYPED_PARAM_STRING; + if (!virStrcpy(temp->field, VIR_DOMAIN_NUMA_NODESET, + sizeof(temp->field))) + goto cleanup; + nodeset = NULL; } } if (virDomainSetNumaParameters(dom, params, nparams, flags) != 0) @@ -5302,6 +5279,7 @@ cmdNumatune(vshControl * ctl, const vshCmd * cmd) } cleanup: + virTypedParameterArrayClear(params, nparams); VIR_FREE(params); virDomainFree(dom); return ret; @@ -15412,7 +15390,6 @@ static const vshCmdDef domManagementCmds[] = { info_managedsaveremove, 0}, {"maxvcpus", cmdMaxvcpus, opts_maxvcpus, info_maxvcpus, 0}, {"memtune", cmdMemtune, opts_memtune, info_memtune, 0}, - {"numatune", cmdNumatune, opts_numatune, info_numatune, 0}, {"migrate", cmdMigrate, opts_migrate, info_migrate, 0}, {"migrate-setmaxdowntime", cmdMigrateSetMaxDowntime, opts_migrate_setmaxdowntime, info_migrate_setmaxdowntime, 0}, @@ -15420,6 +15397,7 @@ static const vshCmdDef domManagementCmds[] = { opts_migrate_setspeed, info_migrate_setspeed, 0}, {"migrate-getspeed", cmdMigrateGetMaxSpeed, opts_migrate_getspeed, info_migrate_getspeed, 0}, + {"numatune", cmdNumatune, opts_numatune, info_numatune, 0}, {"reboot", cmdReboot, opts_reboot, info_reboot, 0}, {"reset", cmdReset, opts_reset, info_reset, 0}, {"restore", cmdRestore, opts_restore, info_restore, 0}, -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org