On 02/15/2011 01:09 AM, Michal Privoznik wrote:

Non-digit characters were accepted in --cellno resulting in unwanted behavior: 'virsh freecell --cellno foo' wasn't rejected. --- This fix addresses bug 639587

tools/virsh.c | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c index c2d165d..765566d 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2283,7 +2283,7 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd) { int func_ret = FALSE; int ret; - int cell, cell_given; + int cell, cell_given, cellStr_given; unsigned long long memory; unsigned long long *nodes = NULL; int all_given; @@ -2295,6 +2295,13 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd)

cell = vshCommandOptInt(cmd, "cellno", &cell_given); all_given = vshCommandOptBool(cmd, "all"); + vshCommandOptString(cmd, "cellno", &cellStr_given);

Hmm. Why are we parsing "cellno" twice, once with OptInt, and again with OptString? We REALLY need to overhaul virsh.c to make the parsers return tri-state information (no option present, option present and parse valid, option present but parse invalid), and update all callers accordingly. In fact, there's already a thread on this exact same topic: https://www.redhat.com/archives/libvir-list/2011-January/msg00132.html with my thoughts on a better rewrite: https://www.redhat.com/archives/libvir-list/2011-January/msg00145.html

+ + if (cellStr_given && !cell_given) { + vshError(ctl, "%s", _("cell number must not contain any " + "non-digit characters.")); + goto cleanup; + }

However, your patch does have the advantage over Osier's first attempt, in that it does not affect all other callers and does not give a misleading message for --cellno=-1. Unless anyone else has any strong opinions on getting this fixed prior to 0.8.8, my inclination would be to postpone this issue until post-release (after all, it only affects users that pass invalid option arguments; it doesn't affect valid uses of the freecell command), and go with the full-blown rewrite of the option argument parsing routines to better handle tri-state detection and default values. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org