Re: [libvirt] [PATCH] virsh: Add a helper to parse cpulist
On 02/04/13 00:55, John Ferlan wrote:
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
Okay.
> 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?
Yes
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
Okay. It was in old code, but trivial to sort it out together.
> + 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?
Good catch, worth fixing together.
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
I'm not sure if this is better than the old one. On one hand, we have
command to get the max cpus. On the other hand, the old one seems
more direct to me. I will leave this for a later patch if need.
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++)'
No, it's right, the loop here is only to set the CPUs in the range M-N,
so it should be lastcpu.
Pushed with the indention nit fixed, and the s/maxcpu/lastcpu/. Thanks.
Osier