ä 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list