[libvirt] [PATCH][PowerPC] Implement sysinfo for libvirt on PowerPC

[Prerna Saxena]

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