Hi Eric, Thanks for taking a look. On 02/10/2012 04:33 AM, Eric Blake wrote:
On 02/09/2012 01:47 AM, Prerna Saxena wrote:
From: Prerna Saxena <prerna@linux.vnet.ibm.com> Date: Tue, 7 Feb 2012 16:55:26 +0530 Subject: [PATCH] Implement sysinfo on PowerPC.
Libvirt on x86 parses 'dmidecode' to gather characteristics of host system, which are then reflected to libvirt users by virSysinfoRead(), invoked by 'virsh sysinfo'. This patch implements it on PowerPC by reading /proc/cpuinfo.
The presently available fields in 'sysinfo' are strongly tied to dmidecode output fields. This patch attempts to retrofit the information available in PowerPC to appropriate sysinfo fields. I will be happy to change the organization of this information to if there are expected outputs for individual fields. (I couldnt find any documentation which explained what each sysinfo field was expected to convey.)
At this point, I think it might be worth waiting until post-0.9.10, and rebasing this on the latest. In particular,
I'll rebase the patch once the merge window is open again :)
+ +#if defined(__powerpc__) +static char* +virSysinfoParseSystem(char *base, virSysinfoDefPtr ret)
We just recently changed the signature of this function in the x86 case, so the two implementations should probably have a similar signature.
Sure, I'll modify this.
+ + ret->system_manufacturer = NULL; + ret->system_product = NULL;
Setting these to NULL is redundant, since ret was just freshly allocated.
Thanks for pointing this out, will remove.
+ ret->system_uuid = NULL;
Is there any alternative way to obtain the host UUID that we could fill in here?
I checked that 'UUID' is a part of SMBIOS specification, and therefore not available on PowerPC system. We do specify a system serial -- maybe that would suffice ?
+/* virSysinfoRead for PowerPC */ +virSysinfoDefPtr +virSysinfoRead(void) { + virSysinfoDefPtr ret = NULL; + char *outbuf = NULL; + + if (VIR_ALLOC(ret) < 0) + goto no_memory; + + /* mark irrelevant fields as 'NULL' */ + ret->bios_vendor = NULL; + ret->bios_version = NULL; + ret->bios_date = NULL; + ret->bios_release = NULL;
Again, redundant, since VIR_ALLOC() already took care of that.
Will remove this.
+ + if(virFileReadAll(CPUINFO, 2048, &outbuf) < 0) { + virSmbiosReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to open %s"),CPUINFO);
Nit: space after ','.
I'll clean this up.
Overall, the idea looks nice.
I'm also wondering how much of that implementation of parsing /proc/cpuinfo can be reused on x86 in the cases where dmidecode is not accessible (such as when running qemu:///session as non-root)? Filling out what we can seems like a good idea.
It would be nice to do this. However, the ppc-specific parsing cannot be reused since format of /proc/cpuinfo is very much tied to architecture. We'll separately need to write a parser that understands the format of /proc/cpuinfo for x86. Thanks, -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India