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

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(a)redhat.com&gt; >> --- >> 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.