qemu-devel
[Top][All Lists]
Advanced

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

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


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH] block: Override the driver in the filename with the user-specified one
Date: Wed, 26 Aug 2015 16:53:06 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0

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: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]