Re: [libvirt] [PATCH] virsh: report error if vcpu number exceed the guest maxvcpu number

2015-07-02 Thread Pavel Hrdina 
On Thu, Jul 02, 2015 at 10:44:36AM +0800, lhuang wrote:
 
 On 07/02/2015 04:29 AM, John Ferlan wrote:
 
  On 06/28/2015 10:10 PM, Luyao Huang wrote:
  If usr pass a vcpu which exceed guest maxvcpu number, virsh client
  will only output an header like this:
 
# virsh vcpupin test3 1000
VCPU: CPU Affinity
--
 
  After this patch:
 
# virsh vcpupin test3 1000
error: vcpu 1000 is out of range of cpu count 2
 
  Signed-off-by: Luyao Huang lhu...@redhat.com
  ---
tools/virsh-domain.c | 5 +
1 file changed, 5 insertions(+)
 
  Seemed odd that this check wasn't there - so I did some digging...
 
  Pavel's commit id '81dd81e' removed a check that seems to be what is
  desired in this path (or was there before his change):
 
   if (vcpu = info.nrVirtCpu) {
   vshError(ctl, %s, _(vcpupin: vCPU index out of range.));
   goto cleanup;
   return false;
   }
 

Looking at the patches, this was removed accidentally.

  As part of this commit, you'll note there was a test change in
  tests/vcpupin:
 
# An out-of-range vCPU number deserves a diagnostic, too.
$abs_top_builddir/tools/virsh --connect test:///default vcpupin test
  100 0,1  out 21
test $? = 1 || fail=1
cat \EOF  exp || fail=1
  -error: vcpupin: vCPU index out of range.
  +error: invalid argument: requested vcpu is higher than allocated vcpus
 

