On Thu, 5 Jan 2012 13:48:43 +0000
Stefan Hajnoczi <stefanha@xxxxxxxxx> wrote:
> On Wed, Jan 4, 2012 at 12:59 PM, Luiz Capitulino <lcapitulino@xxxxxxxxxx> wrote:
> > On Tue, 13 Dec 2011 13:52:27 +0000
> > Stefan Hajnoczi <stefanha@xxxxxxxxxxxxxxxxxx> 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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list