On 03/01/2011 03:16 AM, Michal Privoznik wrote: > This is needed to detect situations when optional argument was > specified with non-integer value: '--int-opt foo'. > --- > tools/virsh.c | 46 +++++++++++++++++++++++++++------------------- > 1 files changed, 27 insertions(+), 19 deletions(-) > > diff --git a/tools/virsh.c b/tools/virsh.c > index 62fca17..e5093a2 100644 > --- a/tools/virsh.c > +++ b/tools/virsh.c > @@ -252,13 +252,14 @@ static const vshCmdGrp *vshCmdGrpSearch(const char *grpname); > static int vshCmdGrpHelp(vshControl *ctl, const char *name); > > static vshCmdOpt *vshCommandOpt(const vshCmd *cmd, const char *name); > -static int vshCommandOptInt(const vshCmd *cmd, const char *name, int *found); > +static int vshCommandOptInt(const vshCmd *cmd, const char *name, int *found, > + int *opt_found); This is awkward. You now have two optional parameters, and still can't provide a default value easily. I'd much rather see this (as pointed out in https://www.redhat.com/archives/libvir-list/2011-January/msg00145.html): 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 That is - the current code returns the parse value, and with your patch would provide tri-state information via two optional pointers; but my preference would be to swap things and provide a tri-state return code and put the parse value in a required pointer. Furthermore, this should be done to _all_ of the vshCommandOpt<int> family of functions. vshCommandOptString may be worth swapping for consistency, since it is also a tri-state (--option not provided, --option provided but with no [or empty] string, and --option provided with string), although I'm not as convinced on that case as I am on the optional integer parsing issue (--option not provided, --option provided but parse as integer failed, --option provided and integer was parsed in range). Fortunately, the size of your patch proves that there weren't that many places to change. I'll see about doing the work myself in the next 15 minutes... -- Eric Blake eblake@xxxxxxxxxx +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list