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

[Stefan Hajnoczi]

On Wed, Jan 4, 2012 at 12:59 PM, Luiz Capitulino <address@hidden> wrote:
> 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.

Ok.

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

I'm not sure what you'd like here, could you be more specific?  I have
introduced the concept of a block job - a long-running operation on
block devices - and the completion event when the job finishes.  I
guess you're thinking of a generic QMP job concept with a completion
event?  Or something else?

What I'd like to avoid at this stage is changing the QMP API as seen
by clients because we already have at least one client which relies on
this API.  I know that sucks and that this is my fault because I've
been dragging out this patch series for too long.  But in a situation
like this I think it's better to live with an API that doesn't meet
the current philosophy on nice APIs but works flawlessly with both new
and existing clients.  But it depends on the specifics, what changes
do you suggest?

>> +    /* 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.

Marcelo sent out patches for this features.  I think this series and
Marcelo's series can/will be merged one after another so that this
resolves itself without us needing to do anything.

http://www.mail-archive.com/address@hidden/msg91742.html

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

Ok.

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

These commands have been in the Image Streaming API spec from the beginning:

http://wiki.qemu.org/Features/LiveBlockMigration/ImageStreamingAPI

We cannot prettify the API at this stage.  You, Anthony, and I
discussed QAPI command naming on IRC maybe two months ago and this
seemed to be the way to do it.  These commands fit the existing
block_* commands and are already in use by libvirt - if we change
names now we break libvirt.

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

Ok.

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

Ok.

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

I know.  We're stuck in a hard place here again because NotSupported
has been in the Image Streaming API spec and hence implemented in
libvirt for a while now.  If we change this then an old client which
only understands NotSupported will not know what to do with the
Unsupported error.

(Unsupported was not in QEMU when the Image Streaming API was defined.)

>> 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).

Ok.