On Mon, 26 Nov 2007 10:17:41 -0500 Daniel Veillard wrote:
On Mon, Nov 26, 2007 at 11:12:29AM +0900, Saori Fukuta wrote:
I want to change the cpu pinning for inactive domain on RHEL-5.1. So, I just add the xenXMDomainPinVcpu to xm_internal.c. We will be allowed to change "cpus" parameter in configuration file with "vcpupin" command by this patch, like "setmem" or "setvcpus".
thanks for looking at this, I think this could be fixed but there is just a couple of things i feel a bit uneasy with the patch: - they work with arrays allocated on the stack of fixed dimention (and rather big), i would rather see a dynamic allocation of the array to a more reasonnable size and growing them dynamically
Yeah, that may be rather big, thanks for the comment. I changed the code to allocate the memory dynamically. + /* allocate memory for mapstr */ + alloc_sz = (maplen * 32); + mapstr = malloc(alloc_sz); + if (mapstr == NULL) + return (-1); + memset(mapstr, 0, alloc_sz); This is why I set (maplen * 32) as size for malloc. - the limited of maplen is 8 - the maxmam cpu value is 2-digit and need to insert a comma for each cpu (i.e. 0,1,2,....,64), so the maximal length will be 192 - as a result, the size we need is around 32 for each maplen
- the repeted use of strcat on each cpu pinning found makes it a quadratic algorithm though it could really be linear if one kept a pointer to the current end of the buffer
I'm not exactly sure what to do, but you may be concerned about overflowing the actual space, right ? If so, how about this one ? + if (cpumap[i] & (1 << j)) { + len = snprintf(mapstr, alloc_sz, "%s%d,", mapstr, (8 * i) + j); + if (len >= alloc_sz) + goto cleanup; + }
- the ranges variable is defined in the middle of the function, this may look notmal in C++ but this is against the conventions in libvirt C code.
Yes, it makes a lot of sense. I reworked with your comment. Could you check it ? Regards, Saori Fukuta