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

[John Ferlan]

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 at linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
> Signed-off-by: Bjoern Walk <bwalk at 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;
>  }
>  
>