[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 3/5] xl: allow for node-wise specification of vcpu pinning
On ven, 2013-09-06 at 17:37 +0100, Ian Jackson wrote: > Dario Faggioli writes ("[PATCH v2 3/5] xl: allow for node-wise specification > of vcpu pinning"): > > Making it possible to use something like the following: > > * "nodes:0-3": all pCPUs of nodes 0,1,2,3; > > * "nodes:0-3,^node:2": all pCPUS of nodes 0,1,3; > > * "1,nodes:1-2,^6": pCPU 1 plus all pCPUs of nodes 1,2 > > but not pCPU 6; > > * ... > ... > > * code rearranged in order to look more simple to follow > > and understand, as requested during review; > > This is much better now. Thank you! > Glad to hear that. :-) > > +static int parse_range(const char *str, unsigned long *a, unsigned long *b) > > { > ... > > + if (endptr == str) > > + return EINVAL; > > + if (*a == ULONG_MAX) > > + return ERANGE; > > So parse_range returns errno value or 0. This isn't mentioned > anywhere and is a bit unusual. > > > +static int update_cpumap_range(const char *str, libxl_bitmap *cpumap) > > +{ > ... > > + rc = libxl_node_bitmap_alloc(ctx, &node_cpumap, 0); > > + if (rc) { > > + fprintf(stderr, "libxl_node_bitmap_alloc failed.\n"); > > + return rc; > > So update_cpumap_range returns a libxl error code. > > > + rc = parse_range(str, &ida, &idb); > > + if (rc) { > > But here you assign the errno value to an `rc' variable which holds a > libxl error code. I think it would be better to make parse_range > return a libxl rc value. > You're right, I cleaned up the code, but messed up quite a bit with the error codes! Thanks for pointing this out, I will put thing in a more consistent state. > I think also that parse_range doesn't notice if the specified range > contains junk between the second number and the comma ? > Mmm... I will double check, although I'm not sure I'm getting you 100%, since parse_range does not deal with comas... That are handled by "strtok_r(cpu, ",", &saveptr)", which now is in vcpupin_parse(). > > +static int vcpupin_parse(char *cpu, libxl_bitmap *cpumap) > > +{ > ... > > + if (STR_HAS_PREFIX(ptr, "all") || > > + STR_HAS_PREFIX(ptr, "nodes:all")) { > > + libxl_bitmap_set_any(cpumap); > > Why not deal with all in parse_range ? You'd avoid the second > special-case of "nodes:", and constructions like > all,^3 > would work. > Having it here already makes that ("all,^3") work, but it is true that it'd simplify the if, and that it belongs more within parse_range() than here. I'll move it. > > -vcpp_out: > > - libxl_bitmap_dispose(&exclude_cpumap); > > + rc = update_cpumap_range(ptr, cpumap); > > + if (rc) { > > + /* If failing, reset the cpumap and exit */ > > + libxl_bitmap_set_none(cpumap); > > Surely the caller who gets a error should expect the cpumap to contain > arbitrary contents ? > Fair enough. BTW, any chance you could have a look at 4/5 and 5/5 as well, so that I can repost? Thanks and Regards, Dario -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) Attachment:
signature.asc _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |