On 04/17/2013 11:40 PM, Li Zhang wrote:
> From: Li Zhang <zhlcindy@xxxxxxxxxxxxxxxxxx>
>
> 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
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list