Re: [libvirt] [PATCH 5/6] tools: fix the wrong check when use virsh setvcpus --maximum

2015-03-24 Thread Pavel Hrdina 
On Tue, Mar 24, 2015 at 05:34:31PM +0100, Peter Krempa wrote:
 On Fri, Mar 20, 2015 at 15:39:03 +0100, Pavel Hrdina wrote:
  From: Luyao Huang lhu...@redhat.com
  
  We will ignore --maximum option when only use setvcpus with
  this option, like this (this error is another issue):
  
   # virsh setvcpus test3 --maximum 10
   error: Failed to create controller cpu for group: No such file or directory
  
  this is because we do not set it in flags before we check if there is
  a flags set.
  
  Refactor these code and fix the logic.
  
  Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1204033
  
  Signed-off-by: Luyao Huang lhu...@redhat.com
  Signed-off-by: Pavel Hrdina phrd...@redhat.com
  ---
   tools/virsh-domain.c | 30 ++
   1 file changed, 6 insertions(+), 24 deletions(-)
  
  diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
  index 1d8225c..9430ad9 100644
  --- a/tools/virsh-domain.c
  +++ b/tools/virsh-domain.c
  @@ -6735,6 +6735,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
   VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
   VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
   VSH_EXCLUSIVE_OPTIONS_VAR(guest, config);
  +VSH_EXCLUSIVE_OPTIONS_VAR(guest, maximum);
  +VSH_EXCLUSIVE_OPTIONS_VAR(config, maximum);
 
 As Yanbing pointed out, you want to make live and maximum exclusive.
 
 Additionally this changes semantics, as currently --maximum was possible
 if and only if --config was specified, which would make it exclusive
 with --current too. This is also implied in the man page.
 
 We have the following options:
 
 1) Make --maximum imply --config and document that properly
 2) Make --maximum mutualy exclusive with --current too
 3) Allow --maximum and --current and document that it will fail for
online domains
 
 I'm fine with either of those options

There is also 4th option: specify that maximum requires config instead of make
it mutually exclusive with current.  This flag requirements are used by some of
the other libvirt APIs.  I'll create another set of macros to tell that some
flags are required and send v2.

Thanks for review.

Pavel

 
 Peter


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

Re: [libvirt] [PATCH 5/6] tools: fix the wrong check when use virsh setvcpus --maximum

2015-03-24 Thread Peter Krempa 
On Fri, Mar 20, 2015 at 15:39:03 +0100, Pavel Hrdina wrote:
 From: Luyao Huang lhu...@redhat.com
 
 We will ignore --maximum option when only use setvcpus with
 this option, like this (this error is another issue):
 
  # virsh setvcpus test3 --maximum 10
  error: Failed to create controller cpu for group: No such file or directory
 
 this is because we do not set it in flags before we check if there is
 a flags set.
 
 Refactor these code and fix the logic.
 
 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1204033
 
 Signed-off-by: Luyao Huang lhu...@redhat.com
 Signed-off-by: Pavel Hrdina phrd...@redhat.com
 ---
  tools/virsh-domain.c | 30 ++
  1 file changed, 6 insertions(+), 24 deletions(-)
 
 diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
 index 1d8225c..9430ad9 100644
 --- a/tools/virsh-domain.c
 +++ b/tools/virsh-domain.c
 @@ -6735,6 +6735,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
  VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
  VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
  VSH_EXCLUSIVE_OPTIONS_VAR(guest, config);
 +VSH_EXCLUSIVE_OPTIONS_VAR(guest, maximum);
 +VSH_EXCLUSIVE_OPTIONS_VAR(config, maximum);

As Yanbing pointed out, you want to make live and maximum exclusive.

Additionally this changes semantics, as currently --maximum was possible
if and only if --config was specified, which would make it exclusive
with --current too. This is also implied in the man page.

We have the following options:

1) Make --maximum imply --config and document that properly
2) Make --maximum mutualy exclusive with --current too
3) Allow --maximum and --current and document that it will fail for
   online domains

I'm fine with either of those options

Peter


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 5/6] tools: fix the wrong check when use virsh setvcpus --maximum

