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

Eric Blake eblake at redhat.com
Fri Apr 19 03:19:55 UTC 2013


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.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130418/d5cb21a3/attachment-0001.sig>


More information about the libvir-list mailing list