[libvirt] QMP: Supporting off tree APIs

On Wed, Jan 4, 2012 at 12:59 PM, Luiz Capitulino <lcapitulino(a)redhat.com&gt; wrote: > On Tue, 13 Dec 2011 13:52:27 +0000 > Stefan Hajnoczi <stefanha(a)linux.vnet.ibm.com&gt; 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?

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

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