[PATCH] block: support locking on change medium

[PATCH] block: support locking on change medium

[Joelle van Dyne]

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>
---
 qapi/block.json                | 23 ++++++++++++++++++++++-
 block/monitor/block-hmp-cmds.c |  2 +-
 block/qapi-sysemu.c            | 22 ++++++++++++++++++++++
 ui/cocoa.m                     |  1 +
 4 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/qapi/block.json b/qapi/block.json
index e66666f5c6..35e8e2e191 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -309,6 +309,23 @@
 { 'enum': 'BlockdevChangeReadOnlyMode',
   'data': ['retain', 'read-only', 'read-write'] }
 
+##
+# @BlockdevChangeFileLockingMode:
+#
+# Specifies the new locking mode of a file image passed to the
+# @blockdev-change-medium command.
+#
+# @auto: Use locking if API is available
+#
+# @off: Disable file image locking
+#
+# @on: Enable file image locking
+#
+# Since: 9.2
+##
+{ 'enum': 'BlockdevChangeFileLockingMode',
+  'data': ['auto', 'off', 'on'] }
+
 ##
 # @blockdev-change-medium:
 #
@@ -330,6 +347,9 @@
 # @read-only-mode: change the read-only mode of the device; defaults
 #     to 'retain'
 #
+# @file-locking-mode: change the locking mode of the file image; defaults
+#     to 'auto' (since: 9.2)
+#
 # @force: if false (the default), an eject request through
 #     blockdev-open-tray will be sent to the guest if it has locked
 #     the tray (and the tray will not be opened immediately); if true,
@@ -378,7 +398,8 @@
             'filename': 'str',
             '*format': 'str',
             '*force': 'bool',
-            '*read-only-mode': 'BlockdevChangeReadOnlyMode' } }
+            '*read-only-mode': 'BlockdevChangeReadOnlyMode',
+            '*file-locking-mode': 'BlockdevChangeFileLockingMode' } }
 
 ##
 # @DEVICE_TRAY_MOVED:
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index bdf2eb50b6..ff64020a80 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -1007,5 +1007,5 @@ void hmp_change_medium(Monitor *mon, const char *device, const char *target,
     }
 
     qmp_blockdev_change_medium(device, NULL, target, arg, true, force,
-                               !!read_only, read_only_mode, errp);
+                               !!read_only, read_only_mode, false, 0, errp);
 }
diff --git a/block/qapi-sysemu.c b/block/qapi-sysemu.c
index e4282631d2..8064bdfb3a 100644
--- a/block/qapi-sysemu.c
+++ b/block/qapi-sysemu.c
@@ -311,6 +311,8 @@ void qmp_blockdev_change_medium(const char *device,
                                 bool has_force, bool force,
                                 bool has_read_only,
                                 BlockdevChangeReadOnlyMode read_only,
+                                bool has_file_locking_mode,
+                                BlockdevChangeFileLockingMode file_locking_mode,
                                 Error **errp)
 {
     BlockBackend *blk;
@@ -362,6 +364,26 @@ void qmp_blockdev_change_medium(const char *device,
         qdict_put_str(options, "driver", format);
     }
 
+    if (!has_file_locking_mode) {
+        file_locking_mode = BLOCKDEV_CHANGE_FILE_LOCKING_MODE_AUTO;
+    }
+
+    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();
+    }
+
     medium_bs = bdrv_open(filename, NULL, options, bdrv_flags, errp);
 
     if (!medium_bs) {
diff --git a/ui/cocoa.m b/ui/cocoa.m
index 4c2dd33532..6e73c6e13e 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -1611,6 +1611,7 @@ - (void)changeDeviceMedia:(id)sender
                                        "raw",
                                        true, false,
                                        false, 0,
+                                       false, 0,
                                        &err);
         });
         handleAnyDeviceErrors(err);
-- 
2.41.0

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

[Akihiko Odaki]

On 2024/09/09 10:58, Joelle van Dyne wrote:
> 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>
> ---
>   qapi/block.json                | 23 ++++++++++++++++++++++-
>   block/monitor/block-hmp-cmds.c |  2 +-
>   block/qapi-sysemu.c            | 22 ++++++++++++++++++++++
>   ui/cocoa.m                     |  1 +
>   4 files changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/block.json b/qapi/block.json
> index e66666f5c6..35e8e2e191 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -309,6 +309,23 @@
>   { 'enum': 'BlockdevChangeReadOnlyMode',
>     'data': ['retain', 'read-only', 'read-write'] }
>   
> +##
> +# @BlockdevChangeFileLockingMode:
> +#
> +# Specifies the new locking mode of a file image passed to the
> +# @blockdev-change-medium command.
> +#
> +# @auto: Use locking if API is available
> +#
> +# @off: Disable file image locking
> +#
> +# @on: Enable file image locking
> +#
> +# Since: 9.2
> +##
> +{ 'enum': 'BlockdevChangeFileLockingMode',
> +  'data': ['auto', 'off', 'on'] }

