On 12/15/2011 03:50 AM, Hu Tao wrote:
> add new command numatune to virsh to get/set numa parameters
> ---
> tools/virsh.c | 181 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 181 insertions(+), 0 deletions(-)
>
> diff --git a/tools/virsh.c b/tools/virsh.c
> index f6571f7..2b509bf 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -61,6 +61,7 @@
> #include "virkeycode.h"
> #include "virnetdevbandwidth.h"
> #include "util/bitmap.h"
> +#include "conf/domain_conf.h"
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?
> + 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));
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.
> + for (i = 0; i < nparams; i++) {
> + temp = ¶ms[i];
> + temp->type = VIR_TYPED_PARAM_ULLONG;
Wrong type. This needs to be done per parameter.
> +
> + /*
> + * Some magic here, this is used to fill the params structure with
> + * the valid arguments passed, after filling the particular
> + * 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) {
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.
> + strncpy(temp->field, VIR_DOMAIN_NUMA_MODE,
> + sizeof(temp->field));
Why strncpy here, but virStrcpy on the other field?
> + cleanup:
> + VIR_FREE(params);
Memory leak. params can contain strdup'd text.
> @@ -15148,6 +15328,7 @@ 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},
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, ¶ms[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 = ¶ms[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@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