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

[Stefan Hajnoczi]

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).

signature.asc
Description: PGP signature