On 01/09/2018 04:08 AM, Bjoern Walk wrote:
John Ferlan <jferlan@redhat.com> [2018-01-08, 07:55AM -0500]:
On 01/08/2018 03:39 AM, Bjoern Walk wrote:
John Ferlan <jferlan@redhat.com> [2018-01-04, 03:56PM -0500]:
On 12/19/2017 05:08 AM, Bjoern Walk wrote:
diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c index ab81b1f7..0c2267e3 100644 --- a/src/util/virsysinfo.c +++ b/src/util/virsysinfo.c @@ -495,6 +495,7 @@ virSysinfoParseS390Processor(const char *base, virSysinfoDefPtr ret) char *tmp_base; char *manufacturer = NULL; char *procline = NULL; + char *ncpu = NULL; int result = -1; virSysinfoProcessorDefPtr processor;
@@ -524,11 +525,41 @@ virSysinfoParseS390Processor(const char *base, virSysinfoDefPtr ret)
VIR_FREE(procline); } + + /* now, for each processor found, extract the frequency information */ + tmp_base = (char *) base; + + while ((tmp_base = strstr(tmp_base, "cpu number")) && + (tmp_base = virSysinfoParseS390Line(tmp_base, "cpu number", &ncpu))) { + unsigned int n; + char *mhz = NULL; + + if (virStrToLong_ui(ncpu, NULL, 10, &n) < 0 || n >= ret->nprocessor) + goto cleanup;
Should these be split? Reason I ask is if n >= ret->nprocessor, then going to cleanup results in returning a failure. That leads to an eventual generic command failed for some reason. Of course that reason shouldn't be possible, but since this is a CYA exercise, the check should have a specific error message - similar to what one would get if other calls failed...
I don't quite follow. You want an explicit error message here if n >= ret->nprocessor? Right now, for this call sequence there is no error reporting at all. This just fills the respective driver->hostsysinfo struct and sets this to NULL in case of an error. Later on, when the hostsysinfo is used and not available, an error is generated.
When I first read, I read it "out of context"... If the first call fails, then you get an error, if the second call fails, then there is no error so the first question that pops in my mind is - should we provide an error message in that case?
In general, yes, we should provide some information about what's happening. But I found all this code to be somewhat unsatisfactory in this regard and wanted to avoid doing to many changes in this series.
I would hope that n >= ret->nprocessor couldn't be true considering what was recently read a few lines earlier and if the number was wrong here, then the cpuinfo output would be AFU'd, right? I don't have picture in my mind of what's being processed, but didn't want to ignore this possible condition.
Since @result would be -1, then going to cleanup at this point would be a failure. The question thus becomes should we ignore (and return 0) when cpuinfo is bad and maybe toss out a VIR_DEBUG or should we error out and throw everything away?
Alright, this sounds good. I agree that we should try to gather as much information as possible and don't discard everything when one part fails. But again, this whole file has somewhat different semantics.
Cannot disagree with either point!
The one thing with waiting for some caller to determine hostsysinfo is not available and an error generated is that we lose the reason why. I defer to you to decide what the best course of action is.
+ + if (!(tmp_base = strstr(tmp_base, "cpu MHz dynamic")) || + !virSysinfoParseS390Line(tmp_base, "cpu MHz dynamic", &mhz) || + !mhz) + goto cleanup;
Now, what about those? Should we also just log and return 0 here? CPU frequency information has only recently been introduced on S390 and running this on a kernel without support for it would now discard hostsysinfo entirely.
Thinking partially in terms of something Andrea posted as a result of something I commented on: https://www.redhat.com/archives/libvir-list/2018-January/msg00240.html Maybe we should take the approach of leaving it as zero if we cannot find what we're looking for... So the above changes to if ((tmp_base = strstr(tmp_base, "cpu MHz dynamic")) && (virSysinfoParseS390Line(tmp_base..., &mhz)) xxxx = mhz; IOW: Update the value if both pass and do nothing if one or the other fails. If both pass then mhz won't be leaked because we'll save it. BTW: Since Andrea has pushed and patches 1 & 2 were R-B'd, I've pushed them. So it'll just be this third patch with adjustments. John