[libvirt] virt-admin commands aliases
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. Thanks for any ideas. Erik
On Mon, 2016-09-05 at 17:37 +0200, Erik Skultety wrote:
Hi there, after my presentation at KVM Forum, it was pointed out from the audience
Disclaimer: I'm the audience in question :)
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.
I'm sorry for not paying enough attention at the time and for not raising the issue while the series was still undergoing review. If I had, we wouldn't need to have this discussion :(
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).
This is very tempting, but I'm not sure we can actually get away with it.
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)
This seems like a fair solution, but note that I haven't looked at the patch implementing it yet - I might change my mind once I did 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
I feel very strongly against this option. Not only "clnt" is four letters long, as opposed to the three letters of both "srv" and "dmn", I also think both "clnt" and "dmn" are absolutely unsightly and have no place in a private API - much less in a public one. Not to mention that we will probably end up adding more entitites, that will have to be shortened in the same way, quite possibly with suboptimal results.
4) leave it as is if such a consensus is reached and accepted I guess this does no need any additional comments.
I feel *even more strongly* about this one :) With all I've written above agains "clnt" and "dmn", I'd rather have those instead of the current inconsistent naming convention. -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Sep 05, 2016 at 07:40:30PM +0200, Andrea Bolognani wrote:
On Mon, 2016-09-05 at 17:37 +0200, Erik Skultety wrote:
Hi there, after my presentation at KVM Forum, it was pointed out from the audience
Disclaimer: I'm the audience in question :)
There's always one trouble-maker in the audience ;-P Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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. 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 -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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 :) 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
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" }, 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. 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (4)
-
Andrea Bolognani -
Daniel P. Berrange -
Erik Skultety -
Martin Kletzander