Patchwork [v3,8/9] qmp: add query-block-jobs

login
register
mail settings
Submitter Stefan Hajnoczi
Date Dec. 13, 2011, 1:52 p.m.
Message ID <1323784351-25531-9-git-send-email-stefanha@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/131095/
State New
Headers show

Comments

Stefan Hajnoczi - Dec. 13, 2011, 1:52 p.m.
Add query-block-jobs, which shows the progress of ongoing block device
operations.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 blockdev.c       |   33 +++++++++++++++++++++++++++++++++
 hmp.c            |   40 ++++++++++++++++++++++++++++++++++++++++
 hmp.h            |    1 +
 monitor.c        |    7 +++++++
 qapi-schema.json |   32 ++++++++++++++++++++++++++++++++
 qmp-commands.hx  |   39 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 152 insertions(+), 0 deletions(-)
Kevin Wolf - Dec. 14, 2011, 2:54 p.m.
Am 13.12.2011 14:52, schrieb Stefan Hajnoczi:
> Add query-block-jobs, which shows the progress of ongoing block device
> operations.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  blockdev.c       |   33 +++++++++++++++++++++++++++++++++
>  hmp.c            |   40 ++++++++++++++++++++++++++++++++++++++++
>  hmp.h            |    1 +
>  monitor.c        |    7 +++++++
>  qapi-schema.json |   32 ++++++++++++++++++++++++++++++++
>  qmp-commands.hx  |   39 +++++++++++++++++++++++++++++++++++++++
>  6 files changed, 152 insertions(+), 0 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index b276b2f..5b2b128 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -997,3 +997,36 @@ void qmp_block_job_cancel(const char *device, Error **errp)
>      trace_qmp_block_job_cancel(job);
>      block_job_cancel(job);
>  }
> +
> +static void do_qmp_query_block_jobs_one(void *opaque, BlockDriverState *bs)
> +{
> +    BlockJobInfoList **prev = opaque;
> +    BlockJob *job = bs->job;
> +
> +    if (job) {
> +        BlockJobInfoList *elem;
> +        BlockJobInfo *info = g_new(BlockJobInfo, 1);
> +        *info = (BlockJobInfo){
> +            .type = g_strdup(job->job_type->job_type),
> +            .device = g_strdup(bdrv_get_device_name(bs)),
> +            .len = job->len,
> +            .offset = job->offset,
> +            .speed = job->speed,
> +        };

Some spaces to align it nicely?

> +
> +        elem = g_new0(BlockJobInfoList, 1);
> +        elem->value = info;
> +
> +        (*prev)->next = elem;
> +        *prev = elem;

I hate these open-coded lists. But not your fault...

> +    }
> +}
> +
> +BlockJobInfoList *qmp_query_block_jobs(Error **errp)
> +{
> +    /* Dummy is a fake list element for holding the head pointer */
> +    BlockJobInfoList dummy = {};
> +    BlockJobInfoList *prev = &dummy;
> +    bdrv_iterate(do_qmp_query_block_jobs_one, &prev);
> +    return dummy.next;
> +}
> 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?

Kevin
Stefan Hajnoczi - Dec. 15, 2011, 8:27 a.m.
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?

Stefan
Kevin Wolf - Dec. 15, 2011, 10:34 a.m.
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.

And I was pretty sure that we all agreed that HMP should be considered
unstable after a transition period. For new commands there's certainly
no reason to have a transition period, so I would consider them unstable
from the very beginning. After all, there are no qemu versions that
support the feature in question, but don't support QMP.

Kevin
Luiz Capitulino - Dec. 15, 2011, 11:30 a.m.
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.

> 
> And I was pretty sure that we all agreed that HMP should be considered
> unstable after a transition period. For new commands there's certainly
> no reason to have a transition period, so I would consider them unstable
> from the very beginning. After all, there are no qemu versions that
> support the feature in question, but don't support QMP.
> 
> Kevin
>
Stefan Hajnoczi - Dec. 15, 2011, noon
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.

Stefan
Luiz Capitulino - Dec. 15, 2011, 12:09 p.m.
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 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.
Kevin Wolf - Dec. 15, 2011, 12:37 p.m.
Am 15.12.2011 13:00, schrieb Stefan Hajnoczi:
> 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.

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.

Kevin
Stefan Hajnoczi - Dec. 15, 2011, 12:51 p.m.
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
> > <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.
> 
> 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
Luiz Capitulino - Jan. 4, 2012, 1:17 p.m.
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.

Patch

diff --git a/blockdev.c b/blockdev.c
index b276b2f..5b2b128 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -997,3 +997,36 @@  void qmp_block_job_cancel(const char *device, Error **errp)
     trace_qmp_block_job_cancel(job);
     block_job_cancel(job);
 }
