[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]