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

[Michal Privoznik]

On 13.09.2016 16:11, Erik Skultety wrote:
> 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.

Well, I can imagine having the check there (even the comment just above
the function suggests it is about command internals - not solely about
options), but I don't care that much about placement of the check. We
just need to make sure that the place is hit during our 'make check' phase.

> 
>> 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.

I think you've misunderstood. The same way we currently have
vshCmddefCheckInternals() <- vshCmddefOptParse() <- vshCmddefHelp() <-
cmdSelfTest() (so that any problem you are describing above is hit in
our test suite), the same way we would hit the error if alias is set
just in flags. I see no problem with that.

> 
> 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.

Right, you will be notified during 'make check' phase right away,
because the test would fail. But without the checks I'm advocating for
no error would be visible.

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

Except it won't fail in test phase for anything missing the .alias but
'self-test' command.

Michal