From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:40641) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rio7B-00089b-Tl for qemu-devel@nongnu.org; Thu, 05 Jan 2012 09:16:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Rio75-0002Ja-S1 for qemu-devel@nongnu.org; Thu, 05 Jan 2012 09:16:21 -0500 Received: from mx1.redhat.com ([209.132.183.28]:5005) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rio75-0002JV-LL for qemu-devel@nongnu.org; Thu, 05 Jan 2012 09:16:15 -0500 Date: Thu, 5 Jan 2012 12:16:03 -0200 From: Luiz Capitulino Message-ID: <20120105121603.309fe735@doriath> In-Reply-To: References: <1323784351-25531-1-git-send-email-stefanha@linux.vnet.ibm.com> <1323784351-25531-6-git-send-email-stefanha@linux.vnet.ibm.com> <20120104105911.7e8ca4e4@doriath> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: [Qemu-devel] QMP: Supporting off tree APIs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , aliguori@us.ibm.com, Stefan Hajnoczi , libvir-list@redhat.com, Marcelo Tosatti , qemu-devel@nongnu.org On Thu, 5 Jan 2012 13:48:43 +0000 Stefan Hajnoczi wrote: > On Wed, Jan 4, 2012 at 12:59 PM, Luiz Capitulino = wrote: > > On Tue, 13 Dec 2011 13:52:27 +0000 > > Stefan Hajnoczi wrote: > > > >> Add the block_stream command, which starts copy backing file contents > >> into the image file. =A0Later 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. >=20 > Ok. >=20 > > When we talked about this implementation (ie. having events, cancellati= on 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 b= lock > > 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 mig= ht > > be possible to add async support to the current server if waiting is not > > an option. >=20 > 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? >=20 > 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 @@ > >> =A0# Notes: Do not use this command. > >> =A0## > >> =A0{ 'command': 'cpu', 'data': {'index': 'int'} } > >> + > >> +## > >> +# @block_stream: > > > > Command names should be verbs and please use an hyphen. >=20 > These commands have been in the Image Streaming API spec from the beginni= ng: >=20 > http://wiki.qemu.org/Features/LiveBlockMigration/ImageStreamingAPI >=20 > 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[] =3D= { > >> =A0 =A0 =A0 =A0 =A0.desc =A0 =A0 =A0=3D "No '%(bus)' bus found for dev= ice '%(device)'", > >> =A0 =A0 =A0}, > >> =A0 =A0 =A0{ > >> + =A0 =A0 =A0 =A0.error_fmt =3D QERR_NOT_SUPPORTED, > >> + =A0 =A0 =A0 =A0.desc =A0 =A0 =A0=3D "Not supported", > > > > We have QERR_UNSUPPORTED already. >=20 > 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. >=20 > (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.