[libvirt] virt-admin commands aliases

[Erik Skultety]

On 06/09/16 11:52, Martin Kletzander wrote:
> On Tue, Sep 06, 2016 at 09:02:19AM +0200, Erik Skultety wrote:
>> On 05/09/16 19:48, Daniel P. Berrange wrote:
>>> On Mon, Sep 05, 2016 at 05:37:07PM +0200, Erik Skultety wrote:
>>>> Hi there,
>>>>
>>>> after my presentation at KVM Forum, it was pointed out from the
>>>> audience
>>>> that we might think about doing something about the naming of the
>>>> virt-admin's comands, since there is some sort of inconsistency: srv-
>>>> vs. client- vs. dmn- (not merged yet). When I sent patches to
>>>> upstream I
>>>> already knew that the naming was not optimal, but I didn't come up with
>>>> anything better so I hoped that the reviewer might think of something
>>>> better which unfortunately did not happen.
>>>>
>>>> Anyway, there are multiple options how this can be approached but I'm
>>>> not 100% satisfied with neither of them:
>>>>
>>>> 1) rename the commands completely
>>>> Although clean, obviously this is the non-preferred option because this
>>>> would break any backwards compatibility however, I think there is a
>>>> fair
>>>> chance that people haven't actually started using it yet (although that
>>>> might change between 7.3 and 7.4).
>>>>
>>>> 2) create aliases for non-abbreviated forms of the commands
>>>> That way, srv- would become server- and dmn- would become daemon-.
>>>> However, by doing this we'll end up with 6 almost identical entries in
>>>> the commands structure which might be error-prone once we decide to
>>>> add/create&add a flag to the command primitive, since the flag would
>>>> have to be added both to the alias and to the original (unlikely, but
>>>> possible that someone might forget about that)
>>>>
>>>> 3) abbreviate client- to something like clnt-
>>>> Identical to the above except for the amount of duplicate entries which
>>>> would be reduced to 2
>>>>
>>>> 4) leave it as is if such a consensus is reached and accepted
>>>> I guess this does no need any additional comments.
>>>
>>> I just vote for 4.
>>>
>>> In retrospect it would have been nice to use 'server' instead of
>>> 'srv', but ultimately it isn't a functional problem.  The "solutions"
>>> create extra code and/or inconsitency and/or break back-compat so just
>>> aren't worth it IMHO.
>>>
>>
>> Yeah, for me personally, it was either number 2 or 4 but as you write,
>> both of them suck in their own way and I just could not decide which one
>> sucked less.
>>
>> Thanks for opinions guys, appreciated :)
>>
> 
> I, honestly, would go with #2.  I know you know it, but to recapitulate;
> we already have a wiring in the code for aliases, so if you change:
> 
>    {.name = "srv-list",
>     .handler = cmdSrvList,
>     .opts = NULL,
>     .info = info_srv_list,
>     .flags = 0
>    },
> 
> to:
>    {.name = "server-list",
>     .handler = cmdSrvList,
>     .opts = NULL,
>     .info = info_srv_list,
>     .flags = 0
>    },
>    {.name = "srv-list",
>     .handler = cmdSrvList,
>     .opts = NULL,
>     .info = info_srv_list,
>     .flags = VSH_CMD_FLAG_ALIAS
>    },
> 
> You will have both commands, you will only see the 'server-list' in the
> help and both will work.  The only thing you need to change is, that if
> you add options to the commands that don't have any (yet), you need to
> add the opts_srv_list to both commands.  But that's it.  For
> srv-clients-list (BTW why isn't it named list-clients or client-list)
> that would require no future change.  I know you mentioned that flag
> changes would need to be done in both structures as well, but honestly,
> thre's VSH_CMD_FLAG_NOCONNECT and VSH_CMD_FLAG_ALIAS.  And that's it.
> And I don't think we'll have any new flags anytime soon.  I even dare to
> say never.
> 
> But even if you don't like that, there are ways to mitigate even this
> duplication.  Just make VSH_CMD_FLAG_ALIAS work as VSH_OT_ALIAS.  That
> is just make the following work:
> 
>    {.name = "server-list",
>     .handler = cmdSrvList,
>     .opts = NULL,
>     .info = info_srv_list,
>     .flags = 0
>    },
>    {.name = "srv-list",
>     .flags = VSH_CMD_FLAG_ALIAS,
>     .aliased = "server-list"
>    },
> 

Hmm, well you'll still end up with N more entries (possibly) creating
more unnecessary "noise" but I have to say that I like ^this proposal.
I'll try to play with it, send the patches and we'll see. If the patches
will end up being NACKed, fair enough, time wasted but I think I can
live with that.

> When parsing you'll see that it's an alias and return the opts, flags
> and info for the original aliased command.  While on that, you can make
> it even better; you can get rid of flags completely by just checking if
> 'aliased' is set.  If it is, just search for the original command and
> that's it.
> 
> I'd vote for doing something like this and maybe keeping the aliases for
> daemon and server added for new commands as well (that is when you add
> server-make-me-a-sandwich, you also add srv- alias as well), but that's
> not a hard requirement.

aaand I'm against adding an alias even for brand-new commands, because
having aliases for commands is not a feature, it's an attempt to cover
up the mess someone created, in this case me.

Erik

> 
> Just my €.02,
> Martin
> 
>> Erik
>>
>>> IOW, admit 'srv' sucks but don't change it, and ensure new server
>>> commands continue to use 'srv' for consistency.
>>>
>>> We can of couse use 'daemon-' as prefix for new commands, since we
>>> have not yet released any versions using 'dmn-' as prefix
>>>
>>>
>>> Regards,
>>> Daniel
>>>
>>
>> -- 
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list