[libvirt] [PATCH] virsh: print error in case of cellno is invalid
If invalid cellno is specified, command "freecell" will still print the amount of available memory of node. As a fix, print error instead. * tools/virsh.c: "vshCommandOptInt", return -1 when value for parameter is specified, but invalid, which means strtol was failed, it won't affects other functions that use "vshCommandOptInt"). --- tools/virsh.c | 14 ++++++++++++-- 1 files changed, 12 insertions(+), 2 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 55e2a68..31f2a54 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2275,6 +2275,12 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd) return FALSE; cell = vshCommandOptInt(cmd, "cellno", &cell_given); + + if (cell == -1) { + vshError(ctl, "%s", _("Invalid value for 'cellno', expecting an int")); + return FALSE; + } + if (!cell_given) { memory = virNodeGetFreeMemory(ctl->conn); if (memory == 0) @@ -10440,13 +10446,17 @@ vshCommandOptInt(const vshCmd *cmd, const char *name, int *found) if ((arg != NULL) && (arg->data != NULL)) { res = strtol(arg->data, &end_p, 10); - if ((arg->data == end_p) || (*end_p!= 0)) + + if ((arg->data == end_p) || (*end_p!= 0)) { num_found = FALSE; - else + res = -1; + } else num_found = TRUE; } + if (found) *found = num_found; + return res; } -- 1.7.3.2
On 01/05/2011 07:03 AM, Osier Yang wrote:
If invalid cellno is specified, command "freecell" will still print the amount of available memory of node. As a fix, print error instead.
* tools/virsh.c: "vshCommandOptInt", return -1 when value for parameter is specified, but invalid, which means strtol was failed, it won't affects other functions that use "vshCommandOptInt"). --- tools/virsh.c | 14 ++++++++++++-- 1 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 55e2a68..31f2a54 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2275,6 +2275,12 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd) return FALSE;
cell = vshCommandOptInt(cmd, "cellno", &cell_given); + + if (cell == -1) { + vshError(ctl, "%s", _("Invalid value for 'cellno', expecting an int"));
-1 is a valid int, but not a valid cellno, so --cellno=-1 would now give a misleading message. I also don't like the fact that you are changing the semantics of vshCommandOptInt, but not the counterparts such as vshCommandOptUL. I'd rather see all the vshCommandOpt* functions have the same semantics regarding their found parameter. And, since some of the vshCommandOpt* return unsigned values, you can't rely on -1 as a sentinel return value to indicate argument present but invalid. You'd have to go with something different, such as altering the semantics of the found argument: instead of being a binary TRUE/FALSE return (using TRUE/FALSE and int* is nasty anyways, because we already have <stdbool.h> and could be using true/false and bool* instead), let's instead have it be a ternary value: found < 0 => argument was present, but invalid (not an integer); return value is 0 found == 0 => argument was missing; return value is 0 found > 0 => argument was present and valid; return value is its value but this means that all existing callers that pass NULL instead of &found as the third argument are silently ignoring errors, at which point, it seems like we should require a non-NULL found argument. But if we do that, we might as well go one step further, and swap the order of the API entirely (to force ALL callers to check for errors): int vshCommandOptUL(const vshCmd *cmd, const char *name, unsigned long *value) ATTRIBUTE_NONNULL(3); Return value is <0 on failure (*value untouched), 0 when option is absent (*value untouched), and >0 on success (*value updated). Swapping the API like that also has the benefit that a client can specify a non-zero default: unsigned long value = 1024; if (vshCommandOptUL(cmd, "arg", &value) < 0) { error; return FALSE; } use value rather than the current code usage of checking whether a value was supplied, and if not, supplying the default. Yes, an API swap like this would be a much bigger change, but it seems like it is cleaner in the long run (since invalid cellno can't be the only case where passing a non-integer string gets silently ignored back to the default integer value).
+ if ((arg->data == end_p) || (*end_p!= 0)) { num_found = FALSE; - else + res = -1; + } else num_found = TRUE;
Style nit: you used: if (cond) { abc; def; } else xyz; But we prefer either: if (!cond) xyz; else { abc; def; } or: if (cond) { abc; def; } else { xyz; } since HACKING documents that an else clause should only ever omit braces when the if clause also omitted braces, but an if clause can omit braces even when the else clause requires them. NACK as-is; but I agree that this is (at least one) real bug to be fixed. Does my idea for a broader rewrite of the vshCommandOpt* function semantics make sense? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Jan 05, 2011 at 10:10:00AM -0700, Eric Blake wrote:
Style nit: you used:
if (cond) { abc; def; } else xyz;
But we prefer either:
if (!cond) xyz; else { abc; def; }
or:
if (cond) { abc; def; } else { xyz; }
since HACKING documents that an else clause should only ever omit braces when the if clause also omitted braces, but an if clause can omit braces even when the else clause requires them.
Hmm, I didn't notice that. I really don't like to see braces in else clauses, without also seeing braces in the if, and have been fixing this to add braces whenever I come across it. IMHO the hacking guideline should only allow if (foo) bar; else wizz; Or if (foo) { bar; ... } else { wizz; ... } Regards, Daniel
* docs/hacking.html.in (Curly braces): Tighten recommendations to disallow if (cond) one-line; else { block; }. * HACKING: Regenerate. Suggested by Daniel P. Berrange. ---
since HACKING documents that an else clause should only ever omit braces when the if clause also omitted braces, but an if clause can omit braces even when the else clause requires them. Hmm, I didn't notice that. I really don't like to see braces in else clauses, without also seeing braces in the if, and have been fixing this to add braces whenever I come across it.
Fine by me - how does this look? HACKING | 35 ++++++++++++++++++++++------------- docs/hacking.html.in | 34 +++++++++++++++++++++++----------- 2 files changed, 45 insertions(+), 24 deletions(-) diff --git a/HACKING b/HACKING index d646709..4a71b37 100644 --- a/HACKING +++ b/HACKING @@ -161,33 +161,42 @@ Do this, instead: However, there is one exception in the other direction, when even a one-line block should have braces. That occurs when that one-line, brace-less block is -an "else" block, and the corresponding "then" block *does* use braces. In that -case, either put braces around the "else" block, or negate the "if"-condition -and swap the bodies, putting the one-line block first and making the longer, -multi-line block be the "else" block. +an "if" or "else" block, and the counterpart block *does* use braces. In that +case, put braces around both blocks. Also, if the "else" block is much shorter +than the "if" block, consider negating the "if"-condition and swapping the +bodies, putting the short block first and making the longer, multi-line block +be the "else" block. if (expr) { ... ... } else - x = y; // BAD: braceless "else" with braced "then" + x = y; // BAD: braceless "else" with braced "then", + // and short block last -This is preferred, especially when the multi-line body is more than a few -lines long, because it is easier to read and grasp the semantics of an -if-then-else block when the simpler block occurs first, rather than after the -more involved block: + if (expr) + x = y; // BAD: braceless "if" with braced "else" + else { + ... + ... + } - if (!expr) +Keeping braces consistent and putting the short block first is preferred, +especially when the multi-line body is more than a few lines long, because it +is easier to read and grasp the semantics of an if-then-else block when the +simpler block occurs first, rather than after the more involved block: + + if (!expr) { x = y; // putting the smaller block first is more readable - else { + } else { ... ... } -If you'd rather not negate the condition, then at least add braces: +But if negating a complex condition is too ugly, then at least add braces: - if (expr) { + if (complex expr not worth negating) { ... ... } else { diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 900e242..0d81b0b 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -209,11 +209,13 @@ <p> However, there is one exception in the other direction, when even a one-line block should have braces. That occurs when that one-line, - brace-less block is an <code>else</code> block, and the corresponding - <code>then</code> block <b>does</b> use braces. In that case, either - put braces around the <code>else</code> block, or negate the - <code>if</code>-condition and swap the bodies, putting the - one-line block first and making the longer, multi-line block be the + brace-less block is an <code>if</code> or <code>else</code> + block, and the counterpart block <b>does</b> use braces. In + that case, put braces around both blocks. Also, if + the <code>else</code> block is much shorter than + the <code>if</code> block, consider negating the + <code>if</code>-condition and swapping the bodies, putting the + short block first and making the longer, multi-line block be the <code>else</code> block. </p> @@ -223,31 +225,41 @@ ... } else - x = y; // BAD: braceless "else" with braced "then" + x = y; // BAD: braceless "else" with braced "then", + // and short block last + + if (expr) + x = y; // BAD: braceless "if" with braced "else" + else { + ... + ... + } </pre> <p> - This is preferred, especially when the multi-line body is more than a + Keeping braces consistent and putting the short block first is + preferred, especially when the multi-line body is more than a few lines long, because it is easier to read and grasp the semantics of an if-then-else block when the simpler block occurs first, rather than after the more involved block: </p> <pre> - if (!expr) + if (!expr) { x = y; // putting the smaller block first is more readable - else { + } else { ... ... } </pre> <p> - If you'd rather not negate the condition, then at least add braces: + But if negating a complex condition is too ugly, then at least + add braces: </p> <pre> - if (expr) { + if (complex expr not worth negating) { ... ... } else { -- 1.7.3.4
On Wed, Jan 05, 2011 at 11:07:28AM -0700, Eric Blake wrote:
* docs/hacking.html.in (Curly braces): Tighten recommendations to disallow if (cond) one-line; else { block; }. * HACKING: Regenerate. Suggested by Daniel P. Berrange. ---
since HACKING documents that an else clause should only ever omit braces when the if clause also omitted braces, but an if clause can omit braces even when the else clause requires them. Hmm, I didn't notice that. I really don't like to see braces in else clauses, without also seeing braces in the if, and have been fixing this to add braces whenever I come across it.
Fine by me - how does this look?
Looks good to me. Daniel
于 2011年01月06日 01:10, Eric Blake 写道:
On 01/05/2011 07:03 AM, Osier Yang wrote:
If invalid cellno is specified, command "freecell" will still print the amount of available memory of node. As a fix, print error instead.
* tools/virsh.c: "vshCommandOptInt", return -1 when value for parameter is specified, but invalid, which means strtol was failed, it won't affects other functions that use "vshCommandOptInt"). --- tools/virsh.c | 14 ++++++++++++-- 1 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 55e2a68..31f2a54 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2275,6 +2275,12 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd) return FALSE;
cell = vshCommandOptInt(cmd, "cellno",&cell_given); + + if (cell == -1) { + vshError(ctl, "%s", _("Invalid value for 'cellno', expecting an int"));
-1 is a valid int, but not a valid cellno, so --cellno=-1 would now give a misleading message.
urgh, yes.
I also don't like the fact that you are changing the semantics of vshCommandOptInt, but not the counterparts such as vshCommandOptUL. I'd rather see all the vshCommandOpt* functions have the same semantics regarding their found parameter.
And, since some of the vshCommandOpt* return unsigned values, you can't rely on -1 as a sentinel return value to indicate argument present but invalid. You'd have to go with something different, such as altering the semantics of the found argument: instead of being a binary TRUE/FALSE return (using TRUE/FALSE and int* is nasty anyways, because we already have<stdbool.h> and could be using true/false and bool* instead), let's instead have it be a ternary value:
found< 0 => argument was present, but invalid (not an integer); return value is 0 found == 0 => argument was missing; return value is 0 found> 0 => argument was present and valid; return value is its value
but this means that all existing callers that pass NULL instead of &found as the third argument are silently ignoring errors, at which point, it seems like we should require a non-NULL found argument. But if we do that, we might as well go one step further, and swap the order of the API entirely (to force ALL callers to check for errors):
int vshCommandOptUL(const vshCmd *cmd, const char *name, unsigned long *value) ATTRIBUTE_NONNULL(3);
Return value is<0 on failure (*value untouched), 0 when option is absent (*value untouched), and>0 on success (*value updated). Swapping the API like that also has the benefit that a client can specify a non-zero default:
agree, I thought like so when making the patch, but was not sure if it would make easy things complicated.
unsigned long value = 1024; if (vshCommandOptUL(cmd, "arg",&value)< 0) { error; return FALSE; } use value
rather than the current code usage of checking whether a value was supplied, and if not, supplying the default.
Yes, an API swap like this would be a much bigger change, but it seems like it is cleaner in the long run (since invalid cellno can't be the only case where passing a non-integer string gets silently ignored back to the default integer value).
indeed, we do have more than one bug caused by this problem.
+ if ((arg->data == end_p) || (*end_p!= 0)) { num_found = FALSE; - else + res = -1; + } else num_found = TRUE;
Style nit: you used:
if (cond) { abc; def; } else xyz;
But we prefer either:
if (!cond) xyz; else { abc; def; }
or:
if (cond) { abc; def; } else { xyz; }
since HACKING documents that an else clause should only ever omit braces when the if clause also omitted braces, but an if clause can omit braces even when the else clause requires them.
good to known, thanks.
NACK as-is; but I agree that this is (at least one) real bug to be fixed. Does my idea for a broader rewrite of the vshCommandOpt* function semantics make sense?
+1, as a vote. Regards Osier
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump -
Osier Yang