Re: [libvirt] [PATCH 2/5] virsh: Fix semantics of --config for "update-device" command

On 03/27/2013 11:39 AM, Peter Krempa wrote: > On 03/27/13 16:22, Laine Stump wrote: >> On 03/21/2013 12:42 PM, Peter Krempa wrote: >>> The man page states that with --config the next boot is affected. This >>> can be understood as if _only_ the next bood was affected. This isn't >>> true if the machine is running. >> >> Are you certain of that? My understanding was that when --config was >> specified, *only* the persistent config should be changed, but not the >> live state, regardless of whether or not the domain is running. >> Otherwise, there is no way to change just the persistent config of a >> running domain. > > Yes, that's why I'm doing this. Before this patch --config meant > "modify the persistent config along with the live change" and there > was no way to use this virsh command to change the next-boot config if > the domain was running. > >> >> Or is your explanation incorrect, and the code correct? >> >> 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. > >> >> --persistent - deprecated synonym for --config > > This used to be the synonym for config, but this might be undestood as > the option you are missing later on ... > >> >> 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. >> >> 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?) > > 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 In short, netfilter changed the meaning of the iptables commandline --ctdir option which, although it was incorrect, had already been in release versions for 2 years. By changing this option, with no way to detect presence/absence of the change short of this very heroic effort by stefanb: https://www.redhat.com/archives/libvir-list/2013-March/msg01403.html they effectively made it unusable except in cases of packages that are closely tied to a specific version of kernel/netfilter. 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).