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

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

> + > + if (!(tmp_base = strstr(tmp_base, "cpu MHz dynamic")) || > + !virSysinfoParseS390Line(tmp_base, "cpu MHz dynamic", &mhz) || > + !mhz) Other virSysinfoParseS390Line callers never check whether the returned 4th argument is NULL - should they? or is the !mhz check here (and the next one) superfluous? I note the @ncpu one above doesn't have it either. In the long run, who cares if it's NULL?

> + goto cleanup; > + > + ret->processor[n].processor_external_clock = mhz; > + > + if (!(tmp_base = strstr(tmp_base, "cpu MHz static")) || > + !virSysinfoParseS390Line(tmp_base, "cpu MHz static", &mhz) || > + !mhz) > + goto cleanup; > + > + ret->processor[n].processor_max_speed = mhz; FWIW, you could remove @mhz and replace with a "virSysinfoProcessorDefPtr processor;" definition followed by an appropriately placed "processsor = &ret->processor[n];", and then and assign directly to &processor->{external_clock|processor_max_speed}

John > + > + VIR_FREE(ncpu); > + } > + > result = 0; > > cleanup: > VIR_FREE(manufacturer); > VIR_FREE(procline); > + VIR_FREE(ncpu); > return result; > } > >