[libvirt] [PATCH] qemu: Fix all callers of qemuMonitorGetBlockJobInfo()

[Peter Krempa]

On Mon, Jan 04, 2016 at 16:10:56 +0100, Andrea Bolognani wrote:
> On Mon, 2016-01-04 at 15:46 +0100, Michal Privoznik wrote:
> > On 04.01.2016 15:20, Andrea Bolognani wrote:
> > > Commit 1b43885 modified one of the callers of this function to take
> > > into account the possible return value of 0 when the block job can't be
> > > found.
> > > 
> > > This commit finishes the job by updating the remaining caller.
> > > ---
> > >  src/qemu/qemu_driver.c | 13 ++++++-------
> > >  1 file changed, 6 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > > index 304165c..c4573d9 100644
> > > --- a/src/qemu/qemu_driver.c
> > > +++ b/src/qemu/qemu_driver.c
> > > @@ -16150,14 +16150,13 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
> > >          rc = qemuMonitorGetBlockJobInfo(priv->mon, disk->info.alias, &info);
> > >          if (qemuDomainObjExitMonitor(driver, vm) < 0)
> > >              goto cleanup;
> > > -        if (rc < 0)
> > > +        if (rc <= 0)
> > >              goto cleanup;
> > > -        if (rc == 1 &&
> > > -            (info.ready == 1 ||
> > > -             (info.ready == -1 &&
> > > -              info.end == info.cur &&
> > > -              (info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY ||
> > > -               info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT))))
> > > +        if (info.ready == 1 ||
> > > +            (info.ready == -1 &&
> > > +             info.end == info.cur &&
> > > +             (info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY ||
> > > +              info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT)))
> > >              disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
> > >      }
> > 
> > Interesting. So previously this code worked just right with no block
> > operation running on @disk. Now it will fail. I guess that's correct
> > approach since this function's job is to abort a block job.
> > 
> > ACK
> 
> I'm actually having second thoughts about this.
> 
> We only call qemuMonitorGetBlockJobInfo() if !disk->mirrorState.
> However, having it return 0 before would skip reading from info (my
> main concern, as it would not have been filled in) and cause the
> check for
> 
>   disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY
> 
> immediately afterwards to fail and an error to be raised before
> jumping to cleanup.
> 
> After my patch, the function will jump to cleanup earlier, still
> returning a negative error code, but not raising any error in the
> process.
> 
> I guess what I'm trying to say is that, unless you can convince
> otherwise, I'm going to have to SNACK this one :)

Wise choice, this would indeed break the error reporting in case when
the mirror dies for some reason. The timing for that to happen would
need to be really unfortunate though since otherwise the mirror job
would be removed by the block job event callback after receiving the
failure event.

Thanks for not breaking it.

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/20160104/f2801c42/attachment-0001.sig>