[libvirt] [PATCH 04/17] virsh: Add helper to request string arguments with error reporting

[Osier Yang]

On 2013年01月22日 02:07, Peter Krempa wrote:
> This patch adds a helper function with similar semantics to
> vshCommandOptString that requests a string argument, but does some error
> reporting without the need to do it in the functions themselves.
>
> The error reporting also provides information about the parameter whose
> retrieval failed.
> ---
>   tools/virsh.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   tools/virsh.h |  4 ++++
>   2 files changed, 55 insertions(+)
>
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 908c6a1..1a3cab0 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -1417,6 +1417,57 @@ vshCommandOptString(const vshCmd *cmd, const char *name, const char **value)
>   }
>
>   /**
> + * vshCommandOptStringReq: Get a required string argumment

Trivial, but we usually describe what the function does at [1].

> + * @ctl virsh control structure

And have a ":" after @foo.

> + * @cmd command structure
> + * @name option name
> + * @value result (updated to NULL or the actual value)

s/actual value/option argument/,

> + *

[1]. Right here.

> + * Returns option as string. Prints error message if needed

I think you mean option argument here.

> + * Return value:
> + * 0 if option is correct (available or not required)
> + * -1 and error message printed on error

Not to be captious. But how about rewording above like:

Returns 0 on success or the option is not present and not
requirent, *value is set to the option argument if success;
Or -1 on failure, with error message printed.

> + */
> +int
> +vshCommandOptStringReq(vshControl *ctl,
> +                       const vshCmd *cmd,
> +                       const char *name,
> +                       const char **value)
> +{
> +    vshCmdOpt *arg;
> +    int ret;
> +    const char *error = NULL;
> +
> +    /* clear out the value */
> +    *value = NULL;
> +
> +    ret = vshCommandOpt(cmd, name,&arg);
> +    /* option is not required and not present */
> +    if (ret == 0)
> +        return 0;
> +    /* this should not be propagated here, just to be sure */
> +    if (ret == -1)
> +        error = N_("Mandatory option not present");
> +
> +    if (ret == -2)
> +        error = N_("Programming error: Invalid option name");
> +
> +    if (!arg->data)
> +        error = N_("Programming error: Requested option is a boolean");
> +
> +    if (!*arg->data&&  !(arg->def->flags&  VSH_OFLAG_EMPTY_OK))
> +        error = N_("Option argument is empty");
> +
> +    if (error) {
> +        vshError(ctl, _("Failed to get option '%s': %s"), name, _(error));
> +        return -1;
> +    }
> +
> +    *value = arg->data;
> +    return 0;
> +}
> +
> +/**
>    * vshCommandOptLongLong:
>    * @cmd command reference
>    * @name option name
> diff --git a/tools/virsh.h b/tools/virsh.h
> index ab7161f..fec9785 100644
> --- a/tools/virsh.h
> +++ b/tools/virsh.h
> @@ -275,6 +275,10 @@ int vshCommandOptUL(const vshCmd *cmd, const char *name,
>   int vshCommandOptString(const vshCmd *cmd, const char *name,
>                           const char **value)
>       ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;
> +int vshCommandOptStringReq(vshControl *ctl, const vshCmd *cmd,
> +                           const char *name, const char **value)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
> +    ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK;
>   int vshCommandOptLongLong(const vshCmd *cmd, const char *name,
>                             long long *value)
>       ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;

ACK with the small nits fixed.