[libvirt] [PATCH] virsh: Make self-test failures noisy

[Erik Skultety]

On Mon, Mar 11, 2019 at 09:18:44PM -0500, Eric Blake wrote:
> In local testing, I accidentally introduced a self-test failure,
> and spent way too much time debugging it. Make sure the testsuite
> log includes some hint as to why command option validation failed.
>
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  tools/vsh.c | 56 +++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 42 insertions(+), 14 deletions(-)
>
> diff --git a/tools/vsh.c b/tools/vsh.c
> index 2fd1564d15..1d30019c2c 100644
> --- a/tools/vsh.c
> +++ b/tools/vsh.c
> @@ -331,21 +331,26 @@ vshCmddefGetInfo(const vshCmdDef * cmd, const char *name)
>
>  /* Check if the internal command definitions are correct */
>  static int
> -vshCmddefCheckInternals(const vshCmdDef *cmd)
> +vshCmddefCheckInternals(vshControl *ctl,
> +                        const vshCmdDef *cmd)
>  {
>      size_t i;
>      const char *help = NULL;
>
>      /* in order to perform the validation resolve the alias first */
>      if (cmd->flags & VSH_CMD_FLAG_ALIAS) {
> -        if (!cmd->alias)
> +        if (!cmd->alias) {
> +            vshError(ctl, _("command '%s' has inconsistent alias"), cmd->name);
>              return -1;
> +        }
>          cmd = vshCmddefSearch(cmd->alias);
>      }
>
>      /* Each command has to provide a non-empty help string. */
> -    if (!(help = vshCmddefGetInfo(cmd, "help")) || !*help)
> +    if (!(help = vshCmddefGetInfo(cmd, "help")) || !*help) {
> +        vshError(ctl, _("command '%s' lacks help"), cmd->name);
>          return -1;
> +    }
>
>      if (!cmd->opts)
>          return 0;
> @@ -353,14 +358,19 @@ vshCmddefCheckInternals(const vshCmdDef *cmd)
>      for (i = 0; cmd->opts[i].name; i++) {
>          const vshCmdOptDef *opt = &cmd->opts[i];
>
> -        if (i > 63)
> +        if (i > 63) {
> +            vshError(ctl, _("command '%s' has too many options"), cmd->name);
>              return -1; /* too many options */
> +        }
>
>          switch (opt->type) {
>          case VSH_OT_STRING:
>          case VSH_OT_BOOL:
> -            if (opt->flags & VSH_OFLAG_REQ)
> -                return -1; /* nor bool nor string options can't be mandatory */
> +            if (opt->flags & VSH_OFLAG_REQ) {
> +                vshError(ctl, _("command '%s' misused VSH_OFLAG_REQ"),
> +                         cmd->name);
> +                return -1; /* neither bool nor string options can be mandatory */
> +            }
>              break;
>
>          case VSH_OT_ALIAS: {
> @@ -368,11 +378,17 @@ vshCmddefCheckInternals(const vshCmdDef *cmd)
>              char *name = (char *)opt->help; /* cast away const */
>              char *p;
>
> -            if (opt->flags || !opt->help)
> +            if (opt->flags || !opt->help) {
> +                vshError(ctl, _("command '%s' has incorrect alias option"),
> +                         cmd->name);
>                  return -1; /* alias options are tracked by the original name */
> +            }
>              if ((p = strchr(name, '=')) &&
> -                VIR_STRNDUP(name, name, p - name) < 0)
> +                VIR_STRNDUP(name, name, p - name) < 0) {
> +                vshError(ctl, _("allocation failure while checking command '%s'"),
> +                         cmd->name);

I think ^this one can be dropped, if there was an allocation failure, we have
much bigger problems and it's likely it will fail again which vshError will
transitively try to do if you look at vshOutputLogFile for example.

>                  return -1;
> +            }
>              for (j = i + 1; cmd->opts[j].name; j++) {
>                  if (STREQ(name, cmd->opts[j].name) &&
>                      cmd->opts[j].type != VSH_OT_ALIAS)
> @@ -381,21 +397,33 @@ vshCmddefCheckInternals(const vshCmdDef *cmd)
>              if (name != opt->help) {
>                  VIR_FREE(name);
>                  /* If alias comes with value, replacement must not be bool */
> -                if (cmd->opts[j].type == VSH_OT_BOOL)
> +                if (cmd->opts[j].type == VSH_OT_BOOL) {
> +                    vshError(ctl, _("command '%s' has mismatched alias type"),
> +                             cmd->name);
>                      return -1;
> +                }
>              }
> -            if (!cmd->opts[j].name)
> +            if (!cmd->opts[j].name) {
> +                vshError(ctl, _("command '%s' has missing alias option"),
> +                         cmd->name);
>                  return -1; /* alias option must map to a later option name */
> +            }
>          }
>              break;
>          case VSH_OT_ARGV:
> -            if (cmd->opts[i + 1].name)
> +            if (cmd->opts[i + 1].name) {
> +                vshError(ctl, _("command '%s' has option after argv"),
> +                         cmd->name);

The commentary below actually gives me better idea about the error than the
error itself, can we use that text instead?

Reviewed-by: Erik Skultety <eskultet at redhat.com>