[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



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

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


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