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

[Erik Skultety]

On 13/09/16 14:30, Michal Privoznik wrote:
> 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.

Oh, I completely missed that one, I would never expected that we
actually document our (so far) only alias in the man page...

> Also, you need to update vshCmddefCheckInternals() to check whether all

^^not the correct spot to check, that is intended just for the command
opts (therefore internals) not for the command itself, so the command
structure should be checked in the vshCommandParse() function.

> commands passing VSH_CMD_FLAG_ALIAS also set .alias. Otherwise we will
> segfault later.

Aand I disagree, if we return just an error code, executing such a
command from virsh is visually a noop ==> so no-go. If we also error out
what purpose would the message serve to the user? Let's say a message
like "missing .alias element", it's our internal static structure and
such an error would have absolutely no meaning (and no value) to the
user because the error would not be caused by a certain combination of
user inputs, exposing a flaw in our logic, rather it is an incorrect
hard-coded setting in the binary.

Additionally, if we went your way, we would have to add a check
preventing a segfault for any missing element in the command structure
for every command, let's say '.opts' element, you'll experience a
segfault in that case as well, but since commit 920ab8bd you'll be
notified about this fact by the self-test command - see my further
comments below.

> 
>>      },
>>      {.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.

Well, omitting the .alias element would segfault as you pointed out, but
adding it here, although pointing to itself, was imho easier than adding
a check if an aliased command does have the .handler element filled in,
so it would be used instead of looking at the .alias element. This
command is also used for testing purposes only (thus it's hidden) and
this is actually the *right* spot to have a check, if an aliased command
also has the '.alias' element filled properly, because this test is
supposed to make us aware of such mistakes when someone adds a command,
but forgets to fill out certain structure element.

Regards,
Erik

> 
>>      },
>>      {.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
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>