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

[Stefan Hajnoczi]

On Thu, Dec 15, 2011 at 01:37:51PM +0100, Kevin Wolf wrote:
> Am 15.12.2011 13:00, schrieb Stefan Hajnoczi:
> > On Thu, Dec 15, 2011 at 11:30 AM, Luiz Capitulino
> > <address@hidden> wrote:
> >> On Thu, 15 Dec 2011 11:34:07 +0100
> >> Kevin Wolf <address@hidden> 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.
> 
> We probably can, but I would prefer fixing it in libvirt. Possibly the
> right fix there would be to remove it entirely from the HMP code - if I
> understand right, the HMP code is only meant to support older qemu
> versions anyway.
> 
> I also don't quite understand why there even is an option to disable QMP
> in libvirt, we have always told that HMP will become unstable.

Yeah, that's a good discussion to have.  We need to get everyone on the
same page.  I am starting a new thread where we can discuss this with
libvir-list and qemu-devel.

Stefan