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