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

2013-03-27 Thread Martin Kletzander 
On 03/21/2013 05: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

s/bood/boot/

 true if the machine is running.
 
 This patch adds the full --live, --config, --current infrastructure and
 tweaks stuff to correctly support the obsolete --persistent flag.
 ---
  tools/virsh-domain.c | 41 ++---
  tools/virsh.pod  | 21 ++---
  2 files changed, 44 insertions(+), 18 deletions(-)
 
[...]
 @@ -9267,7 +9275,22 @@ cmdUpdateDevice(vshControl *ctl, const vshCmd *cmd)
  const char *from = NULL;
  char *buffer = NULL;
  bool ret = false;
 -unsigned int flags;
 +unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
 +bool current = vshCommandOptBool(cmd, current);
 +bool config = vshCommandOptBool(cmd, config);
 +bool live = vshCommandOptBool(cmd, live);
 +bool persistent = vshCommandOptBool(cmd, persistent);
 +
 +VSH_EXCLUSIVE_OPTIONS_VAR(persistent, live);

Previously, --persistent --live was working and made sense as well, but
you are disallowing that now.  With that in mind, why do you allow
--persistent --config then?

From my POV, I'd leave --persistent --live --config allowed.  The change
in what --persistent does (affects also running domain, but it didn't
before), is OK with me.

ACK after 1.0.4 with that one line removed.

Martin

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2013-03-27 Thread Laine Stump 
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.

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

--persistent - deprecated synonym for --config

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

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?

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?)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2013-03-27 Thread Peter Krempa 

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.




--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2013-03-27 Thread Laine Stump 
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).

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2013-03-27 Thread Peter Krempa 

On 03/27/13 17:25, Laine Stump wrote:

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).


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.


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.


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.


The reason I started doing this is user confusion because of the 
different meaning and bad docs.


I don't really care that

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

2013-03-21 Thread Peter Krempa 
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.

This patch adds the full --live, --config, --current infrastructure and
tweaks stuff to correctly support the obsolete --persistent flag.
---
 tools/virsh-domain.c | 41 ++---
 tools/virsh.pod  | 21 ++---
 2 files changed, 44 insertions(+), 18 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 68df01e..6741837 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -9246,13 +9246,21 @@ static const vshCmdOptDef opts_update_device[] = {
  .help = N_(XML file)
 },
 {.name = persistent,
- .type = VSH_OT_ALIAS,
- .help = config
+ .type = VSH_OT_BOOL,
+ .help = N_(make live change persistent)
 },
 {.name = config,
  .type = VSH_OT_BOOL,
  .help = N_(affect next boot)
 },
+{.name = live,
+ .type = VSH_OT_BOOL,
+ .help = N_(affect running domain)
+},
+{.name = current,
+ .type = VSH_OT_BOOL,
+ .help = N_(affect current domain)
+},
 {.name = force,
  .type = VSH_OT_BOOL,
  .help = N_(force device update)
@@ -9267,7 +9275,22 @@ cmdUpdateDevice(vshControl *ctl, const vshCmd *cmd)
 const char *from = NULL;
 char *buffer = NULL;
 bool ret = false;
-unsigned int flags;
+unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
+bool current = vshCommandOptBool(cmd, current);
+bool config = vshCommandOptBool(cmd, config);
+bool live = vshCommandOptBool(cmd, live);
+bool persistent = vshCommandOptBool(cmd, persistent);
+
+VSH_EXCLUSIVE_OPTIONS_VAR(persistent, live);
+VSH_EXCLUSIVE_OPTIONS_VAR(persistent, current);
+
+VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
+VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
+
+if (config || persistent)
+flags |= VIR_DOMAIN_AFFECT_CONFIG;
+if (live)
+flags |= VIR_DOMAIN_AFFECT_LIVE;

 if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
 return false;
@@ -9275,19 +9298,15 @@ cmdUpdateDevice(vshControl *ctl, const vshCmd *cmd)
 if (vshCommandOptStringReq(ctl, cmd, file, from)  0)
 goto cleanup;

+if (persistent 
+virDomainIsActive(dom) == 1)
+flags |= VIR_DOMAIN_AFFECT_LIVE;
+
 if (virFileReadAll(from, VSH_MAX_XML_FILE, buffer)  0) {
 vshReportError(ctl);
 goto cleanup;
 }

-if (vshCommandOptBool(cmd, config)) {
-flags = VIR_DOMAIN_AFFECT_CONFIG;
-if (virDomainIsActive(dom) == 1)
-   flags |= VIR_DOMAIN_AFFECT_LIVE;
-} else {
-flags = VIR_DOMAIN_AFFECT_LIVE;
-}
-
 if (vshCommandOptBool(cmd, force))
 flags |= VIR_DOMAIN_DEVICE_MODIFY_FORCE;

diff --git a/tools/virsh.pod b/tools/virsh.pod
index b5e632e..a9e8c65 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -1887,18 +1887,25 @@ If I--config is specified, alter persistent 
configuration, effect observed
 on next boot, for compatibility purposes, I--persistent is alias of
 I--config.

-=item Bupdate-device Idomain Ifile [I--config] [I--force]
+=item Bupdate-device Idomain Ifile [I--force]
+[[[I--live] [I--config] | [I--current]] | [I--persistent]]

 Update the characteristics of a device associated with Idomain,
-based on the device definition in an XML Ifile.  If the I--config
-option is used, the changes will take affect the next time libvirt
-starts the domain.  For compatibility purposes, I--persistent is
-alias of I--config.  The I--force option can be used to force
-device update, e.g., to eject a CD-ROM even if it is locked/mounted in
-the domain. See the documentation at
+based on the device definition in an XML Ifile.  The I--force option
+can be used to force device update, e.g., to eject a CD-ROM even if it is
+locked/mounted in the domain. See the documentation at
 Lhttp://libvirt.org/formatdomain.html#elementsDevices to learn about
 libvirt XML format for a device.

+If I--live is specified, affect a running domain.
+If I--config is specified, affect the next startup of a persistent domain.
+If I--current is specified, affect the current domain state.
+Both I--live and I--config flags may be given, but I--current is
+exclusive. Not specifying any flag is the same as specifying I--current.
+
+For compatibility purposes, I--persistent behaves like I--config for
+an offline domain, and like I--live I--config for a running domain.
+
 =item Bchange-media Idomain Ipath [I--eject] [I--insert]
 [I--update] [Isource] [I--force] [[I--live] [I--config] | 
[I--current]]

-- 
1.8.1.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list