Re: [Qemu-devel] [PATCH 2/3] block: Add QMP support for streaming to an intermediate layer

[Eric Blake]

On 02/20/2015 06:53 AM, Alberto Garcia wrote:
> This adds the 'top' parameter to the 'block-stream' QMP command and
> checks that its value is valid before passing it to stream_start().
> 
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>  blockdev.c                | 19 +++++++++++++++----
>  hmp.c                     |  2 +-
>  include/qapi/qmp/qerror.h |  3 +++
>  qapi/block-core.json      | 18 ++++++++++++++----
>  qmp-commands.hx           |  2 +-
>  5 files changed, 34 insertions(+), 10 deletions(-)

> @@ -2123,12 +2125,21 @@ void qmp_block_stream(const char *device,
>      aio_context = bdrv_get_aio_context(bs);
>      aio_context_acquire(aio_context);
>  
> -    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
> +    if (has_top) {
> +        top_bs = bdrv_find_backing_image(bs, top);
> +        if (top_bs == NULL) {
> +            error_set(errp, QERR_TOP_NOT_FOUND, top);
> +            goto out;
> +        }

If I understand correctly, bdrv_find_backing_image has problems for
backing nodes that don't have a file name.  Given our shift towards node
names, I think we really want to target node names rather than file
names when specifying which node we will use as the top bound receiving
the stream operations.


> +++ b/include/qapi/qmp/qerror.h
> @@ -127,6 +127,9 @@ void qerror_report_err(Error *err);
>  #define QERR_SET_PASSWD_FAILED \
>      ERROR_CLASS_GENERIC_ERROR, "Could not set password"
>  
> +#define QERR_TOP_NOT_FOUND \
> +    ERROR_CLASS_GENERIC_ERROR, "Top '%s' not found"
> +

Please don't.  Just use error_setg() at the right place with the direct
message (existing QERR_ macros are a legacy holdover, and we shouldn't
be creating more of them).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature