[libvirt] [PATCHv3 7/7] qemu: Refactor qemuDomainBlockJobAbort()

[Peter Krempa]

On Mon, Apr 13, 2015 at 21:08:27 -0400, John Ferlan wrote:
> 
> 
> On 04/09/2015 12:45 PM, Peter Krempa wrote:
> > Change few variable names and refactor the code flow. As an additional
> > bonus the function now fails if the event state is not as expected.
> > ---
> > 
> > Notes:
> >     Version 3:
> >     - fixed error reporting code and success code propagation from pivot
> > 
> >  src/qemu/qemu_driver.c | 107 +++++++++++++++++++++++--------------------------
> >  1 file changed, 51 insertions(+), 56 deletions(-)
> > 
> 
> ACK 1-6...
> 
> Just one question below...
> 
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index eebed55..8d4aa97 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -16273,13 +16273,13 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
> >  {
> >      virQEMUDriverPtr driver = dom->conn->privateData;
> >      char *device = NULL;
> > -    virObjectEventPtr event = NULL;
> > -    virObjectEventPtr event2 = NULL;
> >      virDomainDiskDefPtr disk;
> >      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> >      bool save = false;
> >      int idx;
> > -    bool async;
> > +    bool modern;
> > +    bool pivot = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT);
> > +    bool async = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC);
> >      virDomainObjPtr vm;
> >      int ret = -1;
> > 
> > @@ -16292,7 +16292,7 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
> >      if (virDomainBlockJobAbortEnsureACL(dom->conn, vm->def) < 0)
> >          goto cleanup;
> > 
> > -    if (qemuDomainSupportsBlockJobs(vm, &async) < 0)
> > +    if (qemuDomainSupportsBlockJobs(vm, &modern) < 0)
> >          goto cleanup;
> > 
> >      if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> > @@ -16308,19 +16308,14 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
> >          goto endjob;
> >      disk = vm->def->disks[idx];
> > 
> > -    if (async && !(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC)) {
> > -        /* prepare state for event delivery */
> > +    if (modern && !async) {
> > +        /* prepare state for event delivery. Since qemuDomainBlockPivot is
> > +         * synchronous, but the event is delivered asynchronously we need to
> > +         * wait too */
> >          disk->blockJobStatus = -1;
> >          disk->blockJobSync = true;
> >      }
> > 
> > -    if ((flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT) &&
> > -        !(async && disk->mirror)) {
> > -        virReportError(VIR_ERR_OPERATION_INVALID,
> > -                       _("pivot of disk '%s' requires an active copy job"),
> > -                       disk->dst);
> > -        goto endjob;
> > -    }
> >      if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_NONE &&
> >          disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY) {
> >          virReportError(VIR_ERR_OPERATION_INVALID,
> > @@ -16329,31 +16324,31 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
> >          goto endjob;
> >      }
> > 
> > -    if (disk->mirror && (flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT)) {
> > -        ret = qemuDomainBlockPivot(driver, vm, device, disk);
> > -        if (ret < 0 && async) {
> > +    if (pivot) {
> > +        if ((ret = qemuDomainBlockPivot(driver, vm, device, disk)) < 0) {
> >              disk->blockJobSync = false;
> >              goto endjob;
> >          }
> > -        goto waitjob;
> > -    }
> > -    if (disk->mirror) {
> > -        disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_ABORT;
> > -        save = true;
> > -    }
> > +    } else {
> > +        if (disk->mirror) {
> > +            disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_ABORT;
> > +            save = true;
> > +        }
> > 
> > -    qemuDomainObjEnterMonitor(driver, vm);
> > -    ret = qemuMonitorBlockJobCancel(qemuDomainGetMonitor(vm), device, async);
> > -    if (qemuDomainObjExitMonitor(driver, vm) < 0)
> > -        ret = -1;
> > +        qemuDomainObjEnterMonitor(driver, vm);
> > +        ret = qemuMonitorBlockJobCancel(qemuDomainGetMonitor(vm), device, modern);
> > +        if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> > +            ret = -1;
> > +            goto endjob;
> > +        }
> 
> should this just fall through now?  Asked differently - should
> disk->mirrorState change before goto endjob if ExitMonitor fails?
> 
> Would "if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0)" do the
> trick?

The mirror state was changed to VIR_DOMAIN_DISK_MIRROR_STATE_ABORT prior
to the monitor call. If the call fails, we did not issue the cancel and
thus need to revert to a safe state for a potential new operation.

> 
> ACK - in general - just making sure something wasn't missed.

I've pushed the series, thanks.

> 
> John
> 

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150414/66347585/attachment-0001.sig>