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

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

[Alberto Garcia]

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 <berto@igalia.com>
---
 block.c | 6 ++++++
 1 file changed, 6 insertions(+)

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);
-- 
2.5.0

Re: [Qemu-devel] [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 <berto@igalia.com> --- 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-----

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

[Alberto Garcia]

On Mon 24 Aug 2015 08:54:56 PM CEST, Max Reitz wrote:

  [bdrv_fill_options()]
> User-specified options should always have precedence over any other
> option. The thing is, we consider the filename to be specified by the
> user.

For user-specified options like the "lazy-refcounts" case that I
mentioned it makes sense, because that's the way the user wants to open
it.

For the image format it sounds counter-intuitive to me: the format is
already set when the file is opened, the user doesn't have a choice
there, or does she?

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

Is that really the case?

The drv parameter of bdrv_open() is being set by the user in a number of
places: qmp_drive_backup(), qmp_drive_mirror(), qmp_change_blockdev()
and qemu-img create.

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

I guess in that case we should change that in all the other places I
mentioned above.

Berto

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3458 bytes --]

On 25.08.2015 09:03, Alberto Garcia wrote:
> On Mon 24 Aug 2015 08:54:56 PM CEST, Max Reitz wrote:
> 
>   [bdrv_fill_options()]
>> User-specified options should always have precedence over any other
>> option. The thing is, we consider the filename to be specified by the
>> user.
> 
> For user-specified options like the "lazy-refcounts" case that I
> mentioned it makes sense, because that's the way the user wants to open
> it.
> 
> For the image format it sounds counter-intuitive to me: the format is
> already set when the file is opened, the user doesn't have a choice
> there, or does she?

The user can always override the option in the filename via the "driver"
option in the QDict. If these options are not available for some reason
(e.g. a backing file name), the filename is generally the only way for
the user to set it (unless there is some special way to set the format,
which there often is...).

>> 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.
> 
> Is that really the case?
> 
> The drv parameter of bdrv_open() is being set by the user in a number of
> places: qmp_drive_backup(), qmp_drive_mirror(), qmp_change_blockdev()
> and qemu-img create.

I consider that a bug. Case in point: Why is it only qemu-img create?
Because all the other qemu-img functions use img_open(), which was was
converted to blk_new_open() in 5bd313266bc5874dae9833be95e5dcfce787f1b7,
whereas qemu-img create uses bdrv_img_create(), which uses the
bdrv_open() @drv parameter for the backing file.

blk_new_open() does not have a @drv parameter, which was intentional.

Hm, now that gets me thinking. I basically said we should just drop the
@drv parameter, and replace it by the "driver" QDict option. That means
that in theory, they should actually be equal. Hm.

So I think what we really want is to drop the @drv parameter from
bdrv_open(), which I tried to do indirectly by introducing
blk_new_open() and hoping to replace most bdrv_open() calls by that
later on. But what we can do in the meantime probably is to apply your
patch, because looking at all the bdrv_open() calls, there is always a
very good reason for setting the @drv option which the user actually
should not override.

Yet another thing is the problem described in the patch's commit
message. Why and how is the driver option inherited by the snapshot? I
cannot see the filename being inherited, because the user always has to
specify it explicitly (both in QMP's blockdev-snapshot-sync and in HMP's
snapshot_blkdev). @options as given to bdrv_open() on the snapshot is
either NULL or contains a node-name. Can you given an example of an
(HMP) command line reproducing the problem?

>> 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.
> 
> I guess in that case we should change that in all the other places I
> mentioned above.

I think that would be something we will have to do eventually anyway.
Your patch would probably solve the issue for now, and we can then drop
it once we have dropped the @drv option from bdrv_open().

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Alberto Garcia]

On Wed 26 Aug 2015 04:53:06 PM CEST, Max Reitz wrote:

> Yet another thing is the problem described in the patch's commit
> message. Why and how is the driver option inherited by the snapshot?

I think you're right and my description was wrong, this happens before
the snapshot is created, when bdrv_image_create() opens the backing
image in order to get its size.

The command line is simply 'snapshot_blkdev virtio0 new.qcow2', the only
requirement is that the original image is opened with driver-specific
options.

Berto