2015-03-23 Thread Pavel Hrdina 
On Mon, Mar 23, 2015 at 11:33:50AM +0800, Yanbing Du wrote:
 
 
 On 03/20/2015 10:39 PM, Pavel Hrdina wrote:
  From: Luyao Huang lhu...@redhat.com
 
  We will ignore --maximum option when only use setvcpus with
  this option, like this (this error is another issue):
 
# virsh setvcpus test3 --maximum 10
error: Failed to create controller cpu for group: No such file or 
  directory
 
  this is because we do not set it in flags before we check if there is
  a flags set.
 
  Refactor these code and fix the logic.
 
  Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1204033
 
  Signed-off-by: Luyao Huang lhu...@redhat.com
  Signed-off-by: Pavel Hrdina phrd...@redhat.com
  ---
tools/virsh-domain.c | 30 ++
1 file changed, 6 insertions(+), 24 deletions(-)
 
  diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
  index 1d8225c..9430ad9 100644
  --- a/tools/virsh-domain.c
  +++ b/tools/virsh-domain.c
  @@ -6735,6 +6735,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
VSH_EXCLUSIVE_OPTIONS_VAR(guest, config);
  +VSH_EXCLUSIVE_OPTIONS_VAR(guest, maximum);
  +VSH_EXCLUSIVE_OPTIONS_VAR(config, maximum);
 
 'maximum' should be used with 'config', and 'live' and 'maximum' are 
 mutually exclusive

Yes, you're right, I've definitely meant live instead of config. Good catch.

Pavel

 
 
if (config)
flags |= VIR_DOMAIN_AFFECT_CONFIG;
  @@ -6742,9 +6744,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
flags |= VIR_DOMAIN_AFFECT_LIVE;
if (guest)
flags |= VIR_DOMAIN_VCPU_GUEST;
  -/* none of the options were specified */
  -if (!current  flags == 0)
  -flags = -1;
  +if (maximum)
  +flags |= VIR_DOMAIN_VCPU_MAXIMUM;
 
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
return false;
  @@ -6754,30 +6755,11 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
goto cleanup;
}
 
  -if (flags == -1) {
  +/* none of the options were specified */
  +if (!current  flags == 0) {
if (virDomainSetVcpus(dom, count) != 0)
goto cleanup;
} else {
  -/* If the --maximum flag was given, we need to ensure only the
  -   --config flag is in effect as well */
  -if (maximum) {
  -vshDebug(ctl, VSH_ERR_DEBUG, --maximum flag was given\n);
  -
  -flags |= VIR_DOMAIN_VCPU_MAXIMUM;
  -
  -/* If neither the --config nor --live flags were given, OR
  -   if just the --live flag was given, we need to error out
  -   warning the user that the --maximum flag can only be used
  -   with the --config flag */
  -if (live || !config) {
  -
  -/* Warn the user about the invalid flag combination */
  -vshError(ctl, _(--maximum must be used with --config 
  only));
  -goto cleanup;
  -}
  -}
  -
  -/* Apply the virtual cpu changes */
if (virDomainSetVcpusFlags(dom, count, flags)  0)
goto cleanup;
}
 
 
 -- 
 Regards,
 Yanbing Du
 
 --
 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 5/6] tools: fix the wrong check when use virsh setvcpus --maximum

2015-03-22 Thread Yanbing Du 



On 03/20/2015 10:39 PM, Pavel Hrdina wrote:

From: Luyao Huang lhu...@redhat.com

We will ignore --maximum option when only use setvcpus with
this option, like this (this error is another issue):

  # virsh setvcpus test3 --maximum 10
  error: Failed to create controller cpu for group: No such file or directory

this is because we do not set it in flags before we check if there is
a flags set.

Refactor these code and fix the logic.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1204033

Signed-off-by: Luyao Huang lhu...@redhat.com
Signed-off-by: Pavel Hrdina phrd...@redhat.com
---
  tools/virsh-domain.c | 30 ++
  1 file changed, 6 insertions(+), 24 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 1d8225c..9430ad9 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6735,6 +6735,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
  VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
  VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
  VSH_EXCLUSIVE_OPTIONS_VAR(guest, config);
+VSH_EXCLUSIVE_OPTIONS_VAR(guest, maximum);
+VSH_EXCLUSIVE_OPTIONS_VAR(config, maximum);