You can use OnOffAuto type instead of defining your own.

> +
>   ##
>   # @blockdev-change-medium:
>   #
> @@ -330,6 +347,9 @@
>   # @read-only-mode: change the read-only mode of the device; defaults
>   #     to 'retain'
>   #
> +# @file-locking-mode: change the locking mode of the file image; defaults
> +#     to 'auto' (since: 9.2)
> +#
>   # @force: if false (the default), an eject request through
>   #     blockdev-open-tray will be sent to the guest if it has locked
>   #     the tray (and the tray will not be opened immediately); if true,
> @@ -378,7 +398,8 @@
>               'filename': 'str',
>               '*format': 'str',
>               '*force': 'bool',
> -            '*read-only-mode': 'BlockdevChangeReadOnlyMode' } }
> +            '*read-only-mode': 'BlockdevChangeReadOnlyMode',
> +            '*file-locking-mode': 'BlockdevChangeFileLockingMode' } }
>   
>   ##
>   # @DEVICE_TRAY_MOVED:
> diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
> index bdf2eb50b6..ff64020a80 100644
> --- a/block/monitor/block-hmp-cmds.c
> +++ b/block/monitor/block-hmp-cmds.c
> @@ -1007,5 +1007,5 @@ void hmp_change_medium(Monitor *mon, const char *device, const char *target,
>       }
>   
>       qmp_blockdev_change_medium(device, NULL, target, arg, true, force,
> -                               !!read_only, read_only_mode, errp);
> +                               !!read_only, read_only_mode, false, 0, errp);
>   }
> diff --git a/block/qapi-sysemu.c b/block/qapi-sysemu.c
> index e4282631d2..8064bdfb3a 100644
> --- a/block/qapi-sysemu.c
> +++ b/block/qapi-sysemu.c
> @@ -311,6 +311,8 @@ void qmp_blockdev_change_medium(const char *device,
>                                   bool has_force, bool force,
>                                   bool has_read_only,
>                                   BlockdevChangeReadOnlyMode read_only,
> +                                bool has_file_locking_mode,
> +                                BlockdevChangeFileLockingMode file_locking_mode,
>                                   Error **errp)
>   {
>       BlockBackend *blk;
> @@ -362,6 +364,26 @@ void qmp_blockdev_change_medium(const char *device,
>           qdict_put_str(options, "driver", format);
>       }
>   
> +    if (!has_file_locking_mode) {
> +        file_locking_mode = BLOCKDEV_CHANGE_FILE_LOCKING_MODE_AUTO;
> +    }
> +
> +    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();
> +    }
> +
>       medium_bs = bdrv_open(filename, NULL, options, bdrv_flags, errp);
>   
>       if (!medium_bs) {
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index 4c2dd33532..6e73c6e13e 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -1611,6 +1611,7 @@ - (void)changeDeviceMedia:(id)sender
>                                          "raw",
>                                          true, false,
>                                          false, 0,
> +                                       false, 0,

This change is irrelevant.

Regards,
Akihiko Odaki

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

[Joelle van Dyne]

On Mon, Sep 9, 2024 at 12:36 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/09/09 10:58, Joelle van Dyne wrote:
> > 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>
> > ---
> >   qapi/block.json                | 23 ++++++++++++++++++++++-
> >   block/monitor/block-hmp-cmds.c |  2 +-
> >   block/qapi-sysemu.c            | 22 ++++++++++++++++++++++
> >   ui/cocoa.m                     |  1 +
> >   4 files changed, 46 insertions(+), 2 deletions(-)
> >
> > diff --git a/qapi/block.json b/qapi/block.json
> > index e66666f5c6..35e8e2e191 100644
> > --- a/qapi/block.json
> > +++ b/qapi/block.json
> > @@ -309,6 +309,23 @@
> >   { 'enum': 'BlockdevChangeReadOnlyMode',
> >     'data': ['retain', 'read-only', 'read-write'] }
> >
> > +##
> > +# @BlockdevChangeFileLockingMode:
> > +#
> > +# Specifies the new locking mode of a file image passed to the
> > +# @blockdev-change-medium command.
> > +#
> > +# @auto: Use locking if API is available
> > +#
> > +# @off: Disable file image locking
> > +#
> > +# @on: Enable file image locking
> > +#
> > +# Since: 9.2
> > +##
> > +{ 'enum': 'BlockdevChangeFileLockingMode',
> > +  'data': ['auto', 'off', 'on'] }
>
> You can use OnOffAuto type instead of defining your own.

This can be done. I had thought that defining a new type makes the
argument more explicit about the meaning.

>
> > +
> >   ##
> >   # @blockdev-change-medium:
> >   #
> > @@ -330,6 +347,9 @@
> >   # @read-only-mode: change the read-only mode of the device; defaults
> >   #     to 'retain'
> >   #
> > +# @file-locking-mode: change the locking mode of the file image; defaults
> > +#     to 'auto' (since: 9.2)
> > +#
> >   # @force: if false (the default), an eject request through
> >   #     blockdev-open-tray will be sent to the guest if it has locked
> >   #     the tray (and the tray will not be opened immediately); if true,
> > @@ -378,7 +398,8 @@
> >               'filename': 'str',
> >               '*format': 'str',
> >               '*force': 'bool',
> > -            '*read-only-mode': 'BlockdevChangeReadOnlyMode' } }
> > +            '*read-only-mode': 'BlockdevChangeReadOnlyMode',
> > +            '*file-locking-mode': 'BlockdevChangeFileLockingMode' } }
> >
> >   ##
> >   # @DEVICE_TRAY_MOVED:
> > diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
> > index bdf2eb50b6..ff64020a80 100644
> > --- a/block/monitor/block-hmp-cmds.c
> > +++ b/block/monitor/block-hmp-cmds.c
> > @@ -1007,5 +1007,5 @@ void hmp_change_medium(Monitor *mon, const char *device, const char *target,
> >       }
> >
> >       qmp_blockdev_change_medium(device, NULL, target, arg, true, force,
> > -                               !!read_only, read_only_mode, errp);
> > +                               !!read_only, read_only_mode, false, 0, errp);
> >   }
> > diff --git a/block/qapi-sysemu.c b/block/qapi-sysemu.c
> > index e4282631d2..8064bdfb3a 100644
> > --- a/block/qapi-sysemu.c
> > +++ b/block/qapi-sysemu.c
> > @@ -311,6 +311,8 @@ void qmp_blockdev_change_medium(const char *device,
> >                                   bool has_force, bool force,
> >                                   bool has_read_only,
> >                                   BlockdevChangeReadOnlyMode read_only,
> > +                                bool has_file_locking_mode,
> > +                                BlockdevChangeFileLockingMode file_locking_mode,
> >                                   Error **errp)
> >   {
> >       BlockBackend *blk;
> > @@ -362,6 +364,26 @@ void qmp_blockdev_change_medium(const char *device,
> >           qdict_put_str(options, "driver", format);
> >       }
> >
> > +    if (!has_file_locking_mode) {
> > +        file_locking_mode = BLOCKDEV_CHANGE_FILE_LOCKING_MODE_AUTO;
> > +    }
> > +
> > +    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();
> > +    }
> > +
> >       medium_bs = bdrv_open(filename, NULL, options, bdrv_flags, errp);
> >
> >       if (!medium_bs) {
> > diff --git a/ui/cocoa.m b/ui/cocoa.m
> > index 4c2dd33532..6e73c6e13e 100644
> > --- a/ui/cocoa.m
> > +++ b/ui/cocoa.m
> > @@ -1611,6 +1611,7 @@ - (void)changeDeviceMedia:(id)sender
> >                                          "raw",
> >                                          true, false,
> >                                          false, 0,
> > +                                       false, 0,
>
> This change is irrelevant.

This change is needed otherwise QEMU will not compile.

>
> Regards,
> Akihiko Odaki

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

[Akihiko Odaki]

On 2024/09/09 23:18, Joelle van Dyne wrote:
> On Mon, Sep 9, 2024 at 12:36 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2024/09/09 10:58, Joelle van Dyne wrote:
>>> 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>
>>> ---
>>>    qapi/block.json                | 23 ++++++++++++++++++++++-
>>>    block/monitor/block-hmp-cmds.c |  2 +-
>>>    block/qapi-sysemu.c            | 22 ++++++++++++++++++++++
>>>    ui/cocoa.m                     |  1 +
>>>    4 files changed, 46 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/qapi/block.json b/qapi/block.json
>>> index e66666f5c6..35e8e2e191 100644
>>> --- a/qapi/block.json
>>> +++ b/qapi/block.json
>>> @@ -309,6 +309,23 @@
>>>    { 'enum': 'BlockdevChangeReadOnlyMode',
>>>      'data': ['retain', 'read-only', 'read-write'] }
>>>
>>> +##
>>> +# @BlockdevChangeFileLockingMode:
>>> +#
>>> +# Specifies the new locking mode of a file image passed to the
>>> +# @blockdev-change-medium command.
>>> +#
>>> +# @auto: Use locking if API is available
>>> +#
>>> +# @off: Disable file image locking
>>> +#
>>> +# @on: Enable file image locking
>>> +#
>>> +# Since: 9.2
>>> +##
>>> +{ 'enum': 'BlockdevChangeFileLockingMode',
>>> +  'data': ['auto', 'off', 'on'] }
>>
>> You can use OnOffAuto type instead of defining your own.
> 
> This can be done. I had thought that defining a new type makes the
> argument more explicit about the meaning.

Speaking of semantics, it would be better to use OnOffAuto to match 
BlockdevOptionsFile's locking property.

We could also argue that having a dedicated type would make this 
consistent with the read-only-mode property, which has such a type, but 
there are other properties that use existing types like str and bool so 
I think it is fine to use an existing type here too.

> 
>>
>>> +
>>>    ##
>>>    # @blockdev-change-medium:
>>>    #
>>> @@ -330,6 +347,9 @@
>>>    # @read-only-mode: change the read-only mode of the device; defaults
>>>    #     to 'retain'
>>>    #
>>> +# @file-locking-mode: change the locking mode of the file image; defaults
>>> +#     to 'auto' (since: 9.2)
>>> +#
>>>    # @force: if false (the default), an eject request through
>>>    #     blockdev-open-tray will be sent to the guest if it has locked
>>>    #     the tray (and the tray will not be opened immediately); if true,
>>> @@ -378,7 +398,8 @@
>>>                'filename': 'str',
>>>                '*format': 'str',
>>>                '*force': 'bool',
>>> -            '*read-only-mode': 'BlockdevChangeReadOnlyMode' } }
>>> +            '*read-only-mode': 'BlockdevChangeReadOnlyMode',
>>> +            '*file-locking-mode': 'BlockdevChangeFileLockingMode' } }
>>>
>>>    ##
>>>    # @DEVICE_TRAY_MOVED:
>>> diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
>>> index bdf2eb50b6..ff64020a80 100644
>>> --- a/block/monitor/block-hmp-cmds.c
>>> +++ b/block/monitor/block-hmp-cmds.c
>>> @@ -1007,5 +1007,5 @@ void hmp_change_medium(Monitor *mon, const char *device, const char *target,
>>>        }
>>>
>>>        qmp_blockdev_change_medium(device, NULL, target, arg, true, force,
>>> -                               !!read_only, read_only_mode, errp);
>>> +                               !!read_only, read_only_mode, false, 0, errp);
>>>    }
>>> diff --git a/block/qapi-sysemu.c b/block/qapi-sysemu.c
>>> index e4282631d2..8064bdfb3a 100644
>>> --- a/block/qapi-sysemu.c
>>> +++ b/block/qapi-sysemu.c
>>> @@ -311,6 +311,8 @@ void qmp_blockdev_change_medium(const char *device,
>>>                                    bool has_force, bool force,
>>>                                    bool has_read_only,
>>>                                    BlockdevChangeReadOnlyMode read_only,
>>> +                                bool has_file_locking_mode,
>>> +                                BlockdevChangeFileLockingMode file_locking_mode,
>>>                                    Error **errp)
>>>    {
>>>        BlockBackend *blk;
>>> @@ -362,6 +364,26 @@ void qmp_blockdev_change_medium(const char *device,
>>>            qdict_put_str(options, "driver", format);
>>>        }
>>>
>>> +    if (!has_file_locking_mode) {
>>> +        file_locking_mode = BLOCKDEV_CHANGE_FILE_LOCKING_MODE_AUTO;
>>> +    }
>>> +
>>> +    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();
>>> +    }
>>> +
>>>        medium_bs = bdrv_open(filename, NULL, options, bdrv_flags, errp);
>>>
>>>        if (!medium_bs) {
>>> diff --git a/ui/cocoa.m b/ui/cocoa.m
>>> index 4c2dd33532..6e73c6e13e 100644
>>> --- a/ui/cocoa.m
>>> +++ b/ui/cocoa.m
>>> @@ -1611,6 +1611,7 @@ - (void)changeDeviceMedia:(id)sender
>>>                                           "raw",
>>>                                           true, false,
>>>                                           false, 0,
>>> +                                       false, 0,
>>
>> This change is irrelevant.
> 
> This change is needed otherwise QEMU will not compile.

I misread the code. I thought of it is a whitespace change for an 
existing line but it is adding a line. This change in cocoa.m is fine.

Regards,
Akihiko Odaki

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

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

[Joelle van Dyne]

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. 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. Meanwhile
this solves a very real problem on macOS which is that trying to
change medium with an ISO which the OS has already mounted will always
fail even when "read-only-mode" is set.

>
> Kevin
>

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

[Kevin Wolf]

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