+
+static void do_qmp_query_block_jobs_one(void *opaque, BlockDriverState *bs)
+{
+    BlockJobInfoList **prev = opaque;
+    BlockJob *job = bs->job;
+
+    if (job) {
+        BlockJobInfoList *elem;
+        BlockJobInfo *info = g_new(BlockJobInfo, 1);
+        *info = (BlockJobInfo){
+            .type = g_strdup(job->job_type->job_type),
+            .device = g_strdup(bdrv_get_device_name(bs)),
+            .len = job->len,
+            .offset = job->offset,
+            .speed = job->speed,
+        };
+
+        elem = g_new0(BlockJobInfoList, 1);
+        elem->value = info;
+
+        (*prev)->next = elem;
+        *prev = elem;
+    }
+}
+
+BlockJobInfoList *qmp_query_block_jobs(Error **errp)
+{
+    /* Dummy is a fake list element for holding the head pointer */
+    BlockJobInfoList dummy = {};
+    BlockJobInfoList *prev = &dummy;
+    bdrv_iterate(do_qmp_query_block_jobs_one, &prev);
+    return dummy.next;
+}
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.
+         */
+        if (strcmp(list->value->type, "stream") == 0) {
+            monitor_printf(mon, "Streaming device %s: Completed %" PRId64
+                           " of %" PRId64 " bytes, speed limit %" PRId64
+                           " bytes/s\n",
+                           list->value->device,
+                           list->value->offset,
+                           list->value->len,
+                           list->value->speed);
+        } else {
+            monitor_printf(mon, "Type %s, device %s: Completed %" PRId64
+                           " of %" PRId64 " bytes, speed limit %" PRId64
+                           " bytes/s\n",
+                           list->value->type,
+                           list->value->device,
+                           list->value->offset,
+                           list->value->len,
+                           list->value->speed);
+        }
+        list = list->next;
+    }
+}
+
 void hmp_quit(Monitor *mon, const QDict *qdict)
 {
     monitor_suspend(mon);
diff --git a/hmp.h b/hmp.h
index 30d9051..9178b09 100644
--- a/hmp.h
+++ b/hmp.h
@@ -32,6 +32,7 @@  void hmp_info_vnc(Monitor *mon);
 void hmp_info_spice(Monitor *mon);
 void hmp_info_balloon(Monitor *mon);
 void hmp_info_pci(Monitor *mon);
+void hmp_info_block_jobs(Monitor *mon);
 void hmp_quit(Monitor *mon, const QDict *qdict);
 void hmp_stop(Monitor *mon, const QDict *qdict);
 void hmp_system_reset(Monitor *mon, const QDict *qdict);
diff --git a/monitor.c b/monitor.c
index 5bf29f2..ee22e58 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2641,6 +2641,13 @@  static mon_cmd_t info_cmds[] = {
         .mhandler.info = hmp_info_blockstats,
     },
     {
+        .name       = "block-jobs",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show progress of ongoing block device operations",
+        .mhandler.info = hmp_info_block_jobs,
+    },
+    {
         .name       = "registers",
         .args_type  = "",
         .params     = "",
diff --git a/qapi-schema.json b/qapi-schema.json
index 270d05e..f1d56c0 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -845,6 +845,38 @@ 
 { 'command': 'query-pci', 'returns': ['PciInfo'] }
 
 ##
+# @BlockJobInfo:
+#
+# Information about a long-running block device operation.
+#
+# @type: the job type ('stream' for image streaming)
+#
+# @device: the block device name
+#
+# @len: the maximum progress value
+#
+# @offset: the current progress value
+#
+# @speed: the rate limit, bytes per second
+#
+# Since: 1.1
+##
+{ 'type': 'BlockJobInfo',
+  'data': {'type': 'str', 'device': 'str', 'len': 'int',
+           'offset': 'int', 'speed': 'int'} }
+
+##
+# @query-block-jobs:
+#
+# Return information about long-running block device operations.
+#
+# Returns: a list of @BlockJobInfo for each active block job
+#
+# Since: 1.1
+##
+{ 'command': 'query-block-jobs', 'returns': ['BlockJobInfo'] }
+
+##
 # @quit:
 #
 # This command will cause the QEMU process to exit gracefully.  While every
diff --git a/qmp-commands.hx b/qmp-commands.hx
index a57d079..7fd968d 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2106,3 +2106,42 @@  EQMP
         .args_type  = "",
         .mhandler.cmd_new = qmp_marshal_input_query_balloon,
     },
+
+SQMP
+
+query-block-jobs
+----------------
+
+Show progress of ongoing block device operations.
+
+Return a json-array of all block device operations.  If no operation is active
+then return an empty array.  Each operation is a json-object with the following
+data:
+
+- type:     job type ("stream" for image streaming, json-string)
+- device:   device name (json-string)
+- len:      maximum progress value (json-int)
+- offset:   current progress value (json-int)
+- speed:    rate limit, bytes per second (json-int)
+
+Progress can be observed as offset increases and it reaches len when the
+operation completes.  Offset and len have undefined units but can be used to
+calculate a percentage indicating the progress that has been made.
+
+Example:
+
+-> { "execute": "query-block-jobs" }
+<- { "return":[
+      { "type": "stream", "device": "virtio0",
+        "len": 10737418240, "offset": 709632,
+        "speed": 0 }
+   ]
+ }
+
+EQMP
+
+    {
+        .name       = "query-block-jobs",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_query_block_jobs,
+    },