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

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

 



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?

> +            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..

>              if (def->startupPolicy)
>                  virBufferEscapeString(buf, " startupPolicy='%s'",
>                                        startupPolicy);

printing the startupPolicy generates invalid XML.  Another
(pre-existing) case of an unnecessary 'if'.

-- 
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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]