| Message ID | 1323784351-25531-9-git-send-email-stefanha@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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 >
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
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.
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
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
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.
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, + },
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(-)