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: Oh, I completely missed that one, I would never expected that we actually document our (so far) only alias in the man page... ^^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. 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. 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