Re: [libvirt] [PATCH v4 1/2 RESEND] Add NVRAM device
On 04/17/2013 11:40 PM, Li Zhang wrote:
From: Li Zhang <zhlcindy(a)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
Attachments:
- signature.asc (application/pgp-signature — 621 bytes)