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

[Kevin Wolf]

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.

Kevin