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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list