Re: [libvirt] [PATCH 3/3] util: virsysinfo: parse frequency information on S390
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(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
Signed-off-by: Bjoern Walk <bwalk(a)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;
}