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&gt; > > 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.