[libvirt] [PATCH v4 1/2 RESEND] Add NVRAM device

[Li Zhang]

On 2013年04月19日 11:19, Eric Blake wrote:
> On 04/17/2013 11:40 PM, Li Zhang wrote:
>> From: Li Zhang <zhlcindy at linux.vnet.ibm.com>
>>
>> For pSeries guest in QEMU, NVRAM is one kind of spapr-vio device.
>> Users are allowed to specify spapr-vio devices'address.
>> But NVRAM is not supported in libvirt. So this patch is to
>> add NVRAM device to allow users to specify its address.
>>
>> In QEMU, NVRAM device's address is specified by
>>   "-global spapr-nvram.reg=xxxxx".
>>
>>   
>> +static virDomainNVRAMDefPtr
>> +virDomainNVRAMDefParseXML(const xmlNodePtr node,
>> +                               unsigned int flags)
> Alignment.  The 'c' of const and 'u' of unsigned should be in the same
> column:
>
> virDomainNVRAMDefParseXML(const xmlNodePtr node,
>                            unsigned int flags)
>
>> +{
>> +   virDomainNVRAMDefPtr def;
>> +
>> +    if (VIR_ALLOC(def) < 0) {
>> +        virReportOOMError();
>> +        return NULL;
>> +    }
>> +
>> +    if ( virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0 )
> Style: no spaces inside () and the expression it contains.  This should be:
>
> if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0)
>
>> +        return NULL;
> Memory leak.  If the parse fails, you leaked def.
>
>> @@ -14159,6 +14216,21 @@ virDomainMemballoonDefFormat(virBufferPtr buf,
>>   }
>>   
>>   static int
>> +virDomainNVRAMDefFormat(virBufferPtr buf,
>> +                             virDomainNVRAMDefPtr def,
>> +                             unsigned int flags)
> Another case of incorrect indentation.
>
>> +{
>> +    virBufferAsprintf(buf, "    <nvram>\n");
> Use virBufferAddLit when there is nothing to be formatted (reserve
> virBufferAsprintf for use of "%" sequences).
>
>> +    if (virDomainDeviceInfoIsSet(&def->info, flags))
>> +        if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
> You could combine these two into one:
>
> if (virDomainDeviceInfoIsSet(&def->info, flags) &&
>      virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
>
> I know Dan acked this, but since you have a memory leak, and since Osier
> suggested improvements for 2/2, please send a v5.
>

Sure.