[libvirt] QMP: Supporting off tree APIs

[Luiz Capitulino]

On Thu, 5 Jan 2012 13:48:43 +0000
Stefan Hajnoczi <stefanha at gmail.com> wrote:

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

Exactly. We should have QMP_JOB_COMPLETED instead of BLOCK_JOB_COMPLETED,
for example.

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

Let me try to understand it: is libvirt relying on an off tree API and
we are now required to have stable guarantees to off tree APIs?

I can't even express how insane this looks to me, at least not without
being too harsh.