On 03/27/2013 10:49 AM, Peter Krempa wrote: >>>> Here is what I *thought* was the meaning of these options: >>>> >>>> --config - only change the persistent config, but not the >>>> live state. The command should fail for a transient >>>> domain. >>>> >>>> --live - only change the live state, but not the persistent config. >>>> The command should fail for a domain that isn't running. >>>> >>>> --current - useless (really, I mean that) because its meaning is >>>> different >>>> depending on whether or not the domain is running >>> >>> Yes those we understand in the same way. I agree. And these are the new flags, which most commands have (just a few of the older ones haven't added them yet). >>> >>>> >>>> --persistent - deprecated synonym for --config Possibly,... >>> >>> This used to be the synonym for config, but this might be undestood as >>> the option you are missing later on ... ...but I think that it is a great name to use for sane semantics (--config always, and --live as well if the domain is running). I'm not sure how many old commands that exposed --persistent have anything other than the sane semantics, though. >>> >>>> >>>> no option - also useless because it means the same thing as --current >>>> >>>> What's missing: a way to say "change both live state and persistent >>>> config as appropriate" >>> >>> Well, this patch would abuse the --persistent flag for this purpose. >>> It adds _CONFIG always and _LIVE in case the domain is up. Indeed, that is the sanest semantics. I don't think we can make it default, unless maybe we add a command to set the default effect, make ALL virsh code that uses --config/--live call into the common routine to learn what default to use when there is no flag, and make a persistent .ini-style file that remembers the default setting you'd like virsh to have. >>>> >>>> After discussing it on the list, in the name of consistency I actually >>>> very reluctantly implemented this same logic for virsh net-update even >>>> though I thought it was terrible. Did I get it wrong? >>> >>> No, you've got it correct. Well, except for --persistent but I'm less >>> sure about that than you. >>> >>>> >>>> So what is the *real* meaning of each of these options? (and are you >>>> sure you're not changing the meaning of any of them?) Well, there _have_ been instances where the underlying qemu_driver.c has had broken semantics in some releases, where we fix it later (such as cases of --config not actually changing the config settings, or --config --live together only changing one instead of both), so maybe we argue that any use of --persistent with anything other than sane semantics is just a (long-standing) bug? >>> >>> Except for --persistent, the meaning seems to be well defined now and >>> used appropriately in new places. The problem is with the legacy API's >>> that didn't take up on this approach yet. >> >> I'm concerned about changing the meaning of any existing option, which >> creates trouble for any existing script that uses the options. An >> example of the effects of this can be seen in this thread: >> >> https://www.redhat.com/archives/libvirt-users/2013-March/msg00108.html Yeah, anywhere we have an old API that used --persistent, I'm a bit reluctant to accidentally change that flag, even if changing it to sane semantics makes the most sense. We're almost stuck with needing to invent yet another flag name, that we can use universally without having to worry about breaking back-compat. >> Although it's not listed in a .h file anywhere, commandline options >> *are* part of a public API, and shouldn't be changed after they've been >> in an official release (unless they were broken to the point that nobody >> would have been able to use them anyway). Well, being inconsistent with all other commands does sound kind of like being broken to the point that no one knows which ones to use. > > Looking through really old versions of virsh I found that the > --persistent flag was used in the meaning "make this live change also > affect next boot" afterwards we decided to shed --persistent for > --config with a pure rename. this change introduced the discrepancy > between the original use of --persistent and the new meaning of --config. > > Right now, I don't feel like introducing yet another set of domain > modification scope flags and I'd rather fix all the improper uses of > --config to be uniform and try to preserve the original meaning of the > --persistent flag as it seems useful to me. I tend to agree - the old semantics of --persistent really WERE sane, and we broke things when we tried to make --persistent an alias of --config. > > Summary of the changed behavior: > 1.) --persistent behaves like it has since it was introduced > 2.) --persistent is documented again > 3.) --config no longer influences the live guest (it isn't doing so in > other places, this behavior is known from other places and (sort-of) > documented) > 4.) the user is able to force use of the new API that has better > specified behavior as in the case of the api without flags. Yes, I would be able to live with such a change. > > In case we decide against this slight change (on running domain --config > won't longer influence the running state) my approach to fix the user > confusion will be very different. I'll leave the code in place as-is and > I will only document that the --config flag behaves differently on these > API's compared to other places. This feels a bit worse; making --config a synonym for --persistent was probably the wrong way to go about it. > > The reason I started doing this is user confusion because of the > different meaning and bad docs. I guess we can still document (for any option where meanings might have changed) that the user needs to be aware that older hypervisors might act incorrectly on a --config request. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list