[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 04/17/2013 11:40 PM, Li Zhang wrote:
> From: Li Zhang <zhlcindy 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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]