Re: [Qemu-devel] [PATCH v3 5/9] qmp: add block_stream command

[Luiz Capitulino]

On Tue, 13 Dec 2011 13:52:27 +0000
Stefan Hajnoczi <address@hidden> wrote:

> Add the block_stream command, which starts copy backing file contents
> into the image file.  Later patches add control over the background copy
> speed, cancelation, and querying running streaming operations.

Please also mention that you're adding an event, otherwise it comes as a
surprise to the reviewer.

When we talked about this implementation (ie. having events, cancellation etc)
I thought you were going to do something very specific to the streaming api,
like migration. However, you ended up adding async QMP support to the block
layer.

I have the impression this could be generalized a bit more and be moved to
QMP instead (and I strongly feel that's what we should do).

There's only one problem: we are waiting for the new QMP server to work on
that. I hope to have it integrated by the end of this month, but it might
be possible to add async support to the current server if waiting is not
an option.

> 
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
>  blockdev.c       |   68 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hmp-commands.hx  |   13 ++++++++++
>  hmp.c            |   14 +++++++++++
>  hmp.h            |    1 +
>  monitor.c        |    3 ++
>  monitor.h        |    1 +
>  qapi-schema.json |   53 ++++++++++++++++++++++++++++++++++++++++++
>  qerror.c         |    4 +++
>  qerror.h         |    3 ++
>  qmp-commands.hx  |   18 ++++++++++++++
>  trace-events     |    4 +++
>  11 files changed, 182 insertions(+), 0 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index dbf0251..08355cf 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -13,8 +13,11 @@
>  #include "qerror.h"
>  #include "qemu-option.h"
>  #include "qemu-config.h"
> +#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);
>  
> @@ -887,3 +890,68 @@ int do_block_resize(Monitor *mon, const QDict *qdict, 
> QObject **ret_data)
>  
>      return 0;
>  }
> +
> +static QObject *qobject_from_block_job(BlockJob *job)
> +{
> +    return qobject_from_jsonf("{ 'type': %s,"
> +                              "'device': %s,"
> +                              "'len': %" PRId64 ","
> +                              "'offset': %" PRId64 ","
> +                              "'speed': %" PRId64 " }",
> +                              job->job_type->job_type,
> +                              bdrv_get_device_name(job->bs),
> +                              job->len,
> +                              job->offset,
> +                              job->speed);
> +}
> +
> +static void block_stream_cb(void *opaque, int ret)
> +{
> +    BlockDriverState *bs = opaque;
> +    QObject *obj;
> +
> +    trace_block_stream_cb(bs, bs->job, ret);
> +
> +    assert(bs->job);
> +    obj = qobject_from_block_job(bs->job);
> +    if (ret < 0) {
> +        QDict *dict = qobject_to_qdict(obj);
> +        qdict_put(dict, "error", qstring_from_str(strerror(-ret)));
> +    }
> +
> +    monitor_protocol_event(QEVENT_BLOCK_JOB_COMPLETED, obj);
> +    qobject_decref(obj);
> +}
> +
> +void qmp_block_stream(const char *device, bool has_base,
> +                      const char *base, Error **errp)
> +{
> +    BlockDriverState *bs;
> +    int ret;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    /* Base device not supported */
> +    if (base) {
> +        error_set(errp, QERR_NOT_SUPPORTED);
> +        return;
> +    }

Is this a future feature? In this case I'd rather drop the argument for
now and add it later. The only detail is that we haven't reached consensus
if it will be required to add a new command for that or if we'll be able
to just extend existing commands.

> +
> +    ret = stream_start(bs, NULL, block_stream_cb, bs);
> +    if (ret < 0) {
> +        switch (ret) {
> +        case -EBUSY:
> +            error_set(errp, QERR_DEVICE_IN_USE, device);
> +            return;
> +        default:
> +            error_set(errp, QERR_NOT_SUPPORTED);
> +            return;
> +        }
> +    }
> +
> +    trace_qmp_block_stream(bs, bs->job);
> +}
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 79a9195..c0de41c 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -70,6 +70,19 @@ but should be used with extreme caution.  Note that this 
> command only
>  resizes image files, it can not resize block devices like LVM volumes.
>  ETEXI
>  
> +    {
> +        .name       = "block_stream",
> +        .args_type  = "device:B,base:s?",
> +        .params     = "device [base]",
> +        .help       = "copy data from a backing file into a block device",
> +        .mhandler.cmd = hmp_block_stream,
> +    },
> +
> +STEXI
> address@hidden block_stream
> address@hidden block_stream
> +Copy data from a backing file into a block device.
> +ETEXI
>  
>      {
>          .name       = "eject",
> diff --git a/hmp.c b/hmp.c
> index dfab7ad..8b7d7d4 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -531,3 +531,17 @@ void hmp_cpu(Monitor *mon, const QDict *qdict)
>          monitor_printf(mon, "invalid CPU index\n");
>      }
>  }
> +
> +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) {
> +        monitor_printf(mon, "%s\n", error_get_pretty(error));
> +        error_free(error);
> +    }

