Re: [libvirt] [PATCHv4 2/4] Allow <source> for type=block to have no dev
On Mon, Sep 16, 2013 at 12:03 PM, Eric Blake <eblake(a)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