On Mon, 2015-07-06 at 17:34 +0530, Shivaprasad bhat wrote:
Thanks a lot Andrea. The cleanups are really nice. I had a chance to test the patch and it seems to work consistently in all sucores_per_core modes.
Only two comments written inline .
Glad you're happy with the changes!
+nodeGetThreadsPerSubcore;
The nodeGetThreadsPerSubcore being PPC specific, is it good to have it in nodeinfo.h ? That was the rational on which I had the ioctl wrapper written in virarch.c in v2.
Even though it's ppc64 specific, all the logic that uses that function and needs to take the value of threads_per_subcore into account is already part of that file, so I think it makes sense to have it there. The maintainers might disagree, of course :)
+ if (virBitmapSetBit(cpu_map, cpu) < 0) { + printf("virBitmapSetBit(%d)\n", cpu);
Using printf here. May be you wanted virReportError(?). I think we can ignore the SetBit return, given this is from directory parsing.
That was indeed not supposed to be there, good catch! I've removed it and posted v4 of the series. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team