There's a hmp_handle_error() helper recently merged that does exactly that.

> +}
> diff --git a/hmp.h b/hmp.h
> index 4422578..3cb6fe5 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -37,5 +37,6 @@ 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_cpu(Monitor *mon, const QDict *qdict);
> +void hmp_block_stream(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/monitor.c b/monitor.c
> index 1be222e..a33218c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -479,6 +479,9 @@ void monitor_protocol_event(MonitorEvent event, QObject 
> *data)
>          case QEVENT_SPICE_DISCONNECTED:
>              event_name = "SPICE_DISCONNECTED";
>              break;
> +        case QEVENT_BLOCK_JOB_COMPLETED:
> +            event_name = "BLOCK_JOB_COMPLETED";
> +            break;
>          default:
>              abort();
>              break;
> diff --git a/monitor.h b/monitor.h
> index e76795f..d9b07dc 100644
> --- a/monitor.h
> +++ b/monitor.h
> @@ -35,6 +35,7 @@ typedef enum MonitorEvent {
>      QEVENT_SPICE_CONNECTED,
>      QEVENT_SPICE_INITIALIZED,
>      QEVENT_SPICE_DISCONNECTED,
> +    QEVENT_BLOCK_JOB_COMPLETED,
>      QEVENT_MAX,
>  } MonitorEvent;
>  
> diff --git a/qapi-schema.json b/qapi-schema.json
> index fbbdbe0..65308d2 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -901,3 +901,56 @@
>  # Notes: Do not use this command.
>  ##
>  { 'command': 'cpu', 'data': {'index': 'int'} }
> +
> +##
> +# @block_stream:

Command names should be verbs and please use an hyphen.

> +#
> +# 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.

Nice doc.

> +#
> +# 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

The convention used in this file to document errors is different.

> +#
> +# 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.

Events should be documented in QMP/qmp-events.txt.

> +#
> +# Since: 1.1
> +##
> +{ 'command': 'block_stream', 'data': { 'device': 'str', '*base': 'str' } }
> diff --git a/qerror.c b/qerror.c
> index 14a1d59..605381a 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -174,6 +174,10 @@ static const QErrorStringTable qerror_table[] = {
>          .desc      = "No '%(bus)' bus found for device '%(device)'",
>      },
>      {
> +        .error_fmt = QERR_NOT_SUPPORTED,
> +        .desc      = "Not supported",

We have QERR_UNSUPPORTED already.

> +    },
> +    {
>          .error_fmt = QERR_OPEN_FILE_FAILED,
>          .desc      = "Could not open '%(filename)'",
>      },
> diff --git a/qerror.h b/qerror.h
> index 560d458..b5c0cfe 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -147,6 +147,9 @@ QError *qobject_to_qerror(const QObject *obj);
>  #define QERR_NO_BUS_FOR_DEVICE \
>      "{ 'class': 'NoBusForDevice', 'data': { 'device': %s, 'bus': %s } }"
>  
> +#define QERR_NOT_SUPPORTED \
> +    "{ 'class': 'NotSupported', 'data': {} }"
> +
>  #define QERR_OPEN_FILE_FAILED \
>      "{ 'class': 'OpenFileFailed', 'data': { 'filename': %s } }"
>  
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 94da2a8..8c1c934 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -683,6 +683,24 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "block_stream",
> +        .args_type  = "device:B,base:s?",
> +        .mhandler.cmd_new = qmp_marshal_input_block_stream,
> +    },

You can drop the docs below. The above entry is needed because the current
QMP server still uses it (which causes duplication with the schema file,
this will be fixed soon).

> +
> +SQMP
> +block_stream
> +------------
> +See qapi-schema.json for documentation.
> +
> +Examples:
> +
> +-> { "execute": "block_stream", "arguments": { "device": "virtio0" } }
> +<- { "return":  {} }
> +
> +EQMP
> +
> +    {
>          .name       = "blockdev-snapshot-sync",
>          .args_type  = "device:B,snapshot-file:s?,format:s?",
>          .params     = "device [new-image-file] [format]",
> diff --git a/trace-events b/trace-events
> index 4efcd95..6c1eec2 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -73,6 +73,10 @@ bdrv_co_copy_on_readv(void *bs, int64_t sector_num, int 
> nb_sectors, int64_t clus
>  stream_one_iteration(void *s, int64_t sector_num, int nb_sectors, int 
> is_allocated) "s %p sector_num %"PRId64" nb_sectors %d is_allocated %d"
>  stream_start(void *bs, void *base, void *s, void *co, void *opaque) "bs %p 
> base %p s %p co %p opaque %p"
>  
> +# blockdev.c
> +block_stream_cb(void *bs, void *job, int ret) "bs %p job %p ret %d"
> +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"
>  virtio_blk_rw_complete(void *req, int ret) "req %p ret %d"