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

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



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++) {
> -
>                 if (vcpu != -1 && i != vcpu)
>                     continue;
>  
> @@ -5556,105 +5644,28 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
>                     break;
>              }
>  
> -        } else {
> -            ret = false;
>          }
>          VIR_FREE(cpumaps);
>          goto cleanup;
>      }
>  
>      /* Pin mode: pinning specified vcpu to specified physical cpus*/
> -
> -    cpumap = vshCalloc(ctl, cpumaplen, sizeof(*cpumap));
> -    /* Parse cpulist */
> -    cur = cpulist;
> -    if (*cur == 0) {
> -        goto parse_error;
> -    } else if (*cur == 'r') {
> -        for (cpu = 0; cpu < maxcpu; cpu++)
> -            VIR_USE_CPU(cpumap, cpu);
> -        cur = "";
> -    }
> -
> -    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);
> -
> -        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 (*cur == ',') {
> -            cur++;
> -            virSkipSpaces(&cur);
> -            unuse = false;
> -        } else if (*cur == 0) {
> -            break;
> -        } else {
> -            goto parse_error;
> -        }
> -    }
> +    if (!(cpumap = virParseCPUList(ctl, cpulist, maxcpu, cpumaplen)))
> +        goto cleanup;
>  
>      if (flags == -1) {
> -        if (virDomainPinVcpu(dom, vcpu, cpumap, cpumaplen) != 0) {
> -            ret = false;
> -        }
> +        if (virDomainPinVcpu(dom, vcpu, cpumap, cpumaplen) != 0)
> +            goto cleanup;
>      } else {
> -        if (virDomainPinVcpuFlags(dom, vcpu, cpumap, cpumaplen, flags) != 0) {
> -            ret = false;
> -        }
> +        if (virDomainPinVcpuFlags(dom, vcpu, cpumap, cpumaplen, flags) != 0)
> +            goto cleanup;
>      }
>  
> +    ret = true;
>  cleanup:
>      VIR_FREE(cpumap);
>      virDomainFree(dom);
>      return ret;
> -
> -parse_error:
> -    vshError(ctl, "%s", _("cpulist: Invalid format."));
> -    ret = false;
> -    goto cleanup;
>  }
>  
>  /*
> @@ -5701,13 +5712,11 @@ cmdEmulatorPin(vshControl *ctl, const vshCmd *cmd)
>  {
>      virDomainPtr dom;
>      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;
> -    bool unuse = false;
> -    const char *cur;
> +    int maxcpu;
>      bool config = vshCommandOptBool(cmd, "config");
>      bool live = vshCommandOptBool(cmd, "live");
>      bool current = vshCommandOptBool(cmd, "current");
> @@ -5761,101 +5770,26 @@ cmdEmulatorPin(vshControl *ctl, const vshCmd *cmd)
>              vshPrint(ctl, "       *: ");
>              ret = vshPrintPinInfo(cpumaps, cpumaplen, maxcpu, 0);
>              vshPrint(ctl, "\n");
> -        } else {
> -            ret = false;
>          }
>          VIR_FREE(cpumaps);
>          goto cleanup;
>      }
>  
>      /* Pin mode: pinning emulator threads to specified physical cpus*/
> -
> -    cpumap = vshCalloc(ctl, cpumaplen, sizeof(*cpumap));
> -    /* Parse cpulist */
> -    cur = cpulist;
> -    if (*cur == 0) {
> -        goto parse_error;
> -    } else if (*cur == 'r') {
> -        for (cpu = 0; cpu < maxcpu; cpu++)
> -            VIR_USE_CPU(cpumap, cpu);
> -        cur = "";
> -    }
> -
> -    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);
> -
> -        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 (*cur == ',') {
> -            cur++;
> -            virSkipSpaces(&cur);
> -            unuse = false;
> -        } else if (*cur == 0) {
> -            break;
> -        } else {
> -            goto parse_error;
> -        }
> -    }
> +    if (!(cpumap = virParseCPUList(ctl, cpulist, maxcpu, cpumaplen)))
> +        goto cleanup;
>  
>      if (flags == -1)
>          flags = VIR_DOMAIN_AFFECT_LIVE;
>  
>      if (virDomainPinEmulator(dom, cpumap, cpumaplen, flags) != 0)
> -        ret = false;
> +        goto cleanup;
>  
> +    ret = true;
>  cleanup:
>      VIR_FREE(cpumap);
>      virDomainFree(dom);
>      return ret;
> -
> -parse_error:
> -    vshError(ctl, "%s", _("cpulist: Invalid format."));
> -    ret = false;
> -    goto cleanup;
>  }
>  
>  /*
> 


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