[Libvir] [PATCH] BZ#251641: Allow to change the cpu pinning for inactive domain

[Saori Fukuta]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: add_vcpupin.patch2
Type: application/octet-stream
Size: 4271 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20071127/40399286/attachment-0001.obj>