[libvirt] [PATCH 1/2] node_memory: Do not fail if there is parameter unsupported
It makes no sense to fail the whole command if there is parameter unsupported by the kernel. This patch fixes it by ignoring the unsupported parameter for getMemoryParameters, and ignoring the unsupported parameter for setMemoryParameters too if there are more than one parameters to set, otherwise error out. --- src/libvirt.c | 12 +++-- src/nodeinfo.c | 119 ++++++++++++++++++++++++++++++++++---------------------- 2 files changed, 79 insertions(+), 52 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 4af6089..93ec068 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 + * ignored). 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,9 @@ 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. Tunable + * unsupported by OS wll be ignored if @nparams is not 1, otherwise + * the function fails. * * 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..718a3a4 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -1058,21 +1058,13 @@ nodeGetCPUBitmap(int *max_id ATTRIBUTE_UNUSED) #ifdef __linux__ static int -nodeSetMemoryParameterValue(const char *field, +nodeSetMemoryParameterValue(const char *path, virTypedParameterPtr param) { - char *path = NULL; char *strval = NULL; int ret = -1; int rc = -1; - if (virAsprintf(&path, "%s/%s", - SYSFS_MEMORY_SHARED_PATH, field) < 0) { - virReportOOMError(); - ret = -2; - goto cleanup; - } - if (virAsprintf(&strval, "%u", param->value.ui) == -1) { virReportOOMError(); ret = -2; @@ -1080,13 +1072,12 @@ 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; } ret = 0; cleanup: - VIR_FREE(path); VIR_FREE(strval); return ret; } @@ -1101,7 +1092,9 @@ nodeSetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, virCheckFlags(0, -1); #ifdef __linux__ - int ret = 0; + char *path = NULL; + int ret = -1; + int rc; int i; if (virTypedParameterArrayValidate(params, nparams, @@ -1117,30 +1110,40 @@ nodeSetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, 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); + char *field = strchr(param->field, '_'); + field++; + if (virAsprintf(&path, "%s/%s", + SYSFS_MEMORY_SHARED_PATH, field) < 0) { + virReportOOMError(); + goto cleanup; + } - /* Out of memory */ - if (ret == -2) - return -1; - } else if (STREQ(param->field, - VIR_NODE_MEMORY_SHARED_SLEEP_MILLISECS)) { - ret = nodeSetMemoryParameterValue("sleep_millisecs", param); + /* Ignore the unsupported the param field if there is other + * parameter to set, otherwise error out. + */ + if (!virFileExists(path)) { + if (nparams == 1) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Parameter '%s' is not supported by " + "this kernel"), param->field); + goto cleanup; + } else { + VIR_WARN("Parameter '%s' is not supported by this kernel", + param->field); + continue; + } + } - /* 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); + rc = nodeSetMemoryParameterValue(path, param); - /* Out of memory */ - if (ret == -2) - return -1; - } + /* Out of memory */ + if (rc == -2) + goto cleanup; } + ret = 0; +cleanup: + VIR_FREE(path); return ret; #else virReportError(VIR_ERR_NO_SUPPORT, "%s", @@ -1167,6 +1170,11 @@ nodeGetMemoryParameterValue(const char *field, goto cleanup; } + if (!virFileExists(path)) { + ret = -2; + goto cleanup; + } + if (virFileReadAll(path, 1024, &buf) < 0) goto cleanup; @@ -1217,6 +1225,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 +1237,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 +1250,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 +1263,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 +1276,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 +1289,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 +1302,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 +1315,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 +1328,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. --- 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
ping On 2012年11月22日 11:14, Osier Yang wrote:
It makes no sense to fail the whole command if there is parameter unsupported by the kernel. This patch fixes it by ignoring the unsupported parameter for getMemoryParameters, and ignoring the unsupported parameter for setMemoryParameters too if there are more than one parameters to set, otherwise error out. --- src/libvirt.c | 12 +++-- src/nodeinfo.c | 119 ++++++++++++++++++++++++++++++++++---------------------- 2 files changed, 79 insertions(+), 52 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index 4af6089..93ec068 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 + * ignored). 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,9 @@ 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. Tunable + * unsupported by OS wll be ignored if @nparams is not 1, otherwise + * the function fails. * * 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..718a3a4 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -1058,21 +1058,13 @@ nodeGetCPUBitmap(int *max_id ATTRIBUTE_UNUSED)
#ifdef __linux__ static int -nodeSetMemoryParameterValue(const char *field, +nodeSetMemoryParameterValue(const char *path, virTypedParameterPtr param) { - char *path = NULL; char *strval = NULL; int ret = -1; int rc = -1;
- if (virAsprintf(&path, "%s/%s", - SYSFS_MEMORY_SHARED_PATH, field)< 0) { - virReportOOMError(); - ret = -2; - goto cleanup; - } - if (virAsprintf(&strval, "%u", param->value.ui) == -1) { virReportOOMError(); ret = -2; @@ -1080,13 +1072,12 @@ 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; }
ret = 0; cleanup: - VIR_FREE(path); VIR_FREE(strval); return ret; } @@ -1101,7 +1092,9 @@ nodeSetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, virCheckFlags(0, -1);
#ifdef __linux__ - int ret = 0; + char *path = NULL; + int ret = -1; + int rc; int i;
if (virTypedParameterArrayValidate(params, nparams, @@ -1117,30 +1110,40 @@ nodeSetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, 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); + char *field = strchr(param->field, '_'); + field++; + if (virAsprintf(&path, "%s/%s", + SYSFS_MEMORY_SHARED_PATH, field)< 0) { + virReportOOMError(); + goto cleanup; + }
- /* Out of memory */ - if (ret == -2) - return -1; - } else if (STREQ(param->field, - VIR_NODE_MEMORY_SHARED_SLEEP_MILLISECS)) { - ret = nodeSetMemoryParameterValue("sleep_millisecs", param); + /* Ignore the unsupported the param field if there is other + * parameter to set, otherwise error out. + */ + if (!virFileExists(path)) { + if (nparams == 1) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Parameter '%s' is not supported by " + "this kernel"), param->field); + goto cleanup; + } else { + VIR_WARN("Parameter '%s' is not supported by this kernel", + param->field); + continue; + } + }
- /* 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); + rc = nodeSetMemoryParameterValue(path, param);
- /* Out of memory */ - if (ret == -2) - return -1; - } + /* Out of memory */ + if (rc == -2) + goto cleanup; }
+ ret = 0; +cleanup: + VIR_FREE(path); return ret; #else virReportError(VIR_ERR_NO_SUPPORT, "%s", @@ -1167,6 +1170,11 @@ nodeGetMemoryParameterValue(const char *field, goto cleanup; }
+ if (!virFileExists(path)) { + ret = -2; + goto cleanup; + } + if (virFileReadAll(path, 1024,&buf)< 0) goto cleanup;
@@ -1217,6 +1225,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 +1237,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 +1250,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 +1263,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 +1276,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 +1289,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 +1302,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 +1315,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 +1328,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,
ping
On 2012年11月22日 11:14, Osier Yang wrote:
It makes no sense to fail the whole command if there is parameter unsupported by the kernel. This patch fixes it by ignoring the unsupported parameter for getMemoryParameters, and ignoring the unsupported parameter for setMemoryParameters too if there are more than one parameters to set, otherwise error out.
I'm not sure I agree with these semantics.
* - * 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 + * ignored). On input, @nparams gives the size of the @params
s/ignored/omitted/ But is this really right, or shouldn't we at least be able to fake things? If the OS doesn't support tuning a variable, then we should return a hard-coded value for the default setting of that variable (in relation to newer kernels that DO support tuning), rather than omitting it altogether.
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,9 @@ 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. Tunable + * unsupported by OS wll be ignored if @nparams is not 1,
w/wll/will/ I disagree. I think we should continue to error out on unrecognized tunables; furthermore, we should error out up front (if the user passes an array of 3 elements, where only the 2nd element is unsupported, then we should NOT modify the first element; that is, we should verify that all the elements are settable before making any modification). It is a disservice to the user to ignore their request for a change. And if you go with my approach that getting parameters should return a hard-coded value rather than omitting unsupported tunables, then the user should be allowed to pass the default value of that tunable; the only thing they cannot do is pass a non-default value.
But is this really right, or shouldn't we at least be able to fake things? If the OS doesn't support tuning a variable, then we should return a hard-coded value for the default setting of that variable (in relation to newer kernels that DO support tuning), rather than omitting it altogether.
After some IRC chatter with Osier, I think that omitting an unsupported parameter is better than hard-coding its value to a default. It is easier to always reject an unknown parameter by name than it is to conditionally reject setting a parameter based on whether its value matches the default. That said...
- * Change all or a subset of the node memory tunables. + * Change all or a subset of the node memory tunables. Tunable + * unsupported by OS wll be ignored if @nparams is not 1,
w/wll/will/
I disagree. I think we should continue to error out on unrecognized tunables; furthermore, we should error out up front (if the user passes an array of 3 elements, where only the 2nd element is unsupported, then we should NOT modify the first element; that is, we should verify that all the elements are settable before making any modification). It is a disservice to the user to ignore their request for a change.
We still need a v2 that rejects unsupported parameters rather than silently ignoring them. Remember, the way virsh and python bindings work their 'set' functions is by first doing a 'get' to see _which_ parameters can be set in the first place; so as long as the 'get' function omits an unsupported parameter, then the 'set' function is less likely to encounter an attempt to change the unsupported parameter.
participants (2)
-
Eric Blake -
Osier Yang