This error change is only while setting vcpu pinning.

 
  Searching on their error message lands one in test_driver.c/
  testDomainPinVcpu().  So something specific for a set path, but the path
  you're hitting is the get path.
 
 Yes, i noticed this and checked if need introduce a test or change the 
 old test, but
 i found test driver not support get vcpupin.
 
 
  FWIW: If a similar test was run on my system I get:
  # virsh vcpupin $dom 1000 0,1
  error: invalid argument: vcpu 1000 is out of range of live cpu count 2
 
  #
 
 
  So, if I understand everything that was done - then while your change is
  mostly correct, I think you could perhaps message the error similar to
  the vshCPUCountCollect failure (see the attached patch)
 
 I saw the attached patch, but there is some problem about check the flag 
 (actually
 i had a try with check flags and output a better error before).
 
 If check flags like vshCPUCountCollect failure, there will be a problem 
 when do not
 pass flag or just pass current flag to vcpupin, we will get error like 
 this (pass a too big vcpu
 number):
 
 # virsh list;virsh vcpupin rhel7.0 1000 --current
   IdName   State
 
   3 rhel7.0running
 
 error: vcpu 1000 is out of range of persistent cpu count 4
 
 In this case, we output persistent instead of live, this is because 
 vshCPUCountCollect()
 cannot return certain flags (although there is a description say 
 Returns the count of vCPUs for a
 domain and certain flags). So we need more check for current flags, 
 maybe like this :
 
 diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
 index 27d62e9..334fd3a 100644
 --- a/tools/virsh-domain.c
 +++ b/tools/virsh-domain.c
 @@ -6497,6 +6497,19 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
   goto cleanup;
   }
 
 +if (got_vcpu  vcpu = ncpus) {
 +if (flags  VIR_DOMAIN_AFFECT_LIVE ||
 +(flags  VIR_DOMAIN_AFFECT_CURRENT  
 virDomainIsActive(dom) == 1))
 +vshError(ctl,
 + _(vcpu %d is out of range of live cpu count %d),
 + vcpu, ncpus);
 +else
 +vshError(ctl,
 + _(vcpu %d is out of range of persistent cpu 
 count %d),
 + vcpu, ncpus);
 +goto cleanup;
 +}
 +
   cpumaplen = VIR_CPU_MAPLEN(maxcpu);
   cpumap = vshMalloc(ctl, ncpus * cpumaplen);
   if ((ncpus = virDomainGetVcpuPinInfo(dom, ncpus, cpumap,
 

This modification is much better and correspond to the error messages while
setting the vcpu pinning.

 
  Before I make that change for you - hopefully Pavel can take a look as
  well to be sure I haven't missed something.
 
  With any luck we this could be addressed before the 1.2.17 release, but
  if not since it's been a regression since 1.2.13 and no one's noticed,
  then another release probably won't hurt.
 
 Right, if we can fix it in 1.2.17, it will be better :)
 
 Thanks a lot for your help and review.
 

ACK to the patch than John updated and proposed.

  John
 
 
 Luyao
 
 --
 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] virsh: report error if vcpu number exceed the guest maxvcpu number

2015-07-02 Thread John Ferlan 


On 07/02/2015 05:46 AM, Pavel Hrdina wrote:

...


 diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
 index 27d62e9..334fd3a 100644
 --- a/tools/virsh-domain.c
 +++ b/tools/virsh-domain.c
 @@ -6497,6 +6497,19 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
   goto cleanup;
   }

 +if (got_vcpu  vcpu = ncpus) {
 +if (flags  VIR_DOMAIN_AFFECT_LIVE ||
 +(flags  VIR_DOMAIN_AFFECT_CURRENT  
 virDomainIsActive(dom) == 1))
 +vshError(ctl,
 + _(vcpu %d is out of range of live cpu count %d),
 + vcpu, ncpus);
 +else
 +vshError(ctl,
 + _(vcpu %d is out of range of persistent cpu 
 count %d),
 + vcpu, ncpus);
 +goto cleanup;
 +}
 +
   cpumaplen = VIR_CPU_MAPLEN(maxcpu);
   cpumap = vshMalloc(ctl, ncpus * cpumaplen);
   if ((ncpus = virDomainGetVcpuPinInfo(dom, ncpus, cpumap,

 
 This modification is much better and correspond to the error messages while
 setting the vcpu pinning.
 

I just pushed this now - it'd need a bz for a backport (ahem) since
1.2.17 was cut before the push...


commit 848ab685f74afae102e265108518095942ecb293
Author: Luyao Huang lhu...@redhat.com
Date:   Mon Jun 29 10:10:15 2015 +0800

virsh: report error if vcpu number exceed the guest maxvcpu number

John

 Before I make that change for you - hopefully Pavel can take a look as
 well to be sure I haven't missed something.

 With any luck we this could be addressed before the 1.2.17 release, but
 if not since it's been a regression since 1.2.13 and no one's noticed,
 then another release probably won't hurt.

 Right, if we can fix it in 1.2.17, it will be better :)

 Thanks a lot for your help and review.

 
 ACK to the patch than John updated and proposed.
 

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

Re: [libvirt] [PATCH] virsh: report error if vcpu number exceed the guest maxvcpu number

2015-07-02 Thread lhuang 


On 07/02/2015 06:28 PM, John Ferlan wrote:


On 07/02/2015 05:46 AM, Pavel Hrdina wrote:

...


diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 27d62e9..334fd3a 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6497,6 +6497,19 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
   goto cleanup;
   }

+if (got_vcpu  vcpu = ncpus) {
+if (flags  VIR_DOMAIN_AFFECT_LIVE ||
+(flags  VIR_DOMAIN_AFFECT_CURRENT 
virDomainIsActive(dom) == 1))
+vshError(ctl,
+ _(vcpu %d is out of range of live cpu count %d),
+ vcpu, ncpus);
+else
+vshError(ctl,
+ _(vcpu %d is out of range of persistent cpu
count %d),
+ vcpu, ncpus);
+goto cleanup;
+}
+
   cpumaplen = VIR_CPU_MAPLEN(maxcpu);
   cpumap = vshMalloc(ctl, ncpus * cpumaplen);
   if ((ncpus = virDomainGetVcpuPinInfo(dom, ncpus, cpumap,


This modification is much better and correspond to the error messages while
setting the vcpu pinning.


I just pushed this now - it'd need a bz for a backport (ahem) since
1.2.17 was cut before the push...


commit 848ab685f74afae102e265108518095942ecb293
Author: Luyao Huang lhu...@redhat.com
Date:   Mon Jun 29 10:10:15 2015 +0800

 virsh: report error if vcpu number exceed the guest maxvcpu number


Okay, i will help to find a bz for this patch.

thanks a lot for your help and review, Pavel and John :)


John


Luyao


Before I make that change for you - hopefully Pavel can take a look as
well to be sure I haven't missed something.

With any luck we this could be addressed before the 1.2.17 release, but
if not since it's been a regression since 1.2.13 and no one's noticed,
then another release probably won't hurt.

Right, if we can fix it in 1.2.17, it will be better :)

Thanks a lot for your help and review.


ACK to the patch than John updated and proposed.



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

Re: [libvirt] [PATCH] virsh: report error if vcpu number exceed the guest maxvcpu number

2015-07-01 Thread John Ferlan 


On 06/28/2015 10:10 PM, Luyao Huang wrote:
 If usr pass a vcpu which exceed guest maxvcpu number, virsh client
 will only output an header like this:
 
  # virsh vcpupin test3 1000
  VCPU: CPU Affinity
  --
 
 After this patch:
 
  # virsh vcpupin test3 1000
  error: vcpu 1000 is out of range of cpu count 2
 
 Signed-off-by: Luyao Huang lhu...@redhat.com
 ---
  tools/virsh-domain.c | 5 +
  1 file changed, 5 insertions(+)
 

Seemed odd that this check wasn't there - so I did some digging...

Pavel's commit id '81dd81e' removed a check that seems to be what is
desired in this path (or was there before his change):

if (vcpu = info.nrVirtCpu) {
vshError(ctl, %s, _(vcpupin: vCPU index out of range.));
goto cleanup;
return false;
}

As part of this commit, you'll note there was a test change in
tests/vcpupin:

 # An out-of-range vCPU number deserves a diagnostic, too.
 $abs_top_builddir/tools/virsh --connect test:///default vcpupin test
100 0,1  out 21
 test $? = 1 || fail=1
 cat \EOF  exp || fail=1
-error: vcpupin: vCPU index out of range.
+error: invalid argument: requested vcpu is higher than allocated vcpus


Searching on their error message lands one in test_driver.c/
testDomainPinVcpu().  So something specific for a set path, but the path
you're hitting is the get path.


FWIW: If a similar test was run on my system I get:
# virsh vcpupin $dom 1000 0,1
error: invalid argument: vcpu 1000 is out of range of live cpu count 2

#



So, if I understand everything that was done - then while your change is
mostly correct, I think you could perhaps message the error similar to
the vshCPUCountCollect failure (see the attached patch)


Before I make that change for you - hopefully Pavel can take a look as
well to be sure I haven't missed something.

With any luck we this could be addressed before the 1.2.17 release, but
if not since it's been a regression since 1.2.13 and no one's noticed,
then another release probably won't hurt.

John


 diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
 index 27d62e9..681fc1a 100644
 --- a/tools/virsh-domain.c
 +++ b/tools/virsh-domain.c
 @@ -6497,6 +6497,11 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
  goto cleanup;
  }
  
 +if (got_vcpu  vcpu = ncpus) {
 +vshError(ctl, _(vcpu %d is out of range of cpu count %d), 
 vcpu, ncpus);
 +goto cleanup;
 +}
 +
  cpumaplen = VIR_CPU_MAPLEN(maxcpu);
  cpumap = vshMalloc(ctl, ncpus * cpumaplen);
  if ((ncpus = virDomainGetVcpuPinInfo(dom, ncpus, cpumap,
 

From 63e642f4ad9705118870a237afd9d0bc9084200d Mon Sep 17 00:00:00 2001
From: Luyao Huang lhu...@redhat.com
Date: Mon, 29 Jun 2015 10:10:15 +0800
Subject: [PATCH] virsh: report error if vcpu number exceed the guest maxvcpu
 number

Commit id '81dd81e' caused a regression when attempting to print a
specific vcpuid that is out of the range of the maximum vcpus for
the guest, such as:

 $ virsh vcpupin $dom 1000
 VCPU: CPU Affinity
 --

 $

Rather than just recover the old message, let's adjust the message based
on what would be displayed for a similar failure in the set path, such as:

 $ virsh vcpupin $dom 1000
 error: vcpu 1000 is out of range of persistent cpu count 2

 $ virsh vcpupin $dom 1000 --live
 error: vcpu 1000 is out of range of live cpu count 2

 $

Signed-off-by: Luyao Huang lhu...@redhat.com
Signed-off-by: John Ferlan jfer...@redhat.com
---
 tools/virsh-domain.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 27d62e9..2ff2a40 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6497,6 +6497,18 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
 goto cleanup;
 }
 
+if (got_vcpu  vcpu = ncpus) {
+if (flags  VIR_DOMAIN_AFFECT_LIVE)
+vshError(ctl,
+ _(vcpu %d is out of range of live cpu count %d),
+ vcpu, ncpus);
+else
+vshError(ctl,
+ _(vcpu %d is out of range of persistent cpu count %d),
+ vcpu, ncpus);
+goto cleanup;
+}
+
 cpumaplen = VIR_CPU_MAPLEN(maxcpu);
 cpumap = vshMalloc(ctl, ncpus * cpumaplen);
 if ((ncpus = virDomainGetVcpuPinInfo(dom, ncpus, cpumap,
-- 
2.1.0

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

Re: [libvirt] [PATCH] virsh: report error if vcpu number exceed the guest maxvcpu number

2015-07-01 Thread lhuang 


On 07/02/2015 04:29 AM, John Ferlan wrote:


On 06/28/2015 10:10 PM, Luyao Huang wrote:

If usr pass a vcpu which exceed guest maxvcpu number, virsh client
will only output an header like this:

  # virsh vcpupin test3 1000
  VCPU: CPU Affinity
  --

After this patch:

  # virsh vcpupin test3 1000
  error: vcpu 1000 is out of range of cpu count 2

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  tools/virsh-domain.c | 5 +
  1 file changed, 5 insertions(+)


Seemed odd that this check wasn't there - so I did some digging...

Pavel's commit id '81dd81e' removed a check that seems to be what is
desired in this path (or was there before his change):

 if (vcpu = info.nrVirtCpu) {
 vshError(ctl, %s, _(vcpupin: vCPU index out of range.));
 goto cleanup;
 return false;
 }

As part of this commit, you'll note there was a test change in
tests/vcpupin:

  # An out-of-range vCPU number deserves a diagnostic, too.
  $abs_top_builddir/tools/virsh --connect test:///default vcpupin test
100 0,1  out 21
  test $? = 1 || fail=1
  cat \EOF  exp || fail=1
-error: vcpupin: vCPU index out of range.
+error: invalid argument: requested vcpu is higher than allocated vcpus


Searching on their error message lands one in test_driver.c/
testDomainPinVcpu().  So something specific for a set path, but the path
you're hitting is the get path.


Yes, i noticed this and checked if need introduce a test or change the 
old test, but

i found test driver not support get vcpupin.



FWIW: If a similar test was run on my system I get:
# virsh vcpupin $dom 1000 0,1
error: invalid argument: vcpu 1000 is out of range of live cpu count 2

#


So, if I understand everything that was done - then while your change is
mostly correct, I think you could perhaps message the error similar to
the vshCPUCountCollect failure (see the attached patch)


I saw the attached patch, but there is some problem about check the flag 
(actually

i had a try with check flags and output a better error before).

If check flags like vshCPUCountCollect failure, there will be a problem 
when do not
pass flag or just pass current flag to vcpupin, we will get error like 
this (pass a too big vcpu

number):

# virsh list;virsh vcpupin rhel7.0 1000 --current
 IdName   State

 3 rhel7.0running

error: vcpu 1000 is out of range of persistent cpu count 4

In this case, we output persistent instead of live, this is because 
vshCPUCountCollect()
cannot return certain flags (although there is a description say 
Returns the count of vCPUs for a
domain and certain flags). So we need more check for current flags, 
maybe like this :


diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 27d62e9..334fd3a 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6497,6 +6497,19 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
 goto cleanup;
 }

+if (got_vcpu  vcpu = ncpus) {
+if (flags  VIR_DOMAIN_AFFECT_LIVE ||
+(flags  VIR_DOMAIN_AFFECT_CURRENT  
virDomainIsActive(dom) == 1))

+vshError(ctl,
+ _(vcpu %d is out of range of live cpu count %d),
+ vcpu, ncpus);
+else
+vshError(ctl,
+ _(vcpu %d is out of range of persistent cpu 
count %d),

+ vcpu, ncpus);
+goto cleanup;
+}
+
 cpumaplen = VIR_CPU_MAPLEN(maxcpu);
 cpumap = vshMalloc(ctl, ncpus * cpumaplen);
 if ((ncpus = virDomainGetVcpuPinInfo(dom, ncpus, cpumap,



Before I make that change for you - hopefully Pavel can take a look as
well to be sure I haven't missed something.

With any luck we this could be addressed before the 1.2.17 release, but
if not since it's been a regression since 1.2.13 and no one's noticed,
then another release probably won't hurt.


Right, if we can fix it in 1.2.17, it will be better :)

Thanks a lot for your help and review.


John



Luyao

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