'maximum' should be used with 'config', and 'live' and 'maximum' are 
mutually exclusive




  if (config)
  flags |= VIR_DOMAIN_AFFECT_CONFIG;
@@ -6742,9 +6744,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
  flags |= VIR_DOMAIN_AFFECT_LIVE;
  if (guest)
  flags |= VIR_DOMAIN_VCPU_GUEST;
-/* none of the options were specified */
-if (!current  flags == 0)
-flags = -1;
+if (maximum)
+flags |= VIR_DOMAIN_VCPU_MAXIMUM;

  if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
  return false;
@@ -6754,30 +6755,11 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
  goto cleanup;
  }

-if (flags == -1) {
+/* none of the options were specified */
+if (!current  flags == 0) {
  if (virDomainSetVcpus(dom, count) != 0)
  goto cleanup;
  } else {
-/* If the --maximum flag was given, we need to ensure only the
-   --config flag is in effect as well */
-if (maximum) {
-vshDebug(ctl, VSH_ERR_DEBUG, --maximum flag was given\n);
-
-flags |= VIR_DOMAIN_VCPU_MAXIMUM;
-
-/* If neither the --config nor --live flags were given, OR
-   if just the --live flag was given, we need to error out
-   warning the user that the --maximum flag can only be used
-   with the --config flag */
-if (live || !config) {
-
-/* Warn the user about the invalid flag combination */
-vshError(ctl, _(--maximum must be used with --config only));
-goto cleanup;
-}
-}
-
-/* Apply the virtual cpu changes */
  if (virDomainSetVcpusFlags(dom, count, flags)  0)
  goto cleanup;
  }



--
Regards,
Yanbing Du

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

[libvirt] [PATCH 5/6] tools: fix the wrong check when use virsh setvcpus --maximum

2015-03-20 Thread Pavel Hrdina 
From: Luyao Huang lhu...@redhat.com

We will ignore --maximum option when only use setvcpus with
this option, like this (this error is another issue):

 # virsh setvcpus test3 --maximum 10
 error: Failed to create controller cpu for group: No such file or directory

this is because we do not set it in flags before we check if there is
a flags set.

Refactor these code and fix the logic.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1204033

Signed-off-by: Luyao Huang lhu...@redhat.com
Signed-off-by: Pavel Hrdina phrd...@redhat.com
---
 tools/virsh-domain.c | 30 ++
 1 file changed, 6 insertions(+), 24 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 1d8225c..9430ad9 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6735,6 +6735,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
 VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
 VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
 VSH_EXCLUSIVE_OPTIONS_VAR(guest, config);
+VSH_EXCLUSIVE_OPTIONS_VAR(guest, maximum);
+VSH_EXCLUSIVE_OPTIONS_VAR(config, maximum);
 
 if (config)
 flags |= VIR_DOMAIN_AFFECT_CONFIG;
@@ -6742,9 +6744,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
 flags |= VIR_DOMAIN_AFFECT_LIVE;
 if (guest)
 flags |= VIR_DOMAIN_VCPU_GUEST;
-/* none of the options were specified */
-if (!current  flags == 0)
-flags = -1;
+if (maximum)
+flags |= VIR_DOMAIN_VCPU_MAXIMUM;
 
 if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
 return false;
@@ -6754,30 +6755,11 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
 goto cleanup;
 }
 
-if (flags == -1) {
+/* none of the options were specified */
+if (!current  flags == 0) {
 if (virDomainSetVcpus(dom, count) != 0)
 goto cleanup;
 } else {
-/* If the --maximum flag was given, we need to ensure only the
-   --config flag is in effect as well */
-if (maximum) {
-vshDebug(ctl, VSH_ERR_DEBUG, --maximum flag was given\n);
-
-flags |= VIR_DOMAIN_VCPU_MAXIMUM;
-
-/* If neither the --config nor --live flags were given, OR
-   if just the --live flag was given, we need to error out
-   warning the user that the --maximum flag can only be used
-   with the --config flag */
-if (live || !config) {
-
-/* Warn the user about the invalid flag combination */
-vshError(ctl, _(--maximum must be used with --config only));
-goto cleanup;
-}
-}
-
-/* Apply the virtual cpu changes */
 if (virDomainSetVcpusFlags(dom, count, flags)  0)
 goto cleanup;
 }
-- 
2.0.5

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