[libvirt] [PATCH v2] virsh: Change option parsing functions to return tri-state information.

[Eric Blake]

On 03/07/2011 11:46 AM, Michal Privoznik wrote:
> This is needed to detect situations when optional argument was
> specified with non-integer value: '--int-opt foo'. To keep functions
> uniform vshCommandOptString function was also changed, because it
> returns tri-state value as well. Given result pointer is updated only
> in case of success. If parsing fails, result is not updated at all.
> 
> diff --git a/tools/virsh.c b/tools/virsh.c
> index f3754d7..c274a6b 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 unsigned long vshCommandOptUL(const vshCmd *cmd, const char *name,
> -                                     int *found);
> -static char *vshCommandOptString(const vshCmd *cmd, const char *name,
> -                                 int *found);

Ouch - this was not const-correct pre-patch, but at least assigning
'char *' to 'const char *' is trivial...

> -static long long vshCommandOptLongLong(const vshCmd *cmd, const char *name,
> -                                       int *found);
> +static int vshCommandOptInt(const vshCmd *cmd, const char *name, int *value)
> +                            ATTRIBUTE_NONNULL(3);
> +static int vshCommandOptUL(const vshCmd *cmd, const char *name,
> +                           unsigned long *value) ATTRIBUTE_NONNULL(3);
> +static int vshCommandOptString(const vshCmd *cmd, const char *name,
> +                               char **value) ATTRIBUTE_NONNULL(3);

whereas this rewrite exposes the const-correctness issue - you can't
pass 'const char* var; &var' to a 'char **' argument and still keep the
compiler happy...

> +static int vshCommandOptLongLong(const vshCmd *cmd, const char *name,
> +                                 unsigned long long *value) ATTRIBUTE_NONNULL(3);
>  static int vshCommandOptBool(const vshCmd *cmd, const char *name);
>  static char *vshCommandOptArgv(const vshCmd *cmd, int count);
>  
> @@ -589,9 +590,9 @@ static const vshCmdOptDef opts_help[] = {
>  static int
>  cmdHelp(vshControl *ctl, const vshCmd *cmd)
>   {
> -    const char *name;
> +    char *name = NULL;
>  
> -    name = vshCommandOptString(cmd, "command", NULL);
> +    vshCommandOptString(cmd, "command", &name);

...leading to instances like this where you nukes const designators...

> @@ -773,7 +773,7 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd)
>  {
>      virDomainPtr dom;
>      int ret;
> -    const char *name;
> +    const char *name = NULL;
>  
>      if (!vshConnectionUsability(ctl, ctl->conn))
>          return FALSE;
> @@ -781,7 +781,7 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd)
>      if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
>          return FALSE;
>  
> -    name = vshCommandOptString(cmd, "devname", NULL);
> +    vshCommandOptString(cmd, "devname", (char**) &name);

...or even worse, where you had to introduce a cast.  Would you mind
splitting this into two patches?

The first patch should _just_ touch vshCommandOptString to change its
return type to const char *, and fix the fallout in the few functions
that need to be a bit more careful about type-safety.  I already _know_
that cmdCd will be impacted: commit 5a814012 touched that function, and
exposed a place where we had been leaking memory when
vshCommandOptString returned NULL so we replaced things with a malloc'd
string, so there you will have to split things into using two variables,
one 'const char *' for vshCommandOptString, and the other 'char *' when
doing fallback.  But hopefully most other functions will be okay with
using 'const char *' in the first place.

Then the second patch will swap argument order for all the vshCommandOpt
functions, including vshCommandOpt taking a const char ** third
argument, which should just work without any casts or removal of const
throughout the rest of the code.

That said,

> @@ -1288,16 +1286,14 @@ static int
>  cmdDefine(vshControl *ctl, const vshCmd *cmd)
>  {
>      virDomainPtr dom;
> -    char *from;
> -    int found;
> +    char *from = NULL;
>      int ret = TRUE;
>      char *buffer;
>  
>      if (!vshConnectionUsability(ctl, ctl->conn))
>          return FALSE;
>  
> -    from = vshCommandOptString(cmd, "file", &found);
> -    if (!found)
> +    if (vshCommandOptString(cmd, "file", &from) <= 0)
>          return FALSE;

The rest of this patch (module the const issues) shows how nice the
change is!  Your patch didn't include a diffstat, but it looks like you
have a net reduction in lines of code (always a good sign that the
cleanup was worth it).

> @@ -2862,7 +2852,7 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
>      if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
>          return FALSE;
>  
> -    count = vshCommandOptInt(cmd, "count", &count);
> +     vshCommandOptInt(cmd, "count", &count);

Why the indentation change here?

> @@ -2918,7 +2908,7 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd)
>  {
>      virDomainPtr dom;
>      virDomainInfo info;
> -    unsigned long kilobytes;
> +    unsigned long kilobytes = 0;
>      int ret = TRUE;
>  
>      if (!vshConnectionUsability(ctl, ctl->conn))
> @@ -2927,7 +2917,7 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd)
>      if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
>          return FALSE;
>  
> -    kilobytes = vshCommandOptUL(cmd, "kilobytes", NULL);
> +    vshCommandOptUL(cmd, "kilobytes", &kilobytes);
>      if (kilobytes <= 0) {
>          virDomainFree(dom);
>          vshError(ctl, _("Invalid value of %lu for memory size"), kilobytes);

Hmm, I wonder if we should have been checking if vshCommandOptUL
returned a negative value.  Then again, since you defaulted the value to
0, and the value is unchanged on error, and an explicit 0 is also
invalid, you still end up with the right error message.

If you change the helper functions to be ATTRIBUTE_RETURN_CHECK, then
the compiler would enforce that you check all vshCommandOpt* functions
for parse error.

>  
>  /*
> - * Returns option as INT
> + * @cmd command reference
> + * @name option name
> + * @value result
> + *
> + * Convert option to int
> + * Return value:
> + * >0 if option found and valid (@value updated)
> + * 0 in case option not found (@value untouched)
> + * <0 in all other cases (@value untouched)
>   */
>  static int
> -vshCommandOptInt(const vshCmd *cmd, const char *name, int *found)
> +vshCommandOptInt(const vshCmd *cmd, const char *name, int *value)
>  {
>      vshCmdOpt *arg = vshCommandOpt(cmd, name);
> -    int res = 0, num_found = FALSE;
> +    int ret = 0, num;
>      char *end_p = NULL;
>  
>      if ((arg != NULL) && (arg->data != NULL)) {
> -        res = strtol(arg->data, &end_p, 10);
> -        if ((arg->data == end_p) || (*end_p!= 0))
> -            num_found = FALSE;
> -        else
> -            num_found = TRUE;
> +        num = strtol(arg->data, &end_p, 10);
> +        ret = -1;
> +        if ((arg->data != end_p) && (*end_p == 0) && value) {
> +            *value = num;
> +            ret = 1;
> +        }
>      }
> -    if (found)
> -        *found = num_found;
> -    return res;
> +    return ret;

Nice!  Exactly what I had in mind.

Looking forward to v3.

-- 
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/20110307/7781b9c8/attachment-0001.sig>