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