On Thu, 2015-07-09 at 11:46 -0400, John Ferlan wrote: > > On 07/07/2015 03:25 AM, Andrea Bolognani wrote: > > Changes from v3 to v4: > > > > * removed a printf() statement; > > > > * fixed typo in a commit message. > > > > Shivaprasad G Bhat (2): > > Fix nodeinfo output on PPC64 KVM hosts > > Add testcase for PPC64 kvm host nodeinfo > > Never saw the v4 2/2 come through (nor do I see it in the archive); > however, I assume it's the same as the v3 patch: > > http://www.redhat.com/archives/libvir-list/2015-July/msg00155.html It apparently didn't make it to the list... It was a pretty big message so it might be stuck in the moderation queue as previous versions of the same commit were. That said yes, v4 2/2 is the same as v3 2/2 so all your comments apply. > Given it is and what I found reviewing the following: > > http://www.redhat.com/archives/libvir-list/2015-July/msg00219.html > > regarding nodeinfo.c not really using the tests/nodeinfodata local > path > instead the running host's sysfs (/sys/devices/system) path. > > I found while testing that the proposed patch wouldn't run correctly > on > my host because my /sys/devices/system/cpu/present is "0-3" and the > patch would fail on any test with cpu4+ since the tests/nodeinfodata/ > present file isn't referenced (if it existed). > > I created a series which adjusts the SYSFS_SYSTEM_PATH logic in > nodeinfo.c to allow for a supplied path or uses the default: > > http://www.redhat.com/archives/libvir-list/2015-July/msg00278.html > > Not looking for a review of the 9 patch sysfs series, but I am > curious > to get a perspective on the patch I initially reviewed which modifies > virNodeParseNode to "filter out" or "exclude" cpu's that are offline > because they're defective/empty and perhaps how/if that applies to > this > environment as well. I was actually already planning to review your series, I just haven't found the time yet :) As for the change you're referring to, I guess the point is to detect the case where a cpu is listed among node/node*/cpu* but is not present, because of hardware faults I guess? I'd like to hear the rationale from the patch's author, but I can't find it in the list archives... > I'm also curious what happens if the 2/2 patch is run on a PPC64 host > with less than 96 cores (from .../cpu/present) since the results seem > to > expect the 96 cores to be present. It would seem the existing code > without the sysfs path redirection would fail, since the caller > linuxNodeInfoCPUPopulate would be using the host's sysfs path rather > than the tests sysfs path. The test cases I've added run fine on my laptop, because the code is not using functions that look at host files. The current situation, as you noticed, is not optimal because while some of the functions can be passed a prefix and are hence test-ready, other use absolute paths and break in mocked environments. Your series (which I haven't looked at in detail yet) seems to address that, which is definitely a step forward. Once I've reviewed it, I will rebase the new version of this patch, which I'm already working on, on top of your series and send it to the list. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list