On 01/09/2018 04:08 AM, Bjoern Walk wrote:
> John Ferlan <jferlan@xxxxxxxxxx> [2018-01-08, 07:55AM -0500]:
>>
>>
>> On 01/08/2018 03:39 AM, Bjoern Walk wrote:
>>> John Ferlan <jferlan@xxxxxxxxxx> [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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
