| Message ID | 1440421537-26477-1-git-send-email-berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
-----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-----
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
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
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
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);
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(+)