[libvirt] [RFC v3 PATCH 3/5] PowerPC : Add support for ppc64 qemu

[Prerna Saxena]

Hi Stefan,
Thanks for reviewing the patch.

On 11/29/2011 10:25 PM, Stefan Berger wrote:
> On 11/29/2011 09:59 AM, Prerna Saxena wrote:
>> From: Prerna Saxena<prerna at linux.vnet.ibm.com>
>> Date: Mon, 3 Oct 2011 06:01:33 -0700
>> Subject: [PATCH 3/5] Add support for ppc64 qemu
>>
>> This enables libvirt to select the correct qemu binary
>> (qemu-system-ppc64)
>> for a guest vm based on arch 'ppc64'.
>> Also, libvirt is enabled to correctly parse the list of supported PowerPC
>> CPUs, generated by running 'qemu-system-ppc64 -cpu ?'
>>
>> Signed-off-by: Prerna Saxena<prerna at linux.vnet.ibm.com>
>> ---
>> src/qemu/qemu_capabilities.c | 64
>> ++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 64 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index c5fe41d..c2d3d93 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -185,6 +185,7 @@ static const struct qemu_arch_info const
>> arch_info_hvm[] = {
>> { "mipsel", 32, NULL, "qemu-system-mipsel", NULL, NULL, 0 },
>> { "sparc", 32, NULL, "qemu-system-sparc", NULL, NULL, 0 },
>> { "ppc", 32, NULL, "qemu-system-ppc", NULL, NULL, 0 },
>> + { "ppc64", 64, NULL, "qemu-system-ppc64", NULL, NULL, 0 },
>> { "itanium", 64, NULL, "qemu-system-ia64", NULL, NULL, 0 },
>> { "s390x", 64, NULL, "qemu-system-s390x", NULL, NULL, 0 },
>> };
>> @@ -477,6 +478,67 @@ error:
>> return -1;
>> }
>>
>> +/* ppc64 parser.
>> + * Format : PowerPC<machine> <description>
>> + */
>> +static int
>> +qemuCapsParsePPCModels(const char *output,
>> + unsigned int *retcount,
>> + const char ***retcpus)
>> +{
>> + const char *p = output;
>> + const char *next;
>> + unsigned int count = 0;
>> + const char **cpus = NULL;
>> + int i;
>> + do {
>> + const char *t;
>> +
>> + if ((next = strchr(p, '\n')))
>> + next++;
>> +
>> + if (!STRPREFIX(p, "PowerPC "))
>> + continue;
> If what you are parsing here is indeed corrupted, you may end up in an
> endless loop with p not moving anymore. You have to advance 'p' at least
> by 1 and have a check before whether *p == '\0' to get out of it. You
> may also want to put this check before the calculation of 'next'.

When the execution gets to a 'continue', the do..while loop will resume 
execution from next iteration after processing the conditional check. In 
this case, whe condition in 'while' loop is :
while ((p =next))
which automatically advances 'p'.

>> +
>> + /* Skip the preceding sub-string "PowerPC " */
>> + p += 8;
>> +
>> + /*Malformed string, does not obey the format 'PowerPC<model> <desc>'*/
>> + if (!(t = strchr(p, ' ')) || (next&& t>= next))
>> + continue;
>> +
>> + if (*p == '\0' || *p == '\n')
>> + continue;
> *p = '\0' presumably is end-of-string -- shouldn't we 'break' here ?

Agree. The reason it works fine in its present form is 'cos when p 
becomes empty, 'next' is empty too, and the loop condition fails which 
causes the do..while loop to terminate.

>> +
>> + if (retcpus) {
> if retcpus is not provided you may want to return -1 earlier
>> + unsigned int len;
>> +
>> + if (VIR_REALLOC_N(cpus, count + 1)< 0)
> virReportOOMError();
>> + goto error;
>> +
>> + if (t)
>> + len = t - p - 1;
> len would remain uninitialized if t is not set, but t at this point must
> be set due to the check above. Remove 'if(t)'.

Will do.

>> +
>> + if (!(cpus[count] = strndup(p, len)))
> virReportOOMError();

I need to explicitly free cpus[] array, hence hadnt invoked this routine.
I'll call this just before return '-1'.
>> + goto error;
>> + }
>> + count++;
>> + } while ((p = next));
>> +
>> + if (retcount)
>> + *retcount = count;
>> + if (retcpus)
>> + *retcpus = cpus;
>> + return 0;
>> +
>> +error:
>> + if (cpus) {
>> + for (i = 0; i< count; i++)
>> + VIR_FREE(cpus[i]);
>> + }
>> + VIR_FREE(cpus);
>> + return -1;
>> +}
>>
>> int
>> qemuCapsProbeCPUModels(const char *qemu,
>> @@ -497,6 +559,8 @@ qemuCapsProbeCPUModels(const char *qemu,
>>
>> if (STREQ(arch, "i686") || STREQ(arch, "x86_64"))
>> parse = qemuCapsParseX86Models;
>> + else if (STREQ(arch, "ppc64"))
>> + parse = qemuCapsParsePPCModels;
>> else {
>> VIR_DEBUG("don't know how to parse %s CPU models", arch);
>> return 0;
>


-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India