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

[Doug Goldstein]

On Mon, Sep 16, 2013 at 12:03 PM, Eric Blake <eblake at redhat.com> 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