On Thu, 15 Dec 2011 12:00:16 +0000
Stefan Hajnoczi <stefa...@gmail.com> wrote:
> On Thu, Dec 15, 2011 at 11:30 AM, Luiz Capitulino
> <lcapitul...@redhat.com> wrote:
> > On Thu, 15 Dec 2011 11:34:07 +0100
> > Kevin Wolf <kw...@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 need to fully review this series to ack or nack this, but unfortunately I've
had a busy week and will be on vacations for two weeks starting in a few hours.
I think it would be needed to have at least Kevin and Anthony or me acking this.
