11:55 a.m.
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;
}
/*