Re: [libvirt] [PATCH 3/3] qemu: blockjob: Synchronously update backing chain in XML on ABORT/PIVOT

On 03/30/2015 03:26 AM, Peter Krempa wrote: > When the synchronous pivot option is selected, libvirt would not update > the backing chain until the job was exitted. Some applications then s/exitted/exited/ > received invalid data as their job serialized first. > > This patch removes polling to wait for the ABORT/PIVOT job completion > and replaces it with a condition. If a synchronous operation is > requested the update of the XML is executed in the job of the caller of > the synchronous request. Otherwise the monitor event callback uses a > separate worker to update the backing chain with a new job. > > This is a regression since 1a92c719101e5bfa6fe2b78006ad04c7f075ea28 unreleased, and therefore worth fixing during freeze. > > When the ABORT job is finished synchronously you get the following call > stack: > #0 qemuBlockJobEventProcess > #1 qemuDomainBlockJobImpl > #2 qemuDomainBlockJobAbort > #3 virDomainBlockJobAbort > > While previously or while using the _ASYNC flag you'd get: > #0 qemuBlockJobEventProcess > #1 processBlockJobEvent > #2 qemuProcessEventHandler > #3 virThreadPoolWorker > --- > src/conf/domain_conf.c | 16 +++++++++++++++- > src/conf/domain_conf.h | 6 ++++++ > src/qemu/qemu_driver.c | 45 +++++++++++++++++++-------------------------- > src/qemu/qemu_process.c | 38 +++++++++++++++++++++++++------------- > 4 files changed, 65 insertions(+), 40 deletions(-) > > @@ -16389,37 +16395,24 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, > status); > event2 = virDomainEventBlockJob2NewFromObj(vm, disk->dst, type, > status); > - } else if (!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC)) { > + } else if (disk->blockJobSync) { > /* XXX If the event reports failure, we should reflect > * that back into the return status of this API call. */ Isn't this comment stale now? That is, since you are waiting for the event to free the condition, and we are listening for both success and failure events, we can now tell if the event reports failure.

Otherwise, looks correct to me. I hate that it is so close to the release deadline, but I hate regressions more (which is why we have freezes). I'd feel better if you got a second review, if you can wrangle one up in time, but here's my: ACK.