[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/4] vcpupin: improve vcpupin definition of virsh vcpupin



At 06/20/2011 02:46 PM, Daniel Veillard Write:
> On Fri, Jun 10, 2011 at 03:38:55PM +0900, Taku Izumi wrote:
>>
>> When using vcpupin command, we have to speficy comma-separated list as cpulist, 
>> but this is tedious in case the number of phsycal cpus is large.
>> This patch improves this by introducing special markup "-" and "^" which are 
>> similar to XML schema of "cpuset" attribute. 
>>
>> That is:
> 
>   The example
> 
>>  # virsh vcpupin Guest 0 0-15,^8
>>  
>>  is identical to
>>
>>  # virsh vcpupin Guest 0 0,1,2,3,4,5,6,7,9,10,11,12,13,14,15
>>
>> NOTE: The expression is sequencially evaluated, so "0-15,^8" is not identical

s/sequencially/sequentially/

>> to "^8,0-15".
> 
>   and this limitation should also be added to the virsh command doc I think
> 
>> Signed-off-by: Taku Izumi <izumi taku jp fujitsu com>
>> ---
>>  tools/virsh.c   |  125 +++++++++++++++++++++++++++++++-------------------------
>>  tools/virsh.pod |    4 +
>>  2 files changed, 73 insertions(+), 56 deletions(-)
>>
>> Index: libvirt/tools/virsh.c
>> ===================================================================
>> --- libvirt.orig/tools/virsh.c
>> +++ libvirt/tools/virsh.c
>> @@ -2946,8 +2946,9 @@ cmdVcpupin(vshControl *ctl, const vshCmd
>>      bool ret = true;
>>      unsigned char *cpumap;
>>      int cpumaplen;
>> -    int i;
>> -    enum { expect_num, expect_num_or_comma } state;
>> +    int i, cpu, lastcpu, maxcpu;
>> +    bool unuse = false;
>> +    char *cur;
>>      int config = vshCommandOptBool(cmd, "config");
>>      int live = vshCommandOptBool(cmd, "live");
>>      int current = vshCommandOptBool(cmd, "current");
>> @@ -3003,66 +3004,74 @@ cmdVcpupin(vshControl *ctl, const vshCmd
>>          return false;
>>      }
>>  
>> -    /* Check that the cpulist parameter is a comma-separated list of
>> -     * numbers and give an intelligent error message if not.
>> -     */
>> -    if (cpulist[0] == '\0') {
>> -        vshError(ctl, "%s", _("cpulist: Invalid format. Empty string."));
>> -        virDomainFree (dom);
>> -        return false;
>> -    }
>> +    maxcpu = VIR_NODEINFO_MAXCPUS(nodeinfo);
>> +    cpumaplen = VIR_CPU_MAPLEN(maxcpu);
>> +    cpumap = vshCalloc(ctl, 0, cpumaplen);
>> +
>> +    /* Parse cpulist */
>> +    cur = cpulist;
>> +    if (*cur == 0)
>> +        goto parse_error;
>> +
>> +    while (*cur != 0) {
>> +
>> +        /* the char '^' denotes exclusive */
>> +        if (*cur == '^') {
>> +            cur++;
>> +            unuse = true;
>> +        }
>> +
>> +        /* parse physical CPU number */
>> +        if (!c_isdigit(*cur))
>> +            goto parse_error;
>> +        cpu  = virParseNumber(&cur);
>> +        if (cpu < 0) {
>> +            goto parse_error;
>> +        }
>> +        if (cpu >= maxcpu) {
>> +            vshError(ctl, _("Physical CPU %d doesn't exist."), cpu);
>> +            goto parse_error;
>> +        }
>> +        virSkipSpaces(&cur);
>>  
>> -    state = expect_num;
>> -    for (i = 0; cpulist[i]; i++) {
>> -        switch (state) {
>> -        case expect_num:
>> -          if (!c_isdigit (cpulist[i])) {
>> -                vshError(ctl, _("cpulist: %s: Invalid format. Expecting "
>> -                                "digit at position %d (near '%c')."),
>> -                         cpulist, i, cpulist[i]);
>> -                virDomainFree (dom);
>> -                return false;
>> +        if ((*cur == ',') || (*cur == 0)) {
>> +            if (unuse) {
>> +                VIR_UNUSE_CPU(cpumap, cpu);
>> +            } else {
>> +                VIR_USE_CPU(cpumap, cpu);
>>              }
>> -            state = expect_num_or_comma;
>> -            break;
>> -        case expect_num_or_comma:
>> -            if (cpulist[i] == ',')
>> -                state = expect_num;
>> -            else if (!c_isdigit (cpulist[i])) {
>> -                vshError(ctl, _("cpulist: %s: Invalid format. Expecting "
>> -                                "digit or comma at position %d (near '%c')."),
>> -                         cpulist, i, cpulist[i]);
>> -                virDomainFree (dom);
>> -                return false;
>> +        } else if (*cur == '-') {
>> +            /* the char '-' denotes range */
>> +            if (unuse) {
>> +                goto parse_error;
>> +            }
>> +            cur++;
>> +            virSkipSpaces(&cur);
>> +            /* parse the end of range */
>> +            lastcpu = virParseNumber(&cur);
>> +            if (lastcpu < cpu) {
>> +                goto parse_error;
>> +            }
>> +            if (lastcpu >= maxcpu) {
>> +                vshError(ctl, _("Physical CPU %d doesn't exist."), maxcpu);
>> +                goto parse_error;
>> +            }
>> +            for (i = cpu; i <= lastcpu; i++) {
>> +                VIR_USE_CPU(cpumap, i);
>>              }
>> +            virSkipSpaces(&cur);
>>          }
>> -    }
>> -    if (state == expect_num) {
>> -        vshError(ctl, _("cpulist: %s: Invalid format. Trailing comma "
>> -                        "at position %d."),
>> -                 cpulist, i);
>> -        virDomainFree (dom);
>> -        return false;
>> -    }
>>  
>> -    cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo));
>> -    cpumap = vshCalloc(ctl, 1, cpumaplen);
>> -
>> -    do {
>> -        unsigned int cpu = atoi(cpulist);
>> -
>> -        if (cpu < VIR_NODEINFO_MAXCPUS(nodeinfo)) {
>> -            VIR_USE_CPU(cpumap, cpu);
>> +        if (*cur == ',') {
>> +            cur++;
>> +            virSkipSpaces(&cur);
>> +            unuse = false;
>> +        } else if (*cur == 0) {
>> +            break;
>>          } else {
>> -            vshError(ctl, _("Physical CPU %d doesn't exist."), cpu);
>> -            VIR_FREE(cpumap);
>> -            virDomainFree(dom);
>> -            return false;
>> +            goto parse_error;
>>          }
>> -        cpulist = strchr(cpulist, ',');
>> -        if (cpulist)
>> -            cpulist++;
>> -    } while (cpulist);
>> +    }
>>  
>>      if (flags == -1) {
>>          if (virDomainPinVcpu(dom, vcpu, cpumap, cpumaplen) != 0) {
>> @@ -3074,9 +3083,15 @@ cmdVcpupin(vshControl *ctl, const vshCmd
>>          }
>>      }
>>  
>> +cleanup:
>>      VIR_FREE(cpumap);
>>      virDomainFree(dom);
>>      return ret;
>> +
>> +parse_error:
>> +    vshError(ctl, "%s", _("cpulist: Invalid format."));
>> +    ret = false;
>> +    goto cleanup;
>>  }
>>  
>>  /*
>> Index: libvirt/tools/virsh.pod
>> ===================================================================
>> --- libvirt.orig/tools/virsh.pod
>> +++ libvirt/tools/virsh.pod
>> @@ -794,7 +794,9 @@ vCPUs, the running time, the affinity to
>>  I<--current>
>>  
>>  Pin domain VCPUs to host physical CPUs. The I<vcpu> number must be provided
>> -and I<cpulist> is a comma separated list of physical CPU numbers.
>> +and I<cpulist> is a list of physical CPU numbers. Its syntax is a comma
>> +separated list and a special markup using '-' and '^' (ex. '0-4', '0-3,^2') can
>> +also be allowed. The '-' denotes the range and the '^' denotes exclusive.
>>  If I<--live> is specified, affect a running guest.
>>  If I<--config> is specified, affect the next boot of a persistent guest.
>>  If I<--current> is specified, affect the current guest state.
> 
>   Okay, ACK, the point about having to generate the long comma list is
>   right and this seems like the right solution.

I have pushed it with this(add extra documentation in the virsh.pod):

>From bab581dce13850509b38beef0de329ef35e4c6d1 Mon Sep 17 00:00:00 2001
From: Taku Izumi <izumi taku jp fujitsu com>
Date: Fri, 10 Jun 2011 15:38:55 +0900
Subject: [PATCH] vcpupin: improve vcpupin definition of virsh vcpupin

When using vcpupin command, we have to speficy comma-separated list as cpulist,
but this is tedious in case the number of phsycal cpus is large.
This patch improves this by introducing special markup "-" and "^" which are
similar to XML schema of "cpuset" attribute.

The example:

 # virsh vcpupin Guest 0 0-15,^8

 is identical to

 # virsh vcpupin Guest 0 0,1,2,3,4,5,6,7,9,10,11,12,13,14,15

NOTE: The expression is sequencially evaluated, so "0-15,^8" is not identical
to "^8,0-15".

Signed-off-by: Taku Izumi <izumi taku jp fujitsu com>
---
 tools/virsh.c   |  125 +++++++++++++++++++++++++++++++------------------------
 tools/virsh.pod |    7 +++-
 2 files changed, 76 insertions(+), 56 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index b7b9552..92faffc 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -2998,8 +2998,9 @@ cmdVcpupin(vshControl *ctl, const vshCmd *cmd)
     bool ret = true;
     unsigned char *cpumap;
     int cpumaplen;
-    int i;
-    enum { expect_num, expect_num_or_comma } state;
+    int i, cpu, lastcpu, maxcpu;
+    bool unuse = false;
+    char *cur;
     int config = vshCommandOptBool(cmd, "config");
     int live = vshCommandOptBool(cmd, "live");
     int current = vshCommandOptBool(cmd, "current");
@@ -3055,66 +3056,74 @@ cmdVcpupin(vshControl *ctl, const vshCmd *cmd)
         return false;
     }
 
-    /* Check that the cpulist parameter is a comma-separated list of
-     * numbers and give an intelligent error message if not.
-     */
-    if (cpulist[0] == '\0') {
-        vshError(ctl, "%s", _("cpulist: Invalid format. Empty string."));
-        virDomainFree (dom);
-        return false;
-    }
+    maxcpu = VIR_NODEINFO_MAXCPUS(nodeinfo);
+    cpumaplen = VIR_CPU_MAPLEN(maxcpu);
+    cpumap = vshCalloc(ctl, 0, cpumaplen);
 
-    state = expect_num;
-    for (i = 0; cpulist[i]; i++) {
-        switch (state) {
-        case expect_num:
-          if (!c_isdigit (cpulist[i])) {
-                vshError(ctl, _("cpulist: %s: Invalid format. Expecting "
-                                "digit at position %d (near '%c')."),
-                         cpulist, i, cpulist[i]);
-                virDomainFree (dom);
-                return false;
-            }
-            state = expect_num_or_comma;
-            break;
-        case expect_num_or_comma:
-            if (cpulist[i] == ',')
-                state = expect_num;
-            else if (!c_isdigit (cpulist[i])) {
-                vshError(ctl, _("cpulist: %s: Invalid format. Expecting "
-                                "digit or comma at position %d (near '%c')."),
-                         cpulist, i, cpulist[i]);
-                virDomainFree (dom);
-                return false;
-            }
+    /* Parse cpulist */
+    cur = cpulist;
+    if (*cur == 0)
+        goto parse_error;
+
+    while (*cur != 0) {
+
+        /* the char '^' denotes exclusive */
+        if (*cur == '^') {
+            cur++;
+            unuse = true;
         }
-    }
-    if (state == expect_num) {
-        vshError(ctl, _("cpulist: %s: Invalid format. Trailing comma "
-                        "at position %d."),
-                 cpulist, i);
-        virDomainFree (dom);
-        return false;
-    }
 
-    cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo));
-    cpumap = vshCalloc(ctl, 1, cpumaplen);
+        /* parse physical CPU number */
+        if (!c_isdigit(*cur))
+            goto parse_error;
+        cpu  = virParseNumber(&cur);
+        if (cpu < 0) {
+            goto parse_error;
+        }
+        if (cpu >= maxcpu) {
+            vshError(ctl, _("Physical CPU %d doesn't exist."), cpu);
+            goto parse_error;
+        }
+        virSkipSpaces(&cur);
 
-    do {
-        unsigned int cpu = atoi(cpulist);
+        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 parse_error;
+            }
+            cur++;
+            virSkipSpaces(&cur);
+            /* parse the end of range */
+            lastcpu = virParseNumber(&cur);
+            if (lastcpu < cpu) {
+                goto parse_error;
+            }
+            if (lastcpu >= maxcpu) {
+                vshError(ctl, _("Physical CPU %d doesn't exist."), maxcpu);
+                goto parse_error;
+            }
+            for (i = cpu; i <= lastcpu; i++) {
+                VIR_USE_CPU(cpumap, i);
+            }
+            virSkipSpaces(&cur);
+        }
 
-        if (cpu < VIR_NODEINFO_MAXCPUS(nodeinfo)) {
-            VIR_USE_CPU(cpumap, cpu);
+        if (*cur == ',') {
+            cur++;
+            virSkipSpaces(&cur);
+            unuse = false;
+        } else if (*cur == 0) {
+            break;
         } else {
-            vshError(ctl, _("Physical CPU %d doesn't exist."), cpu);
-            VIR_FREE(cpumap);
-            virDomainFree(dom);
-            return false;
+            goto parse_error;
         }
-        cpulist = strchr(cpulist, ',');
-        if (cpulist)
-            cpulist++;
-    } while (cpulist);
+    }
 
     if (flags == -1) {
         if (virDomainPinVcpu(dom, vcpu, cpumap, cpumaplen) != 0) {
@@ -3126,9 +3135,15 @@ cmdVcpupin(vshControl *ctl, const vshCmd *cmd)
         }
     }
 
+cleanup:
     VIR_FREE(cpumap);
     virDomainFree(dom);
     return ret;
+
+parse_error:
+    vshError(ctl, "%s", _("cpulist: Invalid format."));
+    ret = false;
+    goto cleanup;
 }
 
 /*
diff --git a/tools/virsh.pod b/tools/virsh.pod
index fa37442..b90f37e 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -833,13 +833,18 @@ vCPUs, the running time, the affinity to physical processors.
 I<--current>
 
 Pin domain VCPUs to host physical CPUs. The I<vcpu> number must be provided
-and I<cpulist> is a comma separated list of physical CPU numbers.
+and I<cpulist> is a list of physical CPU numbers. Its syntax is a comma
+separated list and a special markup using '-' and '^' (ex. '0-4', '0-3,^2') can
+also be allowed. The '-' denotes the range and the '^' denotes exclusive.
 If I<--live> is specified, affect a running guest.
 If I<--config> is specified, affect the next boot of a persistent guest.
 If I<--current> is specified, affect the current guest state.
 Both I<--live> and I<--config> flags may be given, but I<--current> is exclusive.
 If no flag is specified, behavior is different depending on hypervisor.
 
+B<Note>: The expression is sequentially evaluated, so "0-15,^8" is not identical
+to "^8,0-15".
+
 =item B<vncdisplay> I<domain-id>
 
 Output the IP address and port number for the VNC display. If the information
-- 
1.7.1


> 
> Daniel
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]