From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:48701)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <den@openvz.org>) id 1ZuisB-0003az-3x
	for qemu-devel@nongnu.org; Fri, 06 Nov 2015 10:24:16 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <den@openvz.org>) id 1Zuis7-0006gJ-5o
	for qemu-devel@nongnu.org; Fri, 06 Nov 2015 10:24:15 -0500
Received: from relay.parallels.com ([195.214.232.42]:53324)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <den@openvz.org>) id 1Zuis6-0006fm-U2
	for qemu-devel@nongnu.org; Fri, 06 Nov 2015 10:24:11 -0500
References: <1446657582-21619-1-git-send-email-den@openvz.org>
	<1446657582-21619-9-git-send-email-den@openvz.org>
	<20151106151845.GO12285@stefanha-x1.localdomain>
From: "Denis V. Lunev" <den@openvz.org>
Message-ID: <563CC60B.6090907@openvz.org>
Date: Fri, 6 Nov 2015 18:23:55 +0300
MIME-Version: 1.0
In-Reply-To: <20151106151845.GO12285@stefanha-x1.localdomain>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 08/11] migration: implement
 bdrv_all_find_vmstate_bs and bdrv_unlock helpers
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Stefan Hajnoczi <stefanha@gmail.com>
Cc: Kevin Wolf <kwolf@redhat.com>, qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>, Juan Quintela <quintela@redhat.com>

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.