On Tue, 1 Nov 2011 18:08:16 +0000
Stefan Hajnoczi <stefa...@linux.vnet.ibm.com> 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"
