On 09/16/2013 02:20 PM, Doug Goldstein wrote:
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.
virBufferEscapeString() intentionally avoids calling printf for a NULL string - but had we reached printf, you are right that glibc would print "(null)" (and other libc's might SEGV).
+ 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.
Yeah, probably a lot of those places that we can clean. So far, we've been doing it piece-wise as we come across places that are touched for other reasons.
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?
For NULL def->src and non-null startupPolicy, the old code would produce: " startupPolicy='...'" and the new code produces "<source startupPolicy='...'" Basically, your patch separates the data that must always be printed ("<source") from the data that is conditional (" dev='...'"). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org