Re: [Qemu-devel] [PATCH 08/11] migration: implement bdrv_all_find_vmstate_bs and bdrv_unlock helpers

[Denis V. Lunev]

On 11/06/2015 06:18 PM, Stefan Hajnoczi wrote:
> On Wed, Nov 04, 2015 at 08:19:39PM +0300, Denis V. Lunev wrote:
> > +BlockDriverState *bdrv_all_find_vmstate_bs(void)
> > +{
> > +    BlockDriverState *bs = NULL;
> > +
> > +    while ((bs = bdrv_next(bs))) {
> > +        AioContext *ctx = bdrv_get_aio_context(bs);
> > +
> > +        aio_context_acquire(ctx);
> > +        if (bdrv_can_snapshot(bs)) {
> > +            return bs;
> This leaves AioContext acquired.  If that is intentional then it must be
> documented because it looks like a bug.
>
> Normally functions that do this have an AioContext **aio_context agument
> so the caller does aio_context_release() later.  This way it's obvious
> that the caller needs to release.
>
> For example, see blockdev.c:find_block_job().
>
> > +        }
> > +        aio_context_release(ctx);
> > +    }
> > +    return NULL;
> > +}
> > +
> > +void bdrv_unlock(BlockDriverState *bs)
> > +{
> > +    aio_context_release(bdrv_get_aio_context(bs));
> > +}
> This API is weird.  There is no lock function.  Please do what I
> mentioned above.
>
> Another advantage of that approach is that we are 100% sure to release
> the same AioContext that was acquired (even if bdrv_set_aio_context()
> gets called halfway through).
no prob if Juan will accept that :) Ho does not want to care
and take the lock anywhere in his code. For me this is
pure matter of taste.