[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 08/15] qmp: add block_job_cancel command
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH v4 08/15] qmp: add block_job_cancel command |
Date: |
Fri, 20 Jan 2012 10:08:54 -0200 |
On Fri, 20 Jan 2012 09:30:45 +0100
Kevin Wolf <address@hidden> wrote:
> Am 20.01.2012 01:02, schrieb Eric Blake:
> > On 01/06/2012 07:01 AM, Stefan Hajnoczi wrote:
> >> Add block_job_cancel, which stops an active block streaming operation.
> >> When the operation has been cancelled the new BLOCK_JOB_CANCELLED event
> >> is emitted.
> >>
> >> Signed-off-by: Stefan Hajnoczi <address@hidden>
> >
> >> +++ b/hmp-commands.hx
> >> @@ -98,6 +98,20 @@ Set maximum speed for a background block operation.
> >> ETEXI
> >>
> >> {
> >> + .name = "block_job_cancel",
> >> + .args_type = "device:B",
> >> + .params = "device",
> >> + .help = "stop an active block streaming operation",
> >> + .mhandler.cmd = hmp_block_job_cancel,
> >> + },
> >> +
> >
> > Looking at this from libvirt's perspective, would it be possible to give
> > this a different name? Then libvirt would know that if
> > block_job_cancel_async exists, we have the official semantics; and if it
> > doesn't exist, then we can try block_job_cancel as a fallback to see if
> > we have the old blocking semantics.
> >
> > But by using the same name as the old unofficial blocking command, it's
> > difficult to tell if we should expect an event, or whether completion of
> > the command means completion of the cancel.
> >
> > On the other hand, I guess we can rely on completion of the command,
> > followed by reading block job status to see if the job is still in
> > flight, will tell us whether we need to worry about waiting for an event
> > - if the job is complete (whether or not this command was the blocking
> > variant), we are done; if the job is ongoing, we have the new semantics
> > and can expect an event; and that only leaves the race of calling the
> > command, then the job completes, then we query and see it done, then the
> > event comes, where we just have to be ready to ignore an unexpected event.
>
> You're quoting the HMP part, is that intentional? You shouldn't be using
> this at all.
>
> Anyway, are there even any qemu versions out there that implement an
> older interface?
>
> >> +##
> >> +# @block_job_cancel:
> >> +#
> >> +# Stop an active block streaming operation.
> >> +#
> >> +# This command returns immediately after marking the active block
> >> streaming
> >> +# operation for cancellation. It is an error to call this command if no
> >> +# operation is in progress.
> >> +#
> >> +# The operation will cancel as soon as possible and then emit the
> >> +# BLOCK_JOB_CANCELLED event. Before that happens the job is still
> >> visible when
> >> +# enumerated using query-block-jobs.
> >
> > Is there any policy on _ vs - in command names? It seems awkward to
> > have block_job_cancel but query-block-jobs.
>
> block_job_cancel is HMP, whereas query-block-jobs is a QMP command. QMP
> uses - consistently. Not sure if HMP is consistent, but it tends to use _.
This very series broke QMP's consistency because it was designed when we
were following HMP's inconsistencies...
- Re: [Qemu-devel] [PATCH v4 04/15] block: add image streaming block job, (continued)
[Qemu-devel] [PATCH v4 10/15] blockdev: make image streaming safe across hotplug, Stefan Hajnoczi, 2012/01/06
[Qemu-devel] [PATCH v4 05/15] block: rate-limit streaming operations, Stefan Hajnoczi, 2012/01/06
[Qemu-devel] [PATCH v4 09/15] qmp: add query-block-jobs, Stefan Hajnoczi, 2012/01/06
[Qemu-devel] [PATCH v4 08/15] qmp: add block_job_cancel command, Stefan Hajnoczi, 2012/01/06
[Qemu-devel] [PATCH v4 12/15] add QERR_BASE_ID_NOT_FOUND, Stefan Hajnoczi, 2012/01/06
[Qemu-devel] [PATCH v4 11/15] block: add bdrv_find_backing_image, Stefan Hajnoczi, 2012/01/06
[Qemu-devel] [PATCH v4 01/15] coroutine: add co_sleep_ns() coroutine sleep function, Stefan Hajnoczi, 2012/01/06