Xen devel - Jan 2012 - Re: [PATCH 1 of 3] libxl: extend pCPUs specification for vcpu-pin.

Dario Faggioli writes ("[Xen-devel] [PATCH 1 of 3] libxl: extend pCPUs
specification for vcpu-pin."):
> Allow for "^<cpuid>" syntax while specifying the pCPUs list
> during a vcpu-pin. This enables doing the following:
> 
>  xl vcpu-pin 1 1 0-4,^2
...
> +    if (strcmp(cpu, "all")) {
> +        for (toka = strtok(cpu, ","), i = 0; toka; toka =
strtok(NULL, ","), ++i) {
OMG you used strtok.  strtok is not thread-safe.  strtok_r is but
still modifies its input string - hence your need to strdup.  If you
do want to do it this way IMO the strdup should be in vcpupin_parse
which should take a const char*.  Are you sure this is the right
approach to parsing this ?

<record type="broken">
Your patch 2 has a number of overly long lines.
</record>

Ian.

Dario Faggioli writes ("Re: [Xen-devel] [PATCH 1 of 3] libxl: extend pCPUs
specification for vcpu-pin."):
> Well, that''s not properly me, is it that?
> 
> hg annotate tools/libxl/xl_cmdimpl.c
> ...
> 21247:     if (strcmp(cpu, "all")) { 
> 21247:         for (toka = strtok(cpu, ","), i = 0; toka; toka =
strtok(NULL, ","), ++i) {
> 21247:             cpuida = strtoul(toka, &endptr, 10);
> 21247:             if (toka == endptr) {
> ...
Oops, sorry for wrongly pointing the finger.
> Again, I know that''s probably not an alibi, but that''s
not my code! :-P
> What I did is just move this from the body of vcpupin() to an helper
> function, and try to add as _less_ thing as I can to support a slightly
> improved syntax.
Right.
> That being said, if you think I should rewrite the parsing from scratch,
> I can think about it, just wanted to clarify my position. :-)
I really don''t like the existing code here; feel free to do whatever
you think is best.

Thanks,
Ian.

Dario Faggioli writes ("Re: [Xen-devel] [PATCH 1 of 3] libxl: extend pCPUs
specification for vcpu-pin."):
> Ok, I''m not really a "string parsing guy" but
I''m interested in getting
> better. I can try restructuring this with strtok_r and strdup... Would
> that be better or you where thinking to something more "radical"?
Personally I''m a fan of flex although it''s probably overkill
for
this.  But you should use an approach your comfortable with.

What I care most about is that whatever approach you take doesn''t make
it too easy to make and hide :-) programming mistakes, since otherwise
parsing code often turns out to be buggy.

Ian.

On Fri, 2012-01-13 at 16:48 +0000, Ian Jackson wrote:
> Dario Faggioli writes ("Re: [Xen-devel] [PATCH 1 of 3] libxl: extend
pCPUs specification for vcpu-pin."):
> > Ok, I''m not really a "string parsing guy" but
I''m interested in getting
> > better. I can try restructuring this with strtok_r and strdup... Would
> > that be better or you where thinking to something more
"radical"?
> 
> Personally I''m a fan of flex although it''s probably
overkill for
> this.  But you should use an approach your comfortable with.
> 
> What I care most about is that whatever approach you take doesn''t
make
> it too easy to make and hide :-) programming mistakes, since otherwise
> parsing code often turns out to be buggy.
Can we take the version which just moves the dodgy parsee now and worry
about reworking it later? I think we''d like to see vcpu pinning in 4.2.

Ian.

On Mon, 2012-01-23 at 10:27 +0000, Dario Faggioli wrote:
> On Mon, 2012-01-23 at 10:21 +0000, Ian Campbell wrote: 
> > Can we take the version which just moves the dodgy parsee now and
worry
> > about reworking it later? I think we''d like to see vcpu
pinning in 4.2.
> > 
> Hey, sorry for being so late in resubmitting, I''ve been distrcted
by
> those (other) NUME issues... I''ll send a new version of this in
the
> afternoon!
> 
> Sorry again,
No need to apologise. I was just going over the 4.2 TODO list to repost
it. I''ve added "xl support for vcpu pinning" to the tools
section and
"NUMA improvements (cpupool autopin?)" to the hypervisor (nice to
have)
section

Ian.