[libvirt] [PATCH 1/2 v2] node_memory: Do not fail if there is parameter unsupported
It makes no sense to fail the whole getting command if there is parameter unsupported by the kernel. This patch fixes it by omitting the unsupported parameter for getMemoryParameters. And For setMemoryParameters, this checks if there is unsupported parameter up front of the setting, and just returns failure if not all parameters are supported. --- v1 - v2: * Change the behaviour of APIs with regard to the agreement in https://www.redhat.com/archives/libvir-list/2012-November/msg01164.html --- src/libvirt.c | 11 +++-- src/nodeinfo.c | 127 ++++++++++++++++++++++++++++++++++++------------------- 2 files changed, 89 insertions(+), 49 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 4af6089..e851049 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -6746,10 +6746,10 @@ error: * @nparams: pointer to number of memory parameters; input and output * @flags: extra flags; not used yet, so callers should always pass 0 * - * Get all node memory parameters. On input, @nparams gives the size - * of the @params array; on output, @nparams gives how many slots were - * filled with parameter information, which might be less but will - * not exceed the input value. + * Get all node memory parameters (parameter unsupported by OS will be + * omitted). On input, @nparams gives the size of the @params array; + * on output, @nparams gives how many slots were filled with parameter + * information, which might be less but will not exceed the input value. * * As a special case, calling with @params as NULL and @nparams as 0 on * input will cause @nparams on output to contain the number of parameters @@ -6811,7 +6811,8 @@ error: * value nparams of virDomainGetSchedulerType) * @flags: extra flags; not used yet, so callers should always pass 0 * - * Change all or a subset of the node memory tunables. + * Change all or a subset of the node memory tunables. The function + * fails if not all of the tunables are supported. * * Note that it's not recommended to use this function while the * outside tuning program is running (such as ksmtuned under Linux), diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 75d6524..096000b 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -1058,14 +1058,15 @@ nodeGetCPUBitmap(int *max_id ATTRIBUTE_UNUSED) #ifdef __linux__ static int -nodeSetMemoryParameterValue(const char *field, - virTypedParameterPtr param) +nodeSetMemoryParameterValue(virTypedParameterPtr param) { char *path = NULL; char *strval = NULL; int ret = -1; int rc = -1; + char *field = strchr(param->field, '_'); + field++; if (virAsprintf(&path, "%s/%s", SYSFS_MEMORY_SHARED_PATH, field) < 0) { virReportOOMError(); @@ -1080,7 +1081,7 @@ nodeSetMemoryParameterValue(const char *field, } if ((rc = virFileWriteStr(path, strval, 0)) < 0) { - virReportSystemError(-rc, _("failed to set %s"), field); + virReportSystemError(-rc, _("failed to set %s"), param->field); goto cleanup; } @@ -1090,6 +1091,38 @@ cleanup: VIR_FREE(strval); return ret; } + +static bool +nodeMemoryParametersIsAllSupported(virTypedParameterPtr params, + int nparams) +{ + char *path = NULL; + int i; + + for (i = 0; i < nparams; i++) { + virTypedParameterPtr param = ¶ms[i]; + + char *field = strchr(param->field, '_'); + field++; + if (virAsprintf(&path, "%s/%s", + SYSFS_MEMORY_SHARED_PATH, field) < 0) { + virReportOOMError(); + return false; + } + + if (!virFileExists(path)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Parameter '%s' is not supported by " + "this kernel"), param->field); + VIR_FREE(path); + return false; + } + + VIR_FREE(path); + } + + return true; +} #endif int @@ -1101,8 +1134,8 @@ nodeSetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, virCheckFlags(0, -1); #ifdef __linux__ - int ret = 0; int i; + int rc; if (virTypedParameterArrayValidate(params, nparams, VIR_NODE_MEMORY_SHARED_PAGES_TO_SCAN, @@ -1114,34 +1147,18 @@ nodeSetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, NULL) < 0) return -1; - for (i = 0; i < nparams; i++) { - virTypedParameterPtr param = ¶ms[i]; - - if (STREQ(param->field, - VIR_NODE_MEMORY_SHARED_PAGES_TO_SCAN)) { - ret = nodeSetMemoryParameterValue("pages_to_scan", param); - - /* Out of memory */ - if (ret == -2) - return -1; - } else if (STREQ(param->field, - VIR_NODE_MEMORY_SHARED_SLEEP_MILLISECS)) { - ret = nodeSetMemoryParameterValue("sleep_millisecs", param); + if (!nodeMemoryParametersIsAllSupported(params, nparams)) + return -1; - /* Out of memory */ - if (ret == -2) - return -1; - } else if (STREQ(param->field, - VIR_NODE_MEMORY_SHARED_MERGE_ACROSS_NODES)) { - ret = nodeSetMemoryParameterValue("merge_across_nodes", param); + for (i = 0; i < nparams; i++) { + rc = nodeSetMemoryParameterValue(¶ms[i]); - /* Out of memory */ - if (ret == -2) - return -1; - } + /* Out of memory */ + if (rc == -2) + return -1; } - return ret; + return 0; #else virReportError(VIR_ERR_NO_SUPPORT, "%s", _("node set memory parameters not implemented" @@ -1167,6 +1184,11 @@ nodeGetMemoryParameterValue(const char *field, goto cleanup; } + if (!virFileExists(path)) { + ret = -2; + goto cleanup; + } + if (virFileReadAll(path, 1024, &buf) < 0) goto cleanup; @@ -1217,6 +1239,7 @@ nodeGetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, unsigned long long pages_volatile; unsigned long long full_scans = 0; int i; + int ret; if ((*nparams) == 0) { *nparams = NODE_MEMORY_PARAMETERS_NUM; @@ -1228,8 +1251,10 @@ nodeGetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, switch (i) { case 0: - if (nodeGetMemoryParameterValue("pages_to_scan", - &pages_to_scan) < 0) + ret = nodeGetMemoryParameterValue("pages_to_scan", &pages_to_scan); + if (ret == -2) + continue; + else if (ret == -1) return -1; if (virTypedParameterAssign(param, VIR_NODE_MEMORY_SHARED_PAGES_TO_SCAN, @@ -1239,8 +1264,10 @@ nodeGetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, break; case 1: - if (nodeGetMemoryParameterValue("sleep_millisecs", - &sleep_millisecs) < 0) + ret = nodeGetMemoryParameterValue("sleep_millisecs", &sleep_millisecs); + if (ret == -2) + continue; + else if (ret == -1) return -1; if (virTypedParameterAssign(param, VIR_NODE_MEMORY_SHARED_SLEEP_MILLISECS, @@ -1250,8 +1277,10 @@ nodeGetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, break; case 2: - if (nodeGetMemoryParameterValue("pages_shared", - &pages_shared) < 0) + ret = nodeGetMemoryParameterValue("pages_shared", &pages_shared); + if (ret == -2) + continue; + else if (ret == -1) return -1; if (virTypedParameterAssign(param, VIR_NODE_MEMORY_SHARED_PAGES_SHARED, @@ -1261,8 +1290,10 @@ nodeGetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, break; case 3: - if (nodeGetMemoryParameterValue("pages_sharing", - &pages_sharing) < 0) + ret = nodeGetMemoryParameterValue("pages_sharing", &pages_sharing); + if (ret == -2) + continue; + else if (ret == -1) return -1; if (virTypedParameterAssign(param, VIR_NODE_MEMORY_SHARED_PAGES_SHARING, @@ -1272,8 +1303,10 @@ nodeGetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, break; case 4: - if (nodeGetMemoryParameterValue("pages_unshared", - &pages_unshared) < 0) + ret = nodeGetMemoryParameterValue("pages_unshared", &pages_unshared); + if (ret == -2) + continue; + else if (ret == -1) return -1; if (virTypedParameterAssign(param, VIR_NODE_MEMORY_SHARED_PAGES_UNSHARED, @@ -1283,8 +1316,10 @@ nodeGetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, break; case 5: - if (nodeGetMemoryParameterValue("pages_volatile", - &pages_volatile) < 0) + ret = nodeGetMemoryParameterValue("pages_volatile", &pages_volatile); + if (ret == -2) + continue; + else if (ret == -1) return -1; if (virTypedParameterAssign(param, VIR_NODE_MEMORY_SHARED_PAGES_VOLATILE, @@ -1294,8 +1329,10 @@ nodeGetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, break; case 6: - if (nodeGetMemoryParameterValue("full_scans", - &full_scans) < 0) + ret = nodeGetMemoryParameterValue("full_scans", &full_scans); + if (ret == -2) + continue; + else if (ret == -1) return -1; if (virTypedParameterAssign(param, VIR_NODE_MEMORY_SHARED_FULL_SCANS, @@ -1305,8 +1342,10 @@ nodeGetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, break; case 7: - if (nodeGetMemoryParameterValue("merge_across_nodes", - &merge_across_nodes) < 0) + ret = nodeGetMemoryParameterValue("merge_across_nodes", &merge_across_nodes); + if (ret == -2) + continue; + else if (ret == -1) return -1; if (virTypedParameterAssign(param, VIR_NODE_MEMORY_SHARED_MERGE_ACROSS_NODES, -- 1.7.7.6
The 3 options accept 0, checking with if on the values of these options causes wrong results. --- v1 - v2: * Rebasing only --- tools/virsh-host.c | 40 ++++++++++++++++++++++------------------ 1 files changed, 22 insertions(+), 18 deletions(-) diff --git a/tools/virsh-host.c b/tools/virsh-host.c index 5e6842a..3701f56 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -965,35 +965,39 @@ cmdNodeMemoryTune(vshControl *ctl, const vshCmd *cmd) unsigned int shm_pages_to_scan = 0; unsigned int shm_sleep_millisecs = 0; unsigned int shm_merge_across_nodes = 0; + bool has_shm_pages_to_scan = false; + bool has_shm_sleep_millisecs = false; + bool has_shm_merge_across_nodes = false; bool ret = false; + int rc = -1; int i = 0; - if (vshCommandOptUInt(cmd, "shm-pages-to-scan", - &shm_pages_to_scan) < 0) { + if ((rc = vshCommandOptUInt(cmd, "shm-pages-to-scan", + &shm_pages_to_scan)) < 0) { vshError(ctl, "%s", _("invalid shm-pages-to-scan number")); return false; + } else if (rc > 0) { + nparams++; + has_shm_pages_to_scan = true; } - if (vshCommandOptUInt(cmd, "shm-sleep-millisecs", - &shm_sleep_millisecs) < 0) { + if ((rc = vshCommandOptUInt(cmd, "shm-sleep-millisecs", + &shm_sleep_millisecs)) < 0) { vshError(ctl, "%s", _("invalid shm-sleep-millisecs number")); return false; + } else if (rc > 0) { + nparams++; + has_shm_sleep_millisecs = true; } - if (vshCommandOptUInt(cmd, "shm-merge-across-nodes", - &shm_merge_across_nodes) < 0) { + if ((rc = vshCommandOptUInt(cmd, "shm-merge-across-nodes", + &shm_merge_across_nodes)) < 0) { vshError(ctl, "%s", _("invalid shm-merge-across-nodes number")); return false; - } - - if (shm_pages_to_scan) - nparams++; - - if (shm_sleep_millisecs) - nparams++; - - if (shm_merge_across_nodes) + } else if (rc > 0) { nparams++; + has_shm_merge_across_nodes = true; + } if (nparams == 0) { /* Get the number of memory parameters */ @@ -1030,7 +1034,7 @@ cmdNodeMemoryTune(vshControl *ctl, const vshCmd *cmd) /* Set the memory parameters */ params = vshCalloc(ctl, nparams, sizeof(*params)); - if (i < nparams && shm_pages_to_scan) { + if (i < nparams && has_shm_pages_to_scan) { if (virTypedParameterAssign(¶ms[i++], VIR_NODE_MEMORY_SHARED_PAGES_TO_SCAN, VIR_TYPED_PARAM_UINT, @@ -1038,7 +1042,7 @@ cmdNodeMemoryTune(vshControl *ctl, const vshCmd *cmd) goto error; } - if (i < nparams && shm_sleep_millisecs) { + if (i < nparams && has_shm_sleep_millisecs) { if (virTypedParameterAssign(¶ms[i++], VIR_NODE_MEMORY_SHARED_SLEEP_MILLISECS, VIR_TYPED_PARAM_UINT, @@ -1046,7 +1050,7 @@ cmdNodeMemoryTune(vshControl *ctl, const vshCmd *cmd) goto error; } - if (i < nparams && shm_merge_across_nodes) { + if (i < nparams && has_shm_merge_across_nodes) { if (virTypedParameterAssign(¶ms[i++], VIR_NODE_MEMORY_SHARED_MERGE_ACROSS_NODES, VIR_TYPED_PARAM_UINT, -- 1.7.7.6
The 3 options accept 0, checking with if on the values of these options causes wrong results.
I had to re-read that twice before I parsed it. Maybe: The 3 options accept 0, and merely checking for non-zero values would cause wrong results. ACK.
On 2012年11月29日 07:54, Eric Blake wrote:
The 3 options accept 0, checking with if on the values of these options causes wrong results.
I had to re-read that twice before I parsed it. Maybe:
My bad. :-)
The 3 options accept 0, and merely checking for non-zero values would cause wrong results.
This is better, and I pushed with changing to it. Regards, Osier
It makes no sense to fail the whole getting command if there is parameter unsupported by the kernel. This patch fixes it by
s/parameter/a parameter/
omitting the unsupported parameter for getMemoryParameters.
And For setMemoryParameters, this checks if there is unsupported
s/For/for/ s/is/is an/
parameter up front of the setting, and just returns failure if not all parameters are supported.
- * filled with parameter information, which might be less but will - * not exceed the input value. + * Get all node memory parameters (parameter unsupported by OS will be
s/parameter/parameters/
#ifdef __linux__ static int -nodeSetMemoryParameterValue(const char *field, - virTypedParameterPtr param) +nodeSetMemoryParameterValue(virTypedParameterPtr param) { char *path = NULL; char *strval = NULL; int ret = -1; int rc = -1;
+ char *field = strchr(param->field, '_'); + field++;
This site should be safe (we only get here if we got past earlier filters), but...
+static bool +nodeMemoryParametersIsAllSupported(virTypedParameterPtr params, + int nparams) +{ + char *path = NULL; + int i; + + for (i = 0; i < nparams; i++) { + virTypedParameterPtr param = ¶ms[i]; + + char *field = strchr(param->field, '_'); + field++;
Are we guaranteed that field is non-NULL, or could the user pass us a param->field name that doesn't contain '_', in which case we are doing bad dereferencing? That is, since this is the earlier filter that makes the rest of the code safe, I think you need to be prepared for the user to pass unexpected strings, and fail if field is NULL at this point. ACK with that fixed.
On 2012年11月29日 07:49, Eric Blake wrote:
It makes no sense to fail the whole getting command if there is parameter unsupported by the kernel. This patch fixes it by
s/parameter/a parameter/
omitting the unsupported parameter for getMemoryParameters.
And For setMemoryParameters, this checks if there is unsupported
s/For/for/ s/is/is an/
parameter up front of the setting, and just returns failure if not all parameters are supported.
- * filled with parameter information, which might be less but will - * not exceed the input value. + * Get all node memory parameters (parameter unsupported by OS will be
s/parameter/parameters/
#ifdef __linux__ static int -nodeSetMemoryParameterValue(const char *field, - virTypedParameterPtr param) +nodeSetMemoryParameterValue(virTypedParameterPtr param) { char *path = NULL; char *strval = NULL; int ret = -1; int rc = -1;
+ char *field = strchr(param->field, '_'); + field++;
This site should be safe (we only get here if we got past earlier filters), but...
+static bool +nodeMemoryParametersIsAllSupported(virTypedParameterPtr params, + int nparams) +{ + char *path = NULL; + int i; + + for (i = 0; i< nparams; i++) { + virTypedParameterPtr param =¶ms[i]; + + char *field = strchr(param->field, '_'); + field++;
Are we guaranteed that field is non-NULL,
Yeah, it's guaranteed by virTypedParameterArrayValidate, any param that not in the set VIR_NODE_MEMORY_SHARED_PAGES_TO_SCAN, VIR_NODE_MEMORY_SHARED_SLEEP_MILLISECS, and VIR_NODE_MEMORY_SHARED_MERGE_ACROSS_NODES will be detected, and all of these 3 params contains "_". Thanks the reviewing, I pushed it with the typos fixed. Regards, Osier
participants (2)
-
Eric Blake -
Osier Yang