[libvirt] [PATCH v2 3/3] virsh: Move error messages inside vshCommandOpt*() functions.

[Michal Privoznik]

On 22.05.2015 10:59, Andrea Bolognani wrote:
> ---
>  tests/vcpupin                |   4 +-
>  tools/virsh-domain-monitor.c |   9 +--
>  tools/virsh-domain.c         | 134 +++++++------------------------------------
>  tools/virsh-host.c           |  57 +++---------------
>  tools/virsh-interface.c      |   6 +-
>  tools/virsh-network.c        |   6 +-
>  tools/virsh-volume.c         |  24 ++------
>  tools/virsh.c                | 111 +++++++++++++++++++++--------------
>  8 files changed, 104 insertions(+), 247 deletions(-)

Nice cleanup.

> diff --git a/tools/virsh.c b/tools/virsh.c
> index 9f44793..db9354c 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -1517,23 +1517,27 @@ vshCommandOpt(const vshCmd *cmd,
>   * <0 in all other cases (@value untouched)
>   */

Does it makes sense to note in comments that this function (and others that you're fixing) is reporting an error?

>  int
> -vshCommandOptInt(vshControl *ctl ATTRIBUTE_UNUSED, const vshCmd *cmd,
> +vshCommandOptInt(vshControl *ctl, const vshCmd *cmd,
>                   const char *name, int *value)
>  {
>      vshCmdOpt *arg;
>      int ret;
>  
> -    ret = vshCommandOpt(cmd, name, &arg, true);
> -    if (ret <= 0)
> +    if ((ret = vshCommandOpt(cmd, name, &arg, true)) <= 0)
>          return ret;
>  
> -    if (virStrToLong_i(arg->data, NULL, 10, value) < 0)
> -        return -1;
> -    return 1;
> +    if ((ret = virStrToLong_i(arg->data, NULL, 10, value)) < 0)
> +        vshError(ctl,
> +                 _("Numeric value '%s' for <%s> option is malformed or out of range"),
> +                 arg->data, name);
> +    else
> +        ret = 1;
> +
> +    return ret;
>  }
>  

While reworking this, you've missed vshCommandOptTimeoutToMs() which in case of something like following '--timeout blah' will report error twice: from both OptInt() and OptTimeoutToMs():

error: Numeric value 'blah' for <timeout> option is malformed or out of range
error: invalid timeout

I think this should be squashed in:

@@ -1906,11 +1907,13 @@ vshCommandOptTimeoutToMs(vshControl *ctl, const vshCmd *cmd,
 {
     int rv = vshCommandOptInt(ctl, cmd, "timeout", timeout);
 
-    if (rv < 0 || (rv > 0 && *timeout < 1)) {
-        vshError(ctl, "%s", _("invalid timeout"));
+    if (rv < 0)
         return -1;
-    }
     if (rv > 0) {
+        if (*timeout < 1) {
+            vshError(ctl, "%s", _("invalid timeout"));
+            return -1;
+        }
         /* Ensure that we can multiply by 1000 without overflowing. */
         if (*timeout > INT_MAX / 1000) {
             vshError(ctl, "%s", _("timeout is too big"));

>  static int
> -vshCommandOptULongLongInternal(vshControl *ctl ATTRIBUTE_UNUSED, const vshCmd *cmd,
> +vshCommandOptULongLongInternal(vshControl *ctl, const vshCmd *cmd,
>                                 const char *name, unsigned long long *value,
>                                 bool wrap)
>  {
> @@ -1754,15 +1767,18 @@ vshCommandOptULongLongInternal(vshControl *ctl ATTRIBUTE_UNUSED, const vshCmd *c
>      if ((ret = vshCommandOpt(cmd, name, &arg, true)) <= 0)
>          return ret;
>  
> -    if (wrap) {
> -        if (virStrToLong_ull(arg->data, NULL, 10, value) < 0)
> -            return -1;
> -    } else {
> -        if (virStrToLong_ullp(arg->data, NULL, 10, value) < 0)
> -            return -1;
> -    }
> +    if (wrap)
> +        ret = virStrToLong_ull(arg->data, NULL, 10, value);
> +    else
> +        ret = virStrToLong_ullp(arg->data, NULL, 10, value);
> +    if (ret < 0)
> +        vshError(ctl,
> +                 _("Numeric value '%s' for <%s> option is malformed or out of range"),
> +                 arg->data, name);
> +    else
> +        ret = 1;
>  
> -    return 1;
> +    return ret;
>  }

You've missed one ocurrance of vshCommandOptULongLong in cmdAllocpages().

>  
>  /**
> @@ -1812,7 +1828,7 @@ vshCommandOptULongLongWrap(vshControl *ctl, const vshCmd *cmd,
>   * See vshCommandOptInt()
>   */
>  int
> -vshCommandOptScaledInt(vshControl *ctl ATTRIBUTE_UNUSED, const vshCmd *cmd,
> +vshCommandOptScaledInt(vshControl *ctl, const vshCmd *cmd,
>                         const char *name, unsigned long long *value,
>                         int scale, unsigned long long max)
>  {
> @@ -1825,9 +1841,16 @@ vshCommandOptScaledInt(vshControl *ctl ATTRIBUTE_UNUSED, const vshCmd *cmd,
>  
>      if (virStrToLong_ull(arg->data, &end, 10, value) < 0 ||
>          virScaleInteger(value, end, scale, max) < 0)
> -        return -1;
> +    {
> +        vshError(ctl,
> +                 _("Numeric value '%s' for <%s> option is malformed or out of range"),
> +                 arg->data, name);
> +        ret = -1;
> +    } else {
> +        ret = 1;
> +    }
>  
> -    return 1;
> +    return ret;
>  }
>  
>  
> 

Interestingly vshCommandOptString() is missing. So far, the only way for the function to fail is if one uses --option "" and VSH_OFLAG_EMPTY_OK is not specified. So if we come up with good error message for that case, we are okay to drop all the other error messages.

Otherwise looking good.

Michal