Let's also parse the available processor frequency information on S390
so that it can be utilized by virsh sysinfo:

    # virsh sysinfo

    <sysinfo type='smbios'>
      ...
      <processor>
	<entry name='family'>2964</entry>
	<entry name='manufacturer'>IBM/S390</entry>
	<entry name='version'>00</entry>
	<entry name='external_clock'>5000</entry>
	<entry name='max_speed'>5000</entry>
	<entry name='serial_number'>145F07</entry>
      </processor>
      ...
    </sysinfo>

Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Signed-off-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
---
 src/util/virsysinfo.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

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;
+
+        if (!(tmp_base = strstr(tmp_base, "cpu MHz dynamic")) ||
+            !virSysinfoParseS390Line(tmp_base, "cpu MHz dynamic", &mhz) ||
+            !mhz)
+            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;
+
+        VIR_FREE(ncpu);
+    }
+
     result = 0;
 
  cleanup:
     VIR_FREE(manufacturer);
     VIR_FREE(procline);
+    VIR_FREE(ncpu);
     return result;
 }
 
-- 
2.13.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 12/19/2017 05:08 AM, Bjoern Walk wrote:
> Let's also parse the available processor frequency information on S390
> so that it can be utilized by virsh sysinfo:
> 
>     # virsh sysinfo
> 
>     <sysinfo type='smbios'>
>       ...
>       <processor>
> 	<entry name='family'>2964</entry>
> 	<entry name='manufacturer'>IBM/S390</entry>
> 	<entry name='version'>00</entry>
> 	<entry name='external_clock'>5000</entry>
> 	<entry name='max_speed'>5000</entry>
> 	<entry name='serial_number'>145F07</entry>
>       </processor>
>       ...
>     </sysinfo>
> 
> Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
> Signed-off-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
> ---
>  src/util/virsysinfo.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> 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;
>  }
>  
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

John Ferlan <jferlan@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;
> >  }
> >  
> > 
> 
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 01/08/2018 03:39 AM, Bjoern Walk wrote:
> John Ferlan <jferlan@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;
>>>  }
>>>  
>>>
>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

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

-- 
IBM Systems
Linux on z Systems & Virtualization Development
------------------------------------------------------------------------
IBM Deutschland
Schönaicher Str. 220
71032 Böblingen
Phone: +49 7031 16 1819
E-Mail: bwalk@de.ibm.com
------------------------------------------------------------------------
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294 
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

John Ferlan <jferlan@redhat.com> [2018-01-10, 05:34PM -0500]:
> 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.

Actually, this is not necessary. Also my initial point was irrelevant.
If no frequency information is available, we just completely skip the
whole loop (since no line with "cpu number" is found) and everything
works as before.

So, with my changes right now, we parse liberally, the only way we bail
out with error (and discard everything in the process) is when the
virStrToLong_uip fails. We report a debug message for the unlikely case
of n >= ret->nprocessor.

I have also added a new test case to the sysinfotest with updated data
for /proc/cpuinfo and /proc/sysinfo. Will send v2 shortly.
> 
> 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.

Thanks a lot!

> John
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

Best,
Bjoern

-- 
IBM Systems
Linux on z Systems & Virtualization Development
------------------------------------------------------------------------
IBM Deutschland
Schönaicher Str. 220
71032 Böblingen
Phone: +49 7031 16 1819
E-Mail: bwalk@de.ibm.com
------------------------------------------------------------------------
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294 
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list