[libvirt] [PATCHv4 2/4] Allow <source> for type=block to have no dev

Eric Blake eblake at redhat.com
Mon Sep 16 21:18:06 UTC 2013


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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130916/9d182dc6/attachment-0001.sig>


More information about the libvir-list mailing list