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;
> }
>
> /*
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list