Re: [Qemu-devel] [PATCH 13/15] qapi: introduce drive-change (v2)

[Luiz Capitulino]

On Fri, 2 Sep 2011 18:06:13 -0300
Luiz Capitulino <address@hidden> wrote:

> On Fri,  2 Sep 2011 12:34:56 -0500
> Anthony Liguori <address@hidden> wrote:
> 
> > A new QMP only command to change the blockdev associated with a block 
> > device.
> > The semantics of change right now are just plain scary.  This command 
> > introduces
> > sane semantics.  For more details, see the documentation in the schema file.
> > 
> > Signed-off-by: Anthony Liguori <address@hidden>
> > ---
> > v1 -> v2
> >  - Rename command to drive-change (Kevin)
> > ---
> >  blockdev.c       |  108 
> > +++++++++++++++++++++++++++++++++++++++++++++++++++---
> >  qapi-schema.json |   30 +++++++++++++++
> >  qmp-commands.hx  |    8 ++++
> >  3 files changed, 140 insertions(+), 6 deletions(-)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index 07eafce..cd338ed 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -688,12 +688,101 @@ void qmp_block_passwd(const char *device, const char 
> > *password, Error **err)
> >      qmp_set_blockdev_password(device, password, err);
> >  }
> >  
> > +static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char 
> > *filename,
> > +                                    int bdrv_flags, BlockDriver *drv,
> > +                                    const char *password, Error **errp)
> > +{
> > +    if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) {
> > +        error_set(errp, QERR_OPEN_FILE_FAILED, filename);
> > +        return;
> > +    }
> > +
> > +    if (bdrv_key_required(bs)) {
> > +        if (password) {
> > +            if (bdrv_set_key(bs, password) < 0) {
> > +                error_set(errp, QERR_INVALID_PASSWORD);
> > +            }
> > +        } else {
> > +            error_set(errp, QERR_DEVICE_ENCRYPTED, 
> > bdrv_get_device_name(bs),
> > +                      bdrv_get_encrypted_filename(bs));
> > +        }
> > +    } else if (password) {
> > +        error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, 
> > bdrv_get_device_name(bs));
> > +    }
> > +}
> > +
> > +void qmp_drive_change(const char *device, const char *filename,
> > +                      bool has_format, const char *format,
> > +                      bool has_password, const char *password,
> > +                      Error **errp)
> > +{
> > +    BlockDriverState *bs, *bs1;
> > +    BlockDriver *drv = NULL;
> > +    int bdrv_flags;
> > +    Error *err = NULL;
> > +    bool probed_raw = false;
> > +
> > +    bs = bdrv_find(device);
> > +    if (!bs) {
> > +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> > +        return;
> > +    }
> > +
> > +    if (has_format) {
> > +        drv = bdrv_find_whitelisted_format(format);
> > +        if (!drv) {
> > +            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> > +            return;
> > +        }
> > +    }
> > +
> > +    bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
> > +    bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
> > +
> > +    if (!has_password) {
> > +        password = NULL;
> > +    }
> > +
> > +    /* Try to open once with a temporary block device to make sure that
> > +     * the disk isn't encrypted and we lack a key.  This also helps keep
> > +     * this operation as a transaction.  That is, if we fail, we try very
> > +     * hard to make sure that the state is the same as before the operation
> > +     * was started.
> > +     */
> > +    bs1 = bdrv_new("");
> > +    qmp_bdrv_open_encrypted(bs1, filename, bdrv_flags, drv, password, 
> > &err);
> > +    if (!has_format && bs1->drv->unsafe_probe) {
> > +        probed_raw = true;
> > +    }
> > +    bdrv_delete(bs1);
> > +
> > +    if (err) {
> > +        error_propagate(errp, err);
> > +        return;
> > +    }
> > +
> > +    if (probed_raw) {
> > +        /* We will not auto probe raw files. */
> > +        error_set(errp, QERR_MISSING_PARAMETER, "format");
> > +        return;
> > +    }
> 
> This has to be noted in the documentation.
> 
> > +
> > +    eject_device(bs, 0, &err);
> > +    if (err) {
> > +        error_propagate(errp, err);
> > +        return;
> > +    }
> 
> Hm. I'm not wondering if this command isn't actually overloading two

s/not/now

> distinct operations: eject and insert. Maybe what we want here is 
> drive-insert?
> 
> > +
> > +    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, password, errp);
> > +}
> > +
> >  int do_change_block(Monitor *mon, const char *device,
> >                      const char *filename, const char *fmt)
> >  {
> >      BlockDriverState *bs;
> >      BlockDriver *drv = NULL;
> >      int bdrv_flags;
> > +    Error *err = NULL;
> >  
> >      bs = bdrv_find(device);
> >      if (!bs) {
> > @@ -707,16 +796,23 @@ int do_change_block(Monitor *mon, const char *device,
> >              return -1;
> >          }
> >      }
> > -    if (eject_device(bs, 0, NULL) < 0) {
> > -        return -1;
> > -    }
> > +
> >      bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
> >      bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
> > -    if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) {
> > -        qerror_report(QERR_OPEN_FILE_FAILED, filename);
> > +
> > +    eject_device(bs, 0, &err);
> > +    if (err) {
> > +        qerror_report_err(err);
> > +        return -1;
> > +    }
> > +
> > +    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, &err);
> > +    if (err) {
> > +        qerror_report_err(err);
> >          return -1;
> >      }
> > -    return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
> > +
> > +    return 0;
> >  }
> >  
> >  int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 0c6c9b8..cbb5bf1 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -124,3 +124,33 @@
> >  #         settings may change after executing this command.
> >  ##
> >  { 'command': 'change-vnc-listen', 'data': {'target': 'str'} }
> > +
> > +##
> > +# @drive-change:
> > +#
> > +# This is the preferred interface for changing a block device.
> > +#
> > +# @device:   The block device name.
> > +#
> > +# @filename: The new filename for the block device.  This may contain 
> > options
> > +#            encoded in a format specified by @format.
> > +#
> > +# @format:   #optional The format to use open the block device
> > +#
> > +# @password: #optional The password to use if the block device is encrypted
> > +#
> > +# Returns:  Nothing on success.
> > +#          If @device is not a valid block device, DeviceNotFound
> > +#          If @format is not a valid block format, InvalidBlockFormat
> > +#          If @filename is encrypted and @password isn't supplied,
> > +#            DeviceEncrypted.  The call should be repeated with @password
> > +#            supplied.
> > +#          If @format is not specified and @filename is a format that 
> > cannot
> > +#            be safely probed, MissingParameter.
> > +#          If @filename cannot be opened, OpenFileFailed
> > +#
> > +# Since: 1.0
> > +##
> > +{ 'command': 'drive-change',
> > +  'data': {'device': 'str', 'filename': 'str', '*format': 'str',
> > +           '*password': 'str'} }
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index 5cab212..623f158 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -121,6 +121,14 @@ EQMP
> >          .mhandler.cmd_new = do_change,
> >      },
> >  
> > +    {
> > +        .name       = "drive-change",
> > +        .args_type  = "device:B,filename:F,format:s?password:s?",
> > +        .params     = "device filename [format] [password]",
> > +        .help       = "change a removable medium, optional format",
> > +        .mhandler.cmd_new = qmp_marshal_input_drive_change,
> > +    },
> > +
> >  SQMP
> >  change
> >  ------
>