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

[John Ferlan]

On 01/09/2018 04:08 AM, Bjoern Walk wrote:
> John Ferlan <jferlan at redhat.com> [2018-01-08, 07:55AM -0500]:
>>
>>
>> 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?
> 
> 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.
> 

Cannot disagree with either point!

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

Thinking partially in terms of something Andrea posted as a result of
something I commented on:

https://www.redhat.com/archives/libvir-list/2018-January/msg00240.html

Maybe we should take the approach of leaving it as zero if we cannot
find what we're looking for...

So the above changes to

    if ((tmp_base = strstr(tmp_base, "cpu MHz dynamic")) &&
        (virSysinfoParseS390Line(tmp_base..., &mhz))
        xxxx = mhz;

IOW: Update the value if both pass and do nothing if one or the other
fails.  If both pass then mhz won't be leaked because we'll save it.

BTW: Since Andrea has pushed and patches 1 & 2 were R-B'd, I've pushed
them.  So it'll just be this third patch with adjustments.

John