Re: [libvirt] [PATCH] virsh: Add a helper to parse cpulist

2013-04-01 Thread Osier Yang 

Ping, anybody can review this?

On 28/03/13 19:01, Osier Yang wrote:

On 28/03/13 19:36, Osier Yang wrote:

vcpupin and emulatorpin use same code to parse the cpulist, this
abstracts the same code as a helper. Along with various code style
fixes, and error improvement (only error Physical CPU %d doesn't
exist if the specified CPU exceed the range, no cpulist: Invalid
format, see the following for an example of the error prior to
this patch).

% virsh vcpupin 4 0 0-8
error: Physical CPU 4 doesn't exist.
error: cpulist: Invalid format.
---
  tools/virsh-domain.c | 278 
---

  1 file changed, 106 insertions(+), 172 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index d1e6f9d..0fe2a51 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -5460,6 +5460,97 @@ vshPrintPinInfo(unsigned char *cpumaps, size_t 
cpumaplen,

  return true;
  }
  +static unsigned char *
+virParseCPUList(vshControl *ctl, const char *cpulist,
+int maxcpu, size_t cpumaplen)
+{



I meant vshParseCPUList. With the attached diff squashed in:




--
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: Add a helper to parse cpulist

2013-04-01 Thread Eric Blake 
On 03/28/2013 05:36 AM, Osier Yang wrote:
 vcpupin and emulatorpin use same code to parse the cpulist, this
 abstracts the same code as a helper. Along with various code style
 fixes, and error improvement (only error Physical CPU %d doesn't
 exist if the specified CPU exceed the range, no cpulist: Invalid
 format, see the following for an example of the error prior to
 this patch).
 
 % virsh vcpupin 4 0 0-8
 error: Physical CPU 4 doesn't exist.
 error: cpulist: Invalid format.
 ---
  tools/virsh-domain.c | 278 
 ---
  1 file changed, 106 insertions(+), 172 deletions(-)

ACK with your s/vir/vsh/ rename squashed in.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

Re: [libvirt] [PATCH] virsh: Add a helper to parse cpulist

2013-04-01 Thread John Ferlan 
On 03/28/2013 07:36 AM, Osier Yang wrote:
 vcpupin and emulatorpin use same code to parse the cpulist, this

How about The 'virsh vcpupin' and 'virsh emulatorpin' commands use the
same code to parse the cpulist. This patch

 abstracts the same code as a helper. Along with various code style
 fixes, and error improvement (only error Physical CPU %d doesn't
 exist if the specified CPU exceed the range, no cpulist: Invalid
 format, see the following for an example of the error prior to
 this patch).
 
 % virsh vcpupin 4 0 0-8
 error: Physical CPU 4 doesn't exist.
 error: cpulist: Invalid format.

I take it the new output doesn't have the second error then?  So say
this changes the error from above to new


 ---
  tools/virsh-domain.c | 278 
 ---
  1 file changed, 106 insertions(+), 172 deletions(-)
 
 diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
 index d1e6f9d..0fe2a51 100644
 --- a/tools/virsh-domain.c
 +++ b/tools/virsh-domain.c
 @@ -5460,6 +5460,97 @@ vshPrintPinInfo(unsigned char *cpumaps, size_t 
 cpumaplen,
  return true;
  }
  
 +static unsigned char *
 +virParseCPUList(vshControl *ctl, const char *cpulist,
 +int maxcpu, size_t cpumaplen)
 +{
 +unsigned char *cpumap = NULL;
 +const char *cur;
 +bool unuse = false;
 +int i, cpu, lastcpu;
 +
 +cpumap = vshCalloc(ctl, cpumaplen, sizeof(*cpumap));
 +
 +/* Parse cpulist */
 +cur = cpulist;
 +if (*cur == 'r') {
 +for (cpu = 0; cpu  maxcpu; cpu++)
 +VIR_USE_CPU(cpumap, cpu);
 +return cpumap;
 +} else if (*cur == 0) {
 +goto error;
 +}
 +
 +while (*cur != 0) {
 +/* The char '^' denotes exclusive */
 +if (*cur == '^') {
 +cur++;
 +unuse = true;
 +}
 +
 +/* Parse physical CPU number */
 +if (!c_isdigit(*cur))
 +goto error;
 +
 +if ((cpu  = virParseNumber(cur))  0)
^
remove extra space

 +goto error;
 +
 +if (cpu = maxcpu) {
 +vshError(ctl, _(Physical CPU %d doesn't exist.), cpu);

You probably could add something to give a hint what maxcpu is here -
although you'll need to be careful since (in your example) maxcpu is 4
and you'd only all up to 3 as a value...

 +goto cleanup;
 +}
 +
 +virSkipSpaces(cur);
 +
 +if (*cur == ',' || *cur == 0) {
 +if (unuse)
 +VIR_UNUSE_CPU(cpumap, cpu);
 +else
 +VIR_USE_CPU(cpumap, cpu);
 +} else if (*cur == '-') {
 +/* The char '-' denotes range */
 +if (unuse)
 +goto error;
 +cur++;
 +virSkipSpaces(cur);
 +
 +/* Parse the end of range */
 +lastcpu = virParseNumber(cur);
 +
 +if (lastcpu  cpu)
 +goto error;
 +
 +if (lastcpu = maxcpu) {
 +vshError(ctl, _(Physical CPU %d doesn't exist.), maxcpu);

I know this is just a cut-n-paste of the previous code, but shouldn't
this be 'lastcpu' in the error message?

Taking a cue from the previous example and knowing this is the 'range'
option - Range ending physical CPU %d is larger than maximum value %d,
lastcpu, maxcpu-1

Or something like that.

 +goto cleanup;
 +}
 +
 +for (i = cpu; i = lastcpu; i++)

Using = doesn't completely make sense here in light of the error above.
 Again, yes, I know cut-n-paste, existing code. Also loop above is 'for
(cpu = 0; cpu  maxcpu; cpu++)'



John

 +VIR_USE_CPU(cpumap, i);
 +
 +virSkipSpaces(cur);
 +}
 +
 +if (*cur == ',') {
 +cur++;
 +virSkipSpaces(cur);
 +unuse = false;
 +} else if (*cur == 0) {
 +break;
 +} else {
 +goto error;
 +}
 +}
 +
 +return cpumap;
 +
 +error:
 +vshError(ctl, %s, _(cpulist: Invalid format.));
 +cleanup:
 +VIR_FREE(cpumap);
 +return NULL;
 +}
 +
  static bool
  cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
  {
 @@ -5467,13 +5558,11 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
  virDomainPtr dom;
  int vcpu = -1;
  const char *cpulist = NULL;
 -bool ret = true;
 +bool ret = false;
  unsigned char *cpumap = NULL;
  unsigned char *cpumaps = NULL;
  size_t cpumaplen;
 -int i, cpu, lastcpu, maxcpu, ncpus;
 -bool unuse = false;
 -const char *cur;
 +int i, maxcpu, ncpus;
  bool config = vshCommandOptBool(cmd, config);
  bool live = vshCommandOptBool(cmd, live);
  bool current = vshCommandOptBool(cmd, current);
 @@ -5545,7 +5634,6 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
  vshPrint(ctl, %s %s\n, _(VCPU:), _(CPU Affinity));
  vshPrint(ctl, --\n);
  for (i = 0; i  ncpus; i++) {
 -

Re: [libvirt] [PATCH] virsh: Add a helper to parse cpulist

2013-04-01 Thread Osier Yang 

On 02/04/13 00:55, John Ferlan wrote:

On 03/28/2013 07:36 AM, Osier Yang wrote:

vcpupin and emulatorpin use same code to parse the cpulist, this

How about The 'virsh vcpupin' and 'virsh emulatorpin' commands use the
same code to parse the cpulist. This patch


Okay.




abstracts the same code as a helper. Along with various code style
fixes, and error improvement (only error Physical CPU %d doesn't
exist if the specified CPU exceed the range, no cpulist: Invalid
format, see the following for an example of the error prior to
this patch).

% virsh vcpupin 4 0 0-8
error: Physical CPU 4 doesn't exist.
error: cpulist: Invalid format.

I take it the new output doesn't have the second error then?


Yes


So say
this changes the error from above to new



---
  tools/virsh-domain.c | 278 ---
  1 file changed, 106 insertions(+), 172 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index d1e6f9d..0fe2a51 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -5460,6 +5460,97 @@ vshPrintPinInfo(unsigned char *cpumaps, size_t cpumaplen,
  return true;
  }
  
+static unsigned char *

+virParseCPUList(vshControl *ctl, const char *cpulist,
+int maxcpu, size_t cpumaplen)
+{
+unsigned char *cpumap = NULL;
+const char *cur;
+bool unuse = false;
+int i, cpu, lastcpu;
+
+cpumap = vshCalloc(ctl, cpumaplen, sizeof(*cpumap));
+
+/* Parse cpulist */
+cur = cpulist;
+if (*cur == 'r') {
+for (cpu = 0; cpu  maxcpu; cpu++)
+VIR_USE_CPU(cpumap, cpu);
+return cpumap;
+} else if (*cur == 0) {
+goto error;
+}
+
+while (*cur != 0) {
+/* The char '^' denotes exclusive */
+if (*cur == '^') {
+cur++;
+unuse = true;
+}
+
+/* Parse physical CPU number */
+if (!c_isdigit(*cur))
+goto error;
+
+if ((cpu  = virParseNumber(cur))  0)

 ^
remove extra space


Okay. It was in old code, but trivial to sort it out together.




+goto error;
+
+if (cpu = maxcpu) {
+vshError(ctl, _(Physical CPU %d doesn't exist.), cpu);

You probably could add something to give a hint what maxcpu is here -
although you'll need to be careful since (in your example) maxcpu is 4
and you'd only all up to 3 as a value..


+goto cleanup;
+}
+
+virSkipSpaces(cur);
+
+if (*cur == ',' || *cur == 0) {
+if (unuse)
+VIR_UNUSE_CPU(cpumap, cpu);
+else
+VIR_USE_CPU(cpumap, cpu);
+} else if (*cur == '-') {
+/* The char '-' denotes range */
+if (unuse)
+goto error;
+cur++;
+virSkipSpaces(cur);
+
+/* Parse the end of range */
+lastcpu = virParseNumber(cur);
+
+if (lastcpu  cpu)
+goto error;
+
+if (lastcpu = maxcpu) {
+vshError(ctl, _(Physical CPU %d doesn't exist.), maxcpu);

I know this is just a cut-n-paste of the previous code, but shouldn't
this be 'lastcpu' in the error message?


Good catch, worth fixing together.



Taking a cue from the previous example and knowing this is the 'range'
option - Range ending physical CPU %d is larger than maximum value %d,
lastcpu, maxcpu-1


I'm not sure if this is better than the old one. On one hand, we have
command to get the max cpus. On the other hand, the old one seems
more direct to me. I will leave this for a later patch if need.



Or something like that.


+goto cleanup;
+}
+
+for (i = cpu; i = lastcpu; i++)

Using = doesn't completely make sense here in light of the error above.
  Again, yes, I know cut-n-paste, existing code. Also loop above is 'for
(cpu = 0; cpu  maxcpu; cpu++)'


No, it's right, the loop here is only to set the CPUs in the range M-N,
so it should be lastcpu.

Pushed with the indention nit fixed, and the s/maxcpu/lastcpu/. Thanks.

Osier

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

Re: [libvirt] [PATCH] virsh: Add a helper to parse cpulist

2013-03-28 Thread Osier Yang 

On 28/03/13 19:36, Osier Yang wrote:

vcpupin and emulatorpin use same code to parse the cpulist, this
abstracts the same code as a helper. Along with various code style
fixes, and error improvement (only error Physical CPU %d doesn't
exist if the specified CPU exceed the range, no cpulist: Invalid
format, see the following for an example of the error prior to
this patch).

% virsh vcpupin 4 0 0-8
error: Physical CPU 4 doesn't exist.
error: cpulist: Invalid format.
---
  tools/virsh-domain.c | 278 ---
  1 file changed, 106 insertions(+), 172 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index d1e6f9d..0fe2a51 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -5460,6 +5460,97 @@ vshPrintPinInfo(unsigned char *cpumaps, size_t cpumaplen,
  return true;
  }
  
+static unsigned char *

+virParseCPUList(vshControl *ctl, const char *cpulist,
+int maxcpu, size_t cpumaplen)
+{



I meant vshParseCPUList. With the attached diff squashed in:


diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 0fe2a51..1729182 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -5461,7 +5461,7 @@ vshPrintPinInfo(unsigned char *cpumaps, size_t cpumaplen,
 }
 
 static unsigned char *
-virParseCPUList(vshControl *ctl, const char *cpulist,
+vshParseCPUList(vshControl *ctl, const char *cpulist,
 int maxcpu, size_t cpumaplen)
 {
 unsigned char *cpumap = NULL;
@@ -5650,7 +5650,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
 }
 
 /* Pin mode: pinning specified vcpu to specified physical cpus*/
-if (!(cpumap = virParseCPUList(ctl, cpulist, maxcpu, cpumaplen)))
+if (!(cpumap = vshParseCPUList(ctl, cpulist, maxcpu, cpumaplen)))
 goto cleanup;
 
 if (flags == -1) {
@@ -5776,7 +5776,7 @@ cmdEmulatorPin(vshControl *ctl, const vshCmd *cmd)
 }
 
 /* Pin mode: pinning emulator threads to specified physical cpus*/
-if (!(cpumap = virParseCPUList(ctl, cpulist, maxcpu, cpumaplen)))
+if (!(cpumap = vshParseCPUList(ctl, cpulist, maxcpu, cpumaplen)))
 goto cleanup;
 
 if (flags == -1)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list