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

[John Ferlan]

On 01/08/2018 03:39 AM, Bjoern Walk wrote:
> 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.
> 

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

It isn't that important...  When I first saw I thought should there be a
VIR_STEAL_PTR usage, but reading for context removed that thought quickly.

John

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