Re: [Qemu-block] [PATCH] block: Override the driver in the filename with the user-specified one

[Max Reitz]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 24.08.2015 15:05, Alberto Garcia wrote:
> If an image is opened with driver-specific options then attempting
> to use snapshot_blkdev will fail with "Driver specified twice".
> 
> The reason is that bs->filename is replaced with a full JSON object
> by bdrv_refresh_filename() when such options are present:
> 
> -drive if=virtio,file=hd0.qcow2,lazy-refcounts=on
> 
> (qemu) info block virtio0: json:{"lazy-refcounts": "on", "driver":
> "qcow2", "file": {"driver": "file", "filename": "hd0.qcow2"}}
> (qcow2)
> 
> A snapshot of that image will try to inherit its options, and that 
> includes parsing its filename when it is in the "json:" format.
> 
> Since the JSON object includes a driver name, it will clash with 
> the one requested by the user in snapshot_blkdev, producing the 
> aforementioned error.
> 
> Signed-off-by: Alberto Garcia <address@hidden> --- block.c | 6
> ++++++ 1 file changed, 6 insertions(+)

User-specified options should always have precedence over any other
option. The thing is, we consider the filename to be specified by the
user. So it is actually correct that this option overrides the @drv
parameter given to bdrv_open(), because that cannot be set by the user
and is always set by qemu internally.

The only way the user can set the driver (other than using a JSON
filename) is by setting the "driver" option, and this will actually
have precedence over the filename (see the comment at the end of this
hunk), which is intended.

So I think the problem here is not in bdrv_fill_options(), but rather
in blockdev.c:external_snapshot_prepare(). This function should not
pass the driver as the @drv parameter to bdrv_open(), but rather set
the "driver" option in @options in order to mark this a user-specified
option.

Max

> diff --git a/block.c b/block.c index d088ee0..b09de04 100644 ---
> a/block.c +++ b/block.c @@ -1014,6 +1014,12 @@ static int
> bdrv_fill_options(QDict **options, const char **pfilename, return
> -EINVAL; }
> 
> +        /* We shouldn't use the driver from the filename if there
> is +         * one explicitly specified already */ +        if
> (drv) { +            qdict_del(json_options, "driver"); +        } 
> + /* Options given in the filename have lower priority than
> options * specified directly */ qdict_join(*options, json_options,
> false);
> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJV22iAAAoJEDuxQgLoOKytPbUIAKLTo4wxxMSuYzASOONbnx7g
PVrY7tcCGq2OEvAKirwufCJrGRQUvqqPci4k+zJzdtK2vAsxS6tYawY5bND00DBh
wjQQIG7B16+ehEMVoyeLS7aslhaBbLMltcbQu7emS3vBMhQ1hUOkaP8IWxtQKRnE
/2Afvi7f7pQ0gbCk0K13g2uGbHL0SE4GVCsGoAiFvIwNoFovHJ1chZlJQiMrAlMs
40HPaJapjjszDNc0mZ5arAoMEg9cZAd+THdJw11RAHdt3ty5/L048Qg3ui531i0O
HPhT8qfhZA/pz6W88vd8wR6dbcSqyB6+uFAFjnV1/Cduj6KL4aZuPlHLdyU+Jg0=
=zsDu
-----END PGP SIGNATURE-----