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

["Denis V. Lunev" <den@openvz.org>]

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.