Re: [libvirt] [PATCH v4 0/2] Fix nodeinfo output on PPC64 KVM hosts
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