[libvirt] [PATCH 0/2] virsh vcpupin cleanups
Ján Tomko (2): Rewrite vshPrintPinInfo Rewrite vshParseCPUList tools/virsh-domain.c | 153 ++++++++++----------------------------------------- 1 file changed, 29 insertions(+), 124 deletions(-) -- 2.0.5
Use virBitmapDataToString instead of constructing the ranges bit by bit, remove the checking of parameters (that is already done by the callers). Let the callers choose the right bitmap, since there's only one that uses this helper on a matrix-in-an-array. --- tools/virsh-domain.c | 41 ++++++++++------------------------------- 1 file changed, 10 insertions(+), 31 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 928360c..d5352d7 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6322,36 +6322,15 @@ static const vshCmdOptDef opts_vcpupin[] = { * Helper function to print vcpupin info. */ static bool -vshPrintPinInfo(unsigned char *cpumaps, size_t cpumaplen, - int maxcpu, int vcpuindex) +vshPrintPinInfo(unsigned char *cpumap, size_t cpumaplen) { - int cpu, lastcpu; - bool bit, lastbit, isInvert; + char *str = NULL; - if (!cpumaps || cpumaplen <= 0 || maxcpu <= 0 || vcpuindex < 0) + if (!(str = virBitmapDataToString(cpumap, cpumaplen))) return false; - bit = lastbit = isInvert = false; - lastcpu = -1; - - for (cpu = 0; cpu < maxcpu; cpu++) { - bit = VIR_CPU_USABLE(cpumaps, cpumaplen, vcpuindex, cpu); - - isInvert = (bit ^ lastbit); - if (bit && isInvert) { - if (lastcpu == -1) - vshPrint(ctl, "%d", cpu); - else - vshPrint(ctl, ",%d", cpu); - lastcpu = cpu; - } - if (!bit && isInvert && lastcpu != cpu - 1) - vshPrint(ctl, "-%d", cpu - 1); - lastbit = bit; - } - if (bit && !isInvert) - vshPrint(ctl, "-%d", maxcpu - 1); - + vshPrint(ctl, "%s", str); + VIR_FREE(str); return true; } @@ -6526,7 +6505,8 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) continue; vshPrint(ctl, "%4zu: ", i); - ret = vshPrintPinInfo(cpumap, cpumaplen, maxcpu, i); + ret = vshPrintPinInfo(VIR_GET_CPUMAP(cpumap, cpumaplen, i), + cpumaplen); vshPrint(ctl, "\n"); if (!ret) break; @@ -6643,12 +6623,12 @@ cmdEmulatorPin(vshControl *ctl, const vshCmd *cmd) flags = VIR_DOMAIN_AFFECT_CURRENT; cpumaps = vshMalloc(ctl, cpumaplen); - if (virDomainGetEmulatorPinInfo(dom, cpumaps, + if (virDomainGetEmulatorPinInfo(dom, cpumap, cpumaplen, flags) >= 0) { vshPrintExtra(ctl, "%s %s\n", _("emulator:"), _("CPU Affinity")); vshPrintExtra(ctl, "----------------------------------\n"); vshPrintExtra(ctl, " *: "); - ret = vshPrintPinInfo(cpumaps, cpumaplen, maxcpu, 0); + ret = vshPrintPinInfo(cpumap, cpumaplen); vshPrint(ctl, "\n"); } VIR_FREE(cpumaps); @@ -6865,8 +6845,7 @@ cmdIOThreadInfo(vshControl *ctl, const vshCmd *cmd) for (i = 0; i < niothreads; i++) { vshPrint(ctl, " %-15u ", info[i]->iothread_id); - ignore_value(vshPrintPinInfo(info[i]->cpumap, info[i]->cpumaplen, - maxcpu, 0)); + ignore_value(vshPrintPinInfo(info[i]->cpumap, info[i]->cpumaplen)); vshPrint(ctl, "\n"); virDomainIOThreadInfoFree(info[i]); } -- 2.0.5
On Fri, Apr 10, 2015 at 16:32:32 +0200, Ján Tomko wrote:
Use virBitmapDataToString instead of constructing the ranges bit by bit, remove the checking of parameters (that is already done by the callers).
Let the callers choose the right bitmap, since there's only one that uses this helper on a matrix-in-an-array. --- tools/virsh-domain.c | 41 ++++++++++------------------------------- 1 file changed, 10 insertions(+), 31 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 928360c..d5352d7 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c
...
@@ -6526,7 +6505,8 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) continue;
vshPrint(ctl, "%4zu: ", i); - ret = vshPrintPinInfo(cpumap, cpumaplen, maxcpu, i); + ret = vshPrintPinInfo(VIR_GET_CPUMAP(cpumap, cpumaplen, i), + cpumaplen); vshPrint(ctl, "\n"); if (!ret) break; @@ -6643,12 +6623,12 @@ cmdEmulatorPin(vshControl *ctl, const vshCmd *cmd) flags = VIR_DOMAIN_AFFECT_CURRENT;
cpumaps = vshMalloc(ctl, cpumaplen); - if (virDomainGetEmulatorPinInfo(dom, cpumaps, + if (virDomainGetEmulatorPinInfo(dom, cpumap,
@cpumap is NULL at this point. virDomainGetEmulatorPinInfo() requires that it's non-NULL. Additionally after this change @cpumaps is unused just allocated and freed.
cpumaplen, flags) >= 0) { vshPrintExtra(ctl, "%s %s\n", _("emulator:"), _("CPU Affinity")); vshPrintExtra(ctl, "----------------------------------\n"); vshPrintExtra(ctl, " *: "); - ret = vshPrintPinInfo(cpumaps, cpumaplen, maxcpu, 0); + ret = vshPrintPinInfo(cpumap, cpumaplen); vshPrint(ctl, "\n"); } VIR_FREE(cpumaps);
ACK if you alocate @cpumap before the call and remove @cpumaps. Peter
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) { 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); 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; 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, -- 2.0.5
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
participants (2)
-
Ján Tomko -
Peter Krempa