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

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?

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?

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;