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

[Bjoern Walk]

John Ferlan <jferlan at 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.

> > +
> > +        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?

Yes, combined with the strstr call I guess this check is not necessary.
I can remove it.

> 
> > +            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}

True, but that would probably make the parsing line harder to read. I'll
see if I can find some improvements.

> 
> John
> 
> 
> > +
> > +        VIR_FREE(ncpu);
> > +    }
> > +
> >      result = 0;
> >  
> >   cleanup:
> >      VIR_FREE(manufacturer);
> >      VIR_FREE(procline);
> > +    VIR_FREE(ncpu);
> >      return result;
> >  }
> >  
> > 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 906 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180108/0c2fba37/attachment-0001.sig>