[libvirt] [PATCH 2/4] virt-admin: Tweak command parsing logic so that aliases point to new commands

[Michal Privoznik]

On 12.09.2016 10:20, Erik Skultety wrote:
> Change the logic in a way, so that VSH_CMD_FLAG_ALIAS behaves similarly to
> how VSH_OT_ALIAS for command options, i.e. there is no need for code duplication
> for the alias and the aliased command structures. Along with that change,
> switch any existing VSH_CMD_FLAG_ALIAS occurrences to this new format.
> 
> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> ---
>  tools/virsh-nodedev.c | 6 ++----
>  tools/virsh.c         | 3 ++-
>  tools/vsh.c           | 6 ++++++
>  tools/vsh.h           | 1 +
>  4 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c
> index 321f15c..9446664 100644
> --- a/tools/virsh-nodedev.c
> +++ b/tools/virsh-nodedev.c
> @@ -986,10 +986,8 @@ const vshCmdDef nodedevCmds[] = {
>       .flags = 0
>      },
>      {.name = "nodedev-dettach",
> -     .handler = cmdNodeDeviceDetach,
> -     .opts = opts_node_device_detach,
> -     .info = info_node_device_detach,
> -     .flags = VSH_CMD_FLAG_ALIAS
> +     .flags = VSH_CMD_FLAG_ALIAS,
> +     .alias = "nodedev-detach"

I think that if we do this we should just stop using the misspelled
version. I mean, drop it from the man page, do not offer it for
autocompletion.
Also, you need to update vshCmddefCheckInternals() to check whether all
commands passing VSH_CMD_FLAG_ALIAS also set .alias. Otherwise we will
segfault later.

>      },
>      {.name = "nodedev-dumpxml",
>       .handler = cmdNodeDeviceDumpXML,
> diff --git a/tools/virsh.c b/tools/virsh.c
> index cb60edc..60fb02b 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -904,7 +904,8 @@ static const vshCmdDef virshCmds[] = {
>       .handler = cmdSelfTest,
>       .opts = NULL,
>       .info = info_selftest,
> -     .flags = VSH_CMD_FLAG_NOCONNECT | VSH_CMD_FLAG_ALIAS
> +     .flags = VSH_CMD_FLAG_NOCONNECT | VSH_CMD_FLAG_ALIAS,
> +     .alias = "self-test"

So this is just for demonstration purposes? It seems so to me.

>      },
>      {.name = NULL}
>  };
> diff --git a/tools/vsh.c b/tools/vsh.c
> index be6a073..ad3e1c7 100644
> --- a/tools/vsh.c
> +++ b/tools/vsh.c
> @@ -1410,6 +1410,12 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser)
>                      vshError(ctl, _("unknown command: '%s'"), tkdata);
>                      goto syntaxError;   /* ... or ignore this command only? */
>                  }
> +                /* aliases need to be resolved to the actual commands */
> +                if (cmd->flags & VSH_CMD_FLAG_ALIAS) {
> +                    VIR_FREE(tkdata);
> +                    tkdata = vshStrdup(ctl, cmd->alias);

See? If cmd->alias is NULL (because somebody set the _ALIAS flag but
haven't provided .alias), tkdata is gonna be NULL.

> +                    cmd = vshCmddefSearch(tkdata);

Which in turn means, we segfault here.

Also, we must not offer those commands who have .alias for autocompletion.

> +                }
>                  if (vshCmddefOptParse(cmd, &opts_need_arg,
>                                        &opts_required) < 0) {
>                      vshError(ctl,
> diff --git a/tools/vsh.h b/tools/vsh.h
> index e53910f..e383e54 100644
> --- a/tools/vsh.h
> +++ b/tools/vsh.h
> @@ -179,6 +179,7 @@ struct _vshCmdDef {
>      const vshCmdOptDef *opts;   /* definition of command options */
>      const vshCmdInfo *info;     /* details about command */
>      unsigned int flags;         /* bitwise OR of VSH_CMD_FLAG */
> +    const char *alias;          /* name of the aliased command */
>  };
>  
>  /*
> 

Michal