Re: [PATCH] block: support locking on change medium

[Kevin Wolf <kwolf@redhat.com>]

Am 09.09.2024 um 16:25 hat Joelle van Dyne geschrieben:
> On Mon, Sep 9, 2024 at 2:56 AM Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > Am 09.09.2024 um 03:58 hat Joelle van Dyne geschrieben:
> > > New optional argument for 'blockdev-change-medium' QAPI command to allow
> > > the caller to specify if they wish to enable file locking.
> > >
> > > Signed-off-by: Joelle van Dyne <j@getutm.app>
> >
> > I feel once you need to control such details of the backend, you should
> > really use a separate 'blockdev-add' commannd.
> >
> > If it feels a bit too cumbersome to send explicit commands to open the
> > tray, remove the medium, insert the new medium referencing the node you
> > added with 'blockdev-add' and then close the tray again, I can
> > understand. Maybe what we should do is extend 'blockdev-change-medium'
> > so that it doesn't only accept a filename to specify the new images, but
> > alternatively also a node-name.
> >
> > > +    switch (file_locking_mode) {
> > > +    case BLOCKDEV_CHANGE_FILE_LOCKING_MODE_AUTO:
> > > +        break;
> > > +
> > > +    case BLOCKDEV_CHANGE_FILE_LOCKING_MODE_OFF:
> > > +        qdict_put_str(options, "file.locking", "off");
> > > +        break;
> > > +
> > > +    case BLOCKDEV_CHANGE_FILE_LOCKING_MODE_ON:
> > > +        qdict_put_str(options, "file.locking", "on");
> > > +        break;
> > > +
> > > +    default:
> > > +        abort();
> > > +    }
> >
> > Using "file.locking" makes assumptions about what the passed filename
> > string would result in. There is nothing that guarantees that the block
> > driver even has a "file" child, or that the "file" child is referring
> > to a file-posix driver rather than using a different protocol or being a
> > filter driver above yet another node. It also doesn't consider backing
> > files and other non-primary children of the opened node.
> >
> > So this is not correct, and I don't think there is any realistic way of
> > making it correct with this approach.
> 
> The existence of "filename" already makes this assumption that the
> input is a file child.

No. Try using something like "blkdebug::image.iso" or "nbd://localhost".

In the former case, you get another layer and the "file" child would be
a blkdebug node. To turn off locking on the file-posix block driver
you'd need to set "file.file.locking" in this case.

And the latter doesn't have any file-posix involved, it goes straight to
the NBD block driver.

> While I agree with you that there are better ways to solve this
> problem, ultimately "blockdev-change-medium" will have to be
> deprecated when this hypothetical "better" way of referencing a node
> added with blockdev-add is introduced.

This is not a hypothetical better way. It is how you can achieve this
today.

Extending blockdev-change-medium to alternatively accept a node-name
would just introduce a convenience shortcut (as is the whole command)
that doesn't require you to send four QMP commands (blockdev-open-tray,
blockdev-remove-medium, blockdev-insert-medium and blockdev-close-tray).

There is no need to deprecate blockdev-change-medium with a filename any
more than there is a need to deprecate -hda on the command line. We
could do it because there are alternatives that provide a strict
superset of functionality, but we don't have to.

Kevin