Re: [libvirt] [PATCH] qemu: fix nested job with driver lock held

qemuMigrationUpdateJobStatus (called in a loop by migration and save tasks) uses qemuDomainObjEnterMonitorWithDriver; however, that function ended up starting a nested job without releasing the driver. Since no one else is making nested calls, we can inline the internal functions to properly track driver_locked.

* src/qemu/qemu_domain.h (qemuDomainObjBeginNestedJob) (qemuDomainObjBeginNestedJobWithDriver) (qemuDomainObjEndNestedJob): Drop unused prototypes. * src/qemu/qemu_domain.c (qemuDomainObjEnterMonitorInternal): Reflect driver lock to nested job. (qemuDomainObjBeginNestedJob) (qemuDomainObjBeginNestedJobWithDriver) (qemuDomainObjEndNestedJob): Drop unused functions. --- This does not solve the crash in 'virsh managedsave', but hopefully will make analysis easier in finding where we are freeing the monitor too early when probing to see if outgoing migration is completed. src/qemu/qemu_domain.c | 59 +++++++++++------------------------------------ src/qemu/qemu_domain.h | 9 ------- 2 files changed, 14 insertions(+), 54 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index fe88ce3..2eaaf3a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -832,31 +832,6 @@ int qemuDomainObjBeginAsyncJobWithDriver(struct qemud_driver *driver, } /* - * Use this to protect monitor sections within active async job. - * - * The caller must call qemuDomainObjBeginAsyncJob{,WithDriver} before it can - * use this method. Never use this method if you only own non-async job, use - * qemuDomainObjBeginJob{,WithDriver} instead. - */ -int -qemuDomainObjBeginNestedJob(struct qemud_driver *driver, - virDomainObjPtr obj) -{ - return qemuDomainObjBeginJobInternal(driver, false, obj, - QEMU_JOB_ASYNC_NESTED, - QEMU_ASYNC_JOB_NONE); -} - -int -qemuDomainObjBeginNestedJobWithDriver(struct qemud_driver *driver, - virDomainObjPtr obj) -{ - return qemuDomainObjBeginJobInternal(driver, true, obj, - QEMU_JOB_ASYNC_NESTED, - QEMU_ASYNC_JOB_NONE); -} - -/* * obj must be locked before calling, qemud_driver does not matter * * To be called after completing the work associated with the @@ -888,21 +863,6 @@ qemuDomainObjEndAsyncJob(struct qemud_driver *driver, virDomainObjPtr obj) return virDomainObjUnref(obj); } -void -qemuDomainObjEndNestedJob(struct qemud_driver *driver, virDomainObjPtr obj) -{ - qemuDomainObjPrivatePtr priv = obj->privateData; - - qemuDomainObjResetJob(priv); - qemuDomainObjSaveJob(driver, obj); - virCondSignal(&priv->job.cond); - - /* safe to ignore since the surrounding async job increased the reference - * counter as well */ - ignore_value(virDomainObjUnref(obj)); -} - - static int ATTRIBUTE_NONNULL(1) qemuDomainObjEnterMonitorInternal(struct qemud_driver *driver, bool driver_locked, @@ -911,7 +871,9 @@ qemuDomainObjEnterMonitorInternal(struct qemud_driver *driver, qemuDomainObjPrivatePtr priv = obj->privateData; if (priv->job.active == QEMU_JOB_NONE&& priv->job.asyncJob) { - if (qemuDomainObjBeginNestedJob(driver, obj)< 0) + if (qemuDomainObjBeginJobInternal(driver, driver_locked, obj, + QEMU_JOB_ASYNC_NESTED, + QEMU_ASYNC_JOB_NONE)< 0) return -1; if (!virDomainObjIsActive(obj)) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", @@ -952,8 +914,15 @@ qemuDomainObjExitMonitorInternal(struct qemud_driver *driver, priv->mon = NULL; } - if (priv->job.active == QEMU_JOB_ASYNC_NESTED) - qemuDomainObjEndNestedJob(driver, obj); + if (priv->job.active == QEMU_JOB_ASYNC_NESTED) { + qemuDomainObjResetJob(priv); + qemuDomainObjSaveJob(driver, obj); + virCondSignal(&priv->job.cond); + + /* safe to ignore since the surrounding async job increased + * the reference counter as well */ + ignore_value(virDomainObjUnref(obj)); + } } /* @@ -962,7 +931,7 @@ qemuDomainObjExitMonitorInternal(struct qemud_driver *driver, * To be called immediately before any QEMU monitor API call * Must have already either called qemuDomainObjBeginJob() and checked * that the VM is still active or called qemuDomainObjBeginAsyncJob, in which - * case this will call qemuDomainObjBeginNestedJob. + * case this will start a nested job. * * To be followed with qemuDomainObjExitMonitor() once complete */ @@ -988,7 +957,7 @@ void qemuDomainObjExitMonitor(struct qemud_driver *driver, * To be called immediately before any QEMU monitor API call * Must have already either called qemuDomainObjBeginJobWithDriver() and * checked that the VM is still active or called qemuDomainObjBeginAsyncJob, - * in which case this will call qemuDomainObjBeginNestedJobWithDriver. + * in which case this will start a nested job. * * To be followed with qemuDomainObjExitMonitorWithDriver() once complete */ diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 679259f..8bff8b0 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -143,9 +143,6 @@ int qemuDomainObjBeginAsyncJob(struct qemud_driver *driver, virDomainObjPtr obj, enum qemuDomainAsyncJob asyncJob) ATTRIBUTE_RETURN_CHECK; -int qemuDomainObjBeginNestedJob(struct qemud_driver *driver, - virDomainObjPtr obj) - ATTRIBUTE_RETURN_CHECK; int qemuDomainObjBeginJobWithDriver(struct qemud_driver *driver, virDomainObjPtr obj, enum qemuDomainJob job) @@ -154,9 +151,6 @@ int qemuDomainObjBeginAsyncJobWithDriver(struct qemud_driver *driver, virDomainObjPtr obj, enum qemuDomainAsyncJob asyncJob) ATTRIBUTE_RETURN_CHECK; -int qemuDomainObjBeginNestedJobWithDriver(struct qemud_driver *driver, - virDomainObjPtr obj) - ATTRIBUTE_RETURN_CHECK; int qemuDomainObjEndJob(struct qemud_driver *driver, virDomainObjPtr obj) @@ -164,9 +158,6 @@ int qemuDomainObjEndJob(struct qemud_driver *driver, int qemuDomainObjEndAsyncJob(struct qemud_driver *driver, virDomainObjPtr obj) ATTRIBUTE_RETURN_CHECK; -void qemuDomainObjEndNestedJob(struct qemud_driver *driver, - virDomainObjPtr obj); - void qemuDomainObjSetJobPhase(struct qemud_driver *driver, virDomainObjPtr obj, int phase);