Re: [libvirt] [PATCH 3/3] util: virsysinfo: parse frequency information on S390

John Ferlan <jferlan(a)redhat.com&gt; [2018-01-08, 07:55AM -0500]: > > > On 01/08/2018 03:39 AM, Bjoern Walk wrote: >> John Ferlan <jferlan(a)redhat.com&gt; [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.

> 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.