Re: [Qemu-devel] [RFC] block: convert block_stream command to QAPI

[Luiz Capitulino]

On Tue,  1 Nov 2011 18:08:16 +0000
Stefan Hajnoczi <address@hidden> wrote:

> Move from plain HMP/QMP command implementation to QAPI.  The
> block_stream command takes arguments, returns nothing, may raise errors,
> and will raise a QMP event when the operation completes.
> ---
> Hi Luiz,
> I converted block_stream to QAPI.  The in-tree examples are quite simple so 
> far
> so I hope I've converted this command in a reasonable way.  Please let me know
> what you think:

In general it looks good. The fact that the diff is against your latest series
(vs. against master) didn't help me, so I couldn't test the command.

>  - HMP wrapper does QError/Error housekeeping, is there a nicer way?

Yes. Usually, the HMP command should print the error or do something
about it. More comments below.

>  - To avoid duplicating documentation I move it to qapi-schema.json.

That's ok, the qmp-commands.hx file will die soon.

>  - QMP events seem independent of QAPI, of course raising a monitor event
>    precludes this QAPI code from really being used as an internal C API today.
>    Thoughts?

We'll add an event type soon, so for now it's ok to use the current interface
for that.

Some review comments below.

> 
> CCed qemu-devel because I suspect others are figuring out QAPI/monitor
> integration too.
> 
>  blockdev.c       |   25 ++++++++++++-------------
>  hmp-commands.hx  |    3 +--
>  hmp.c            |   14 ++++++++++++++
>  hmp.h            |    1 +
>  qapi-schema.json |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx  |   49 ++-----------------------------------------------
>  trace-events     |    2 +-
>  7 files changed, 84 insertions(+), 63 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 6d78597..380de39 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -16,6 +16,7 @@
>  #include "qemu-objects.h"
>  #include "sysemu.h"
>  #include "block_int.h"
> +#include "qmp-commands.h"
>  #include "trace.h"
>  
>  static QTAILQ_HEAD(drivelist, DriveInfo) drives = 
> QTAILQ_HEAD_INITIALIZER(drives);
> @@ -817,39 +818,37 @@ static void block_stream_cb(void *opaque, int ret)
>      qobject_decref(obj);
>  }
>  
> -int do_block_stream(Monitor *mon, const QDict *params, QObject **ret_data)
> +void qmp_block_stream(const char *device, bool has_base,
> +                      const char *base, Error **errp)
>  {
> -    const char *device = qdict_get_str(params, "device");
> -    const char *base = qdict_get_try_str(params, "base");
>      BlockDriverState *bs;
>      int ret;
>  
>      bs = bdrv_find(device);
>      if (!bs) {
> -        qerror_report(QERR_DEVICE_NOT_FOUND, device);
> -        return -1;
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
>      }
>  
>      /* Base device not supported */
>      if (base) {
> -        qerror_report(QERR_NOT_SUPPORTED);
> -        return -1;
> +        error_set(errp, QERR_NOT_SUPPORTED);
> +        return;
>      }
>  
>      ret = stream_start(bs, NULL, block_stream_cb, bs);
>      if (ret < 0) {
>          switch (ret) {
>          case -EBUSY:
> -            qerror_report(QERR_DEVICE_IN_USE, device);
> -            return -1;
> +            error_set(errp, QERR_DEVICE_IN_USE, device);
> +            return;
>          default:
> -            qerror_report(QERR_NOT_SUPPORTED);
> -            return -1;
> +            error_set(errp, QERR_NOT_SUPPORTED);
> +            return;
>          }
>      }
>  
> -    trace_do_block_stream(bs, bs->job);
> -    return 0;
> +    trace_qmp_block_stream(bs, bs->job);
>  }
>  
>  static BlockJob *find_block_job(const char *device)
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index a8b83f0..b2af867 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -75,8 +75,7 @@ ETEXI
>          .args_type  = "device:B,base:s?",
>          .params     = "device [base]",
>          .help       = "copy data from a backing file into a block device",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_block_stream,
> +        .mhandler.cmd = hmp_block_stream,
>      },
>  
>  STEXI
> diff --git a/hmp.c b/hmp.c
> index 34416fc..e65510d 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -114,3 +114,17 @@ void hmp_system_powerdown(Monitor *mon, const QDict 
> *qdict)
>  {
>      qmp_system_powerdown(NULL);
>  }
> +
> +void hmp_block_stream(Monitor *mon, const QDict *qdict)
> +{
> +    Error *error = NULL;
> +    const char *device = qdict_get_str(qdict, "device");
> +    const char *base = qdict_get_try_str(qdict, "base");
> +
> +    qmp_block_stream(device, base != NULL, base, &error);
> +
> +    if (error_is_set(&error)) {
> +        qerror_report_err(error);
> +    }
> +    error_free(error);

If all you want here is to inform users about the error, you could do
something like this:

if (error_is_set(&error)) {
    monitor_printf(mon, "%s\n", error_get_pretty(error));
    error_free(error);
    return;
}

> +}
> diff --git a/hmp.h b/hmp.h
> index 92433cf..779a601 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -27,5 +27,6 @@ void hmp_quit(Monitor *mon, const QDict *qdict);
>  void hmp_stop(Monitor *mon, const QDict *qdict);
>  void hmp_system_reset(Monitor *mon, const QDict *qdict);
>  void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
> +void hmp_block_stream(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5922c4a..5c3f898 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -271,3 +271,56 @@
>  #        prompting the user in some way.
>  ##
>  { 'command': 'system_powerdown' }
> +
> +##
> +# @block_stream:
> +#
> +# Copy data from a backing file into a block device.
> +#
> +# The block streaming operation is performed in the background until the 
> entire
> +# backing file has been copied.  This command returns immediately once 
> streaming
> +# has started.  The status of ongoing block streaming operations can be 
> checked
> +# with query-block-jobs.  The operation can be stopped before it has 
> completed
> +# using the block_job_cancel command.
> +#
> +# If a base file is specified then sectors are not copied from that base 
> file and
> +# its backing chain.  When streaming completes the image file will have the 
> base
> +# file as its backing file.  This can be used to stream a subset of the 
> backing
> +# file chain instead of flattening the entire image.
> +#
> +# On successful completion the image file is updated to drop the backing 
> file.
> +#
> +# Arguments:
> +#
> +# @device: device name
> +# @base:   common backing file
> +#
> +# Errors:
> +#
> +# DeviceInUse:    streaming is already active on this device
> +# DeviceNotFound: device name is invalid
> +# NotSupported:   image streaming is not supported by this device
> +#
> +# Events:
> +#
> +# On completion the BLOCK_JOB_COMPLETED event is raised with the following
> +# fields:
> +#
> +# - type:     job type ("stream" for image streaming, json-string)
> +# - device:   device name (json-string)
> +# - len:      maximum progress value (json-int)
> +# - offset:   current progress value (json-int)
> +# - speed:    rate limit, bytes per second (json-int)
> +# - error:    error message (json-string, only on error)
> +#
> +# The completion event is raised both on success and on failure.  On
> +# success offset is equal to len.  On failure offset and len can be
> +# used to indicate at which point the operation failed.
> +#
> +# On failure the error field contains a human-readable error message.  There 
> are
> +# no semantics other than that streaming has failed and clients should not 
> try
> +# to interpret the error string.
> +#
> +# Since: 1.1
> +##
> +{ 'command': 'block_stream', 'data': { 'device': 'str', '*base': 'str' } }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 6e8207b..79ce09a 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -689,58 +689,13 @@ EQMP
>          .args_type  = "device:B,base:s?",
>          .params     = "device [base]",
>          .user_print = monitor_user_noop,

You can drop params and user_print. The members args_type and name are
still needed though. I will fix this soon (but not for 1.0, of course).

> -        .mhandler.cmd_new = do_block_stream,
> +        .mhandler.cmd_new = qmp_marshal_input_block_stream,
>      },
>  
>  SQMP
>  block_stream
>  ------------
> -
> -Copy data from a backing file into a block device.
> -
> -The block streaming operation is performed in the background until the entire
> -backing file has been copied.  This command returns immediately once 
> streaming
> -has started.  The status of ongoing block streaming operations can be checked
> -with query-block-jobs.  The operation can be stopped before it has completed
> -using the block_job_cancel command.
> -
> -If a base file is specified then sectors are not copied from that base file 
> and
> -its backing chain.  When streaming completes the image file will have the 
> base
> -file as its backing file.  This can be used to stream a subset of the backing
> -file chain instead of flattening the entire image.
> -
> -On successful completion the image file is updated to drop the backing file.
> -
> -Arguments:
> -
> -- device: device name (json-string)
> -- base:   common backing file (json-string, optional)
> -
> -Errors:
> -
> -DeviceInUse:    streaming is already active on this device
> -DeviceNotFound: device name is invalid
> -NotSupported:   image streaming is not supported by this device
> -
> -Events:
> -
> -On completion the BLOCK_JOB_COMPLETED event is raised with the following
> -fields:
> -
> -- type:     job type ("stream" for image streaming, json-string)
> -- device:   device name (json-string)
> -- len:      maximum progress value (json-int)
> -- offset:   current progress value (json-int)
> -- speed:    rate limit, bytes per second (json-int)
> -- error:    error message (json-string, only on error)
> -
> -The completion event is raised both on success and on failure.  On
> -success offset is equal to len.  On failure offset and len can be
> -used to indicate at which point the operation failed.
> -
> -On failure the error field contains a human-readable error message.  There 
> are
> -no semantics other than that streaming has failed and clients should not try
> -to interpret the error string.
> +See qapi-schema.json for documentation.
>  
>  Examples:
>  
> diff --git a/trace-events b/trace-events
> index 39023cc..8b4cdc3 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -79,7 +79,7 @@ stream_start(void *bs, void *base, void *s, void *co, void 
> *opaque) "bs %p base
>  block_job_cancel_cb(void *cancel_data, void *opaque) "cancel_data %p opaque 
> %p"
>  do_block_job_cancel(void *job, void *cancel_data, void *opaque) "job %p 
> cancel_data %p opaque %p"
>  block_stream_cb(void *bs, void *job, int ret) "bs %p job %p ret %d"
> -do_block_stream(void *bs, void *job) "bs %p job %p"
> +qmp_block_stream(void *bs, void *job) "bs %p job %p"
>  
>  # hw/virtio-blk.c
>  virtio_blk_req_complete(void *req, int status) "req %p status %d"