[libvirt] [PATCH 1/2] virsh: Change integer option parsing functions to return tri-state information.

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110304/0cca5e6e/attachment-0001.sig>