Andrea Bolognani <abologna@redhat.com> [2017-12-13, 12:34PM +0100]:
So you'd leave it in virHostCPUGetInfoPopulateLinux()? Why?
I found it more important that all the initialization (cpu, nodes, etc.) is in one place. But it actually doesn't matter.
With my approach, the value is changed only in virHostCPUGetInfoParseCPUInfo(), which makes IMHO more sense than spreading the code that changes it across two functions.
+ /* Start with parsing CPU clock speed from /proc/cpuinfo */ + if (virHostCPUGetInfoParseCPUInfo(cpuinfo, arch, mhz) < 0) + goto cleanup;
Why do we cleanup here and abandon the rest of the information? Since the information in /proc/cpuinfo is kind of volatile in its format, shouldn't we be liberal in what we accept? If we can't parse it, we just report mhz = 0, but gathering the rest of the information in this function is still valuable.
Most functions in libvirt either perform all tasks succesfully or return a failure, so failing here is in line both with that and with the existing behavior.
So for example for S390 we have introduced CPU frequency information in /proc/cpuinfo only recently. That means that depending on your kernel version, you either read freq. info and the rest of the stuff or you discard the whole CPU. I found this highly unintuitive.
-- Andrea Bolognani / Red Hat / Virtualization
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- 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