[libvirt] [PATCH 2/2] Rewrite vshParseCPUList

[Peter Krempa]

On Fri, Apr 10, 2015 at 16:32:33 +0200, Ján Tomko wrote:
> Use virBitmap helpers that were added after this function.
> 
> Change cpumaplen to int and fill it out by this function.
> ---
>  tools/virsh-domain.c | 112 +++++++++------------------------------------------
>  1 file changed, 19 insertions(+), 93 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index d5352d7..90e23aa 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -6335,95 +6335,23 @@ vshPrintPinInfo(unsigned char *cpumap, size_t cpumaplen)
>  }
>  
>  static unsigned char *
> -vshParseCPUList(vshControl *ctl, const char *cpulist,
> -                int maxcpu, size_t cpumaplen)
> +vshParseCPUList(int *cpumaplen, const char *cpulist, int maxcpu)

Hmm virBitmapToData could be refactored to take a size_t rather than
using int here


>  {
>      unsigned char *cpumap = NULL;
> -    const char *cur;
> -    bool unuse = false;
> -    int cpu, lastcpu;
> -    size_t i;
> -
> -    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)
> -            goto error;
> +    virBitmapPtr map = NULL;
>  
> -        if (cpu >= maxcpu) {
> -            vshError(ctl, _("Physical CPU %d doesn't exist."), cpu);
> -            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."), lastcpu);
> -                goto cleanup;
> -            }
> -
> -            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 error;
> -        }
> +    if (cpulist[0] == 'r') {
> +        if (!(map = virBitmapNew(maxcpu)))
> +            return NULL;
> +        virBitmapSetAll(map);
> +    } else {
> +        if (virBitmapParse(cpulist, '\0', &map, maxcpu) < 0)
> +            return NULL;
>      }
>  
> +    if (virBitmapToData(map, &cpumap, cpumaplen) < 0)
> +        virBitmapFree(map);

@map needs to be freed on success too.

>      return cpumap;
> -
> - error:
> -    vshError(ctl, "%s", _("cpulist: Invalid format."));
> - cleanup:
> -    VIR_FREE(cpumap);
> -    return NULL;
>  }
>  
>  static bool
> @@ -6434,7 +6362,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
>      const char *cpulist = NULL;
>      bool ret = false;
>      unsigned char *cpumap = NULL;
> -    size_t cpumaplen;
> +    int cpumaplen;

And changing the types here on ...

>      int maxcpu, ncpus;
>      size_t i;
>      bool config = vshCommandOptBool(cmd, "config");
> @@ -6473,7 +6401,6 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
>  
>      if ((maxcpu = vshNodeGetCPUCount(ctl->conn)) < 0)
>          return false;
> -    cpumaplen = VIR_CPU_MAPLEN(maxcpu);
>  
>      if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
>          return false;
> @@ -6495,6 +6422,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
>              goto cleanup;
>          }
>  
> +        cpumaplen = VIR_CPU_MAPLEN(maxcpu);
>          cpumap = vshMalloc(ctl, ncpus * cpumaplen);
>          if ((ncpus = virDomainGetVcpuPinInfo(dom, ncpus, cpumap,
>                                               cpumaplen, flags)) >= 0) {
> @@ -6514,7 +6442,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
>          }
>      } else {
>          /* Pin mode: pinning specified vcpu to specified physical cpus*/
> -        if (!(cpumap = vshParseCPUList(ctl, cpulist, maxcpu, cpumaplen)))
> +        if (!(cpumap = vshParseCPUList(&cpumaplen, cpulist, maxcpu)))
>              goto cleanup;
>  
>          if (flags == -1) {
> @@ -6580,7 +6508,7 @@ cmdEmulatorPin(vshControl *ctl, const vshCmd *cmd)
>      bool ret = false;
>      unsigned char *cpumap = NULL;
>      unsigned char *cpumaps = NULL;
> -    size_t cpumaplen;
> +    int cpumaplen;
>      int maxcpu;
>      bool config = vshCommandOptBool(cmd, "config");
>      bool live = vshCommandOptBool(cmd, "live");
> @@ -6613,8 +6541,6 @@ cmdEmulatorPin(vshControl *ctl, const vshCmd *cmd)
>          return false;
>      }
>  
> -    cpumaplen = VIR_CPU_MAPLEN(maxcpu);
> -
>      /* Query mode: show CPU affinity information then exit.*/
>      if (query) {
>          /* When query mode and neither "live", "config" nor "current"
> @@ -6622,6 +6548,7 @@ cmdEmulatorPin(vshControl *ctl, const vshCmd *cmd)
>          if (flags == -1)
>              flags = VIR_DOMAIN_AFFECT_CURRENT;
>  
> +        cpumaplen = VIR_CPU_MAPLEN(maxcpu);
>          cpumaps = vshMalloc(ctl, cpumaplen);
>          if (virDomainGetEmulatorPinInfo(dom, cpumap,
>                                          cpumaplen, flags) >= 0) {
> @@ -6636,7 +6563,7 @@ cmdEmulatorPin(vshControl *ctl, const vshCmd *cmd)
>      }
>  
>      /* Pin mode: pinning emulator threads to specified physical cpus*/
> -    if (!(cpumap = vshParseCPUList(ctl, cpulist, maxcpu, cpumaplen)))
> +    if (!(cpumap = vshParseCPUList(&cpumaplen, cpulist, maxcpu)))
>          goto cleanup;
>  
>      if (flags == -1)
> @@ -6912,7 +6839,7 @@ cmdIOThreadPin(vshControl *ctl, const vshCmd *cmd)
>      int maxcpu;
>      bool ret = false;
>      unsigned char *cpumap = NULL;
> -    size_t cpumaplen;
> +    int cpumaplen;
>      unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
>  
>      VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
> @@ -6938,9 +6865,8 @@ cmdIOThreadPin(vshControl *ctl, const vshCmd *cmd)
>  
>      if ((maxcpu = vshNodeGetCPUCount(ctl->conn)) < 0)
>          goto cleanup;
> -    cpumaplen = VIR_CPU_MAPLEN(maxcpu);
>  
> -    if (!(cpumap = vshParseCPUList(ctl, cpulist, maxcpu, cpumaplen)))
> +    if (!(cpumap = vshParseCPUList(&cpumaplen, cpulist, maxcpu)))
>          goto cleanup;
>  
>      if (virDomainPinIOThread(dom, iothread_id,

ACK with the memleak fixed. Refactoring virBitmapToData is optional.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150413/e3bee61e/attachment-0001.sig>