Re: [Qemu-devel] [PATCH v3 8/9] qmp: add query-block-jobs

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 15 Dec 2011 12:00:16 +0000
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Thu, Dec 15, 2011 at 11:30 AM, Luiz Capitulino
> <lcapitulino@redhat.com> wrote:
> > On Thu, 15 Dec 2011 11:34:07 +0100
> > Kevin Wolf <kwolf@redhat.com> wrote:
> >
> >> Am 15.12.2011 09:27, schrieb Stefan Hajnoczi:
> >> > On Wed, Dec 14, 2011 at 03:54:52PM +0100, Kevin Wolf wrote:
> >> >> Am 13.12.2011 14:52, schrieb Stefan Hajnoczi:
> >> >>> diff --git a/hmp.c b/hmp.c
> >> >>> index 66d9d0f..c16d6a1 100644
> >> >>> --- a/hmp.c
> >> >>> +++ b/hmp.c
> >> >>> @@ -499,6 +499,46 @@ void hmp_info_pci(Monitor *mon)
> >> >>>      qapi_free_PciInfoList(info);
> >> >>>  }
> >> >>>
> >> >>> +void hmp_info_block_jobs(Monitor *mon)
> >> >>> +{
> >> >>> +    BlockJobInfoList *list;
> >> >>> +    Error *err = NULL;
> >> >>> +
> >> >>> +    list = qmp_query_block_jobs(&err);
> >> >>> +    assert(!err);
> >> >>> +
> >> >>> +    if (!list) {
> >> >>> +        monitor_printf(mon, "No active jobs\n");
> >> >>> +        return;
> >> >>> +    }
> >> >>> +
> >> >>> +    while (list) {
> >> >>> +        /* The HMP output for streaming jobs is special because historically it
> >> >>> +         * was different from other job types so applications may depend on the
> >> >>> +         * exact string.
> >> >>> +         */
> >> >>
> >> >> Er, what? This is new code. What HMP clients use this string? I know
> >> >> that libvirt already got support for this before we implemented it, but
> >> >> shouldn't that be QMP only?
> >> >
> >> > Libvirt HMP uses this particular string, which turned out to be
> >> > sub-optimal once I realized we might support other types of block jobs
> >> > in the future.
> >> >
> >> > You can still build libvirt HMP-only by disabling the yajl library
> >> > dependency.  The approach I've taken is to make the interfaces available
> >> > over both HMP and QMP (and so has the libvirt-side code).
> >> >
> >> > In any case, we have defined both HMP and QMP.  Libvirt implements both
> >> > and I don't think there's a reason to provide only QMP.
> >> >
> >> > Luiz: For future features, are we supposed to provide only QMP
> >> > interfaces, not HMP?
> >>
> >> Of course, qemu should provide them as HMP command. But libvirt
> >> shouldn't use HMP commands. HMP is intended for human users, not as an
> >> API for management.
> >
> > That's correct.
> >
> > What defines if you're going to have a HMP version of a command is if
> > you expect it to be used by humans and if you do all its output and
> > arguments should be user friendly. You should never expect nor assume
> > it's going to be used by a management interface.
> 
> Okay, thanks Kevin and Luiz for explaining.  In this case I know it is
> used by a management interface because libvirt has code to use it.
> 
> I was my mistake to include the HMP side as part of the "API" but it's
> here now and I think we can live with this.

I'll have to nack this. Having stable guarantees with something that's
not in the tree is just impossible, not even mentioning it's HMP.

Having said that, the HMP string can be whatever you want. I will just ask
you to drop the comment and won't refuse patches that change/improve it.