On 04/06/2012 02:29 PM, Eric Blake wrote:
> From: Adam Litke <agl@xxxxxxxxxx>
>
> Without the VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC flag, libvirt will internally poll
> using qemu's "query-block-jobs" API and will not return until the operation has
> been completed. API users are advised that this operation is unbounded and
> further interaction with the domain during this period may block. Future
> patches may refactor things to allow other queries in parallel with this
> polling. Unfortunately, there's no good way to tell if qemu will emit the
> new event, so this implementation always polls to deal with older qemu.
>
> Signed-off-by: Adam Litke <agl@xxxxxxxxxx>
> Cc: Stefan Hajnoczi <stefanha@xxxxxxxxx>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
> src/qemu/qemu_driver.c | 55 +++++++++++++++++++++++++++++++++++++++++------
> 1 files changed, 48 insertions(+), 7 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d9e35be..5f0eb05 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -11599,7 +11599,7 @@ cleanup:
> static int
> qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base,
> unsigned long bandwidth, virDomainBlockJobInfoPtr info,
> - int mode)
> + int mode, unsigned int flags)
> {
> struct qemud_driver *driver = dom->conn->privateData;
> virDomainObjPtr vm = NULL;
> @@ -11641,6 +11641,45 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base,
> ret = qemuMonitorBlockJob(priv->mon, device, base, bandwidth, info, mode);
> qemuDomainObjExitMonitorWithDriver(driver, vm);
Do I understand correctly that qemu's abort was always asynchronous, and
prior to this, an abort call to libvirt would erroneously return
immediately? (I'm still trying to understand the code...)
>
> + /* Qemu provides asynchronous block job cancellation, but without
> + * the VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC flag libvirt guarantees a
> + * synchronous operation. Provide this behavior by waiting here,
> + * so we don't get confused by newly scheduled block jobs.
> + */
> + if (ret == 0 && mode == BLOCK_JOB_ABORT &&
> + !(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC)) {
> + ret = 1;
Why is ret set to 1? It will be assigned the return value of
qemuMonitorBlockJob before it's ever used for anything, so this seems
unnecessary.
> + while (1) {
> + /* Poll every 50ms */
> + struct timespec ts = { .tv_sec = 0,
> + .tv_nsec = 50 * 1000 * 1000ull };
This timespec could just as well be static const, couldn't it? No big
deal one way or the other, though.
> + virDomainBlockJobInfo dummy;
> +
> + qemuDomainObjEnterMonitorWithDriver(driver, vm);
> + ret = qemuMonitorBlockJob(priv->mon, device, NULL, 0, &dummy,
> + BLOCK_JOB_INFO);
> + qemuDomainObjExitMonitorWithDriver(driver, vm);
> +
> + if (ret <= 0)
> + break;
> +
> + virDomainObjUnlock(vm);
> + qemuDriverUnlock(driver);
> +
> + nanosleep(&ts, NULL);
> +
> + qemuDriverLock(driver);
> + virDomainObjLock(vm);
> +
> + if (!virDomainObjIsActive(vm)) {
> + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("domain is not running"));
> + ret = -1;
> + break;
> + }
> + }
> + }
> +
> endjob:
> if (qemuDomainObjEndJob(driver, vm) == 0) {
> vm = NULL;
> @@ -11658,8 +11697,9 @@ cleanup:
> static int
> qemuDomainBlockJobAbort(virDomainPtr dom, const char *path, unsigned int flags)
> {
> - virCheckFlags(0, -1);
> - return qemuDomainBlockJobImpl(dom, path, NULL, 0, NULL, BLOCK_JOB_ABORT);
> + virCheckFlags(VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC, -1);
> + return qemuDomainBlockJobImpl(dom, path, NULL, 0, NULL, BLOCK_JOB_ABORT,
> + flags);
> }
>
> static int
> @@ -11667,7 +11707,8 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path,
> virDomainBlockJobInfoPtr info, unsigned int flags)
> {
> virCheckFlags(0, -1);
> - return qemuDomainBlockJobImpl(dom, path, NULL, 0, info, BLOCK_JOB_INFO);
> + return qemuDomainBlockJobImpl(dom, path, NULL, 0, info, BLOCK_JOB_INFO,
> + flags);
> }
>
> static int
> @@ -11676,7 +11717,7 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, const char *path,
> {
> virCheckFlags(0, -1);
> return qemuDomainBlockJobImpl(dom, path, NULL, bandwidth, NULL,
> - BLOCK_JOB_SPEED);
> + BLOCK_JOB_SPEED, flags);
> }
>
> static int
> @@ -11687,10 +11728,10 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base,
>
> virCheckFlags(0, -1);
> ret = qemuDomainBlockJobImpl(dom, path, base, bandwidth, NULL,
> - BLOCK_JOB_PULL);
> + BLOCK_JOB_PULL, flags);
> if (ret == 0 && bandwidth != 0)
> ret = qemuDomainBlockJobImpl(dom, path, NULL, bandwidth, NULL,
> - BLOCK_JOB_SPEED);
> + BLOCK_JOB_SPEED, flags);
> return ret;
> }
>
ACK once the items above are addressed (either by explaining, changing
code, or telling me why I'm wrong)
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list