On Mon, Sep 16, 2013 at 12:03 PM, Eric Blake <eblake@xxxxxxxxxx> wrote: > On 09/09/2013 07:48 PM, Doug Goldstein wrote: >> Currently the XML parser already allows the following syntax: >> <disk type='block' device='cdrom'> >> <source startupPolicy='optional'/> >> <target dev='hda' bus='ide'/> >> <address type='drive' controller='0' bus='0' target='0' unit='0'/> >> </disk> >> >> But it did not support writing out the <source> entry without the actual >> dev value. It would print dev='(null)'. This change allows it to write >> out XML to match the parser. >> --- > >> +++ b/src/conf/domain_conf.c >> @@ -14202,8 +14202,9 @@ virDomainDiskSourceDefFormat(virBufferPtr buf, >> } >> break; >> case VIR_DOMAIN_DISK_TYPE_BLOCK: >> - virBufferEscapeString(buf, " <source dev='%s'", >> - def->src); > > If def->src is NULL, this is a no-op, rather than printing def='(null)'. > Are you sure your commit message is accurate? The (null) was assumed based on my knowledge of gnulib's *printf helpers. So I guess the commit message was wrong. > >> + virBufferAddLit(buf, " <source"); >> + if (def->src) >> + virBufferEscapeString(buf, " dev='%s'", def->src); > > The 'if' is unnecessary; virBufferEscapeString is a no-op for a NULL > argument. However, you are correct that we MUST output "<source" > independently from def->src being NULL, otherwise.. A lot of this file does if checks for NULL of the parameters. It seems like we could clean it up a bit with that condition. > >> if (def->startupPolicy) >> virBufferEscapeString(buf, " startupPolicy='%s'", >> startupPolicy); > > printing the startupPolicy generates invalid XML. Another > (pre-existing) case of an unnecessary 'if'. > So you're saying that <source startupPolicy='optional'/> is invalid XML? Or something else? -- Doug Goldstein -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list