* [Qemu-devel] [RFC] block: convert block_stream command to QAPI
@ 2011-11-01 18:08 Stefan Hajnoczi
2011-11-03 15:51 ` Luiz Capitulino
0 siblings, 1 reply; 3+ messages in thread
From: Stefan Hajnoczi @ 2011-11-01 18:08 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: qemu-devel, Stefan Hajnoczi
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:
- HMP wrapper does QError/Error housekeeping, is there a nicer way?
- To avoid duplicating documentation I move it to qapi-schema.json.
- 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?
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);
+}
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,
- .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"
--
1.7.7
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [RFC] block: convert block_stream command to QAPI
2011-11-01 18:08 [Qemu-devel] [RFC] block: convert block_stream command to QAPI Stefan Hajnoczi
@ 2011-11-03 15:51 ` Luiz Capitulino
2011-11-04 8:08 ` Stefan Hajnoczi
0 siblings, 1 reply; 3+ messages in thread
From: Luiz Capitulino @ 2011-11-03 15:51 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel
On Tue, 1 Nov 2011 18:08:16 +0000
Stefan Hajnoczi <stefanha@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"
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [RFC] block: convert block_stream command to QAPI
2011-11-03 15:51 ` Luiz Capitulino
@ 2011-11-04 8:08 ` Stefan Hajnoczi
0 siblings, 0 replies; 3+ messages in thread
From: Stefan Hajnoczi @ 2011-11-04 8:08 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: qemu-devel
On Thu, Nov 03, 2011 at 01:51:54PM -0200, Luiz Capitulino wrote:
> On Tue, 1 Nov 2011 18:08:16 +0000
> Stefan Hajnoczi <stefanha@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.
Thanks for the feedback. I will send out proper patches for all image
streaming commands soon. Just wanted to make sure I'm using doing the
QAPI conversion correctly before I do all of the commands.
Stefan
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-11-04 9:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-11-01 18:08 [Qemu-devel] [RFC] block: convert block_stream command to QAPI Stefan Hajnoczi
2011-11-03 15:51 ` Luiz Capitulino
2011-11-04 8:08 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).