From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57078) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V3oEy-0005r8-9J for qemu-devel@nongnu.org; Mon, 29 Jul 2013 10:16:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V3oEs-0008Kv-9V for qemu-devel@nongnu.org; Mon, 29 Jul 2013 10:16:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:4663) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V3oEs-0008Kl-2C for qemu-devel@nongnu.org; Mon, 29 Jul 2013 10:15:54 -0400 Date: Mon, 29 Jul 2013 16:15:47 +0200 From: Kevin Wolf Message-ID: <20130729141547.GK10467@dhcp-200-207.str.redhat.com> References: <1374762197-7261-1-git-send-email-pbonzini@redhat.com> <1374762197-7261-7-git-send-email-pbonzini@redhat.com> <20130729133407.GH10467@dhcp-200-207.str.redhat.com> <51F67535.8030404@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <51F67535.8030404@redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 06/19] block: remove bdrv_is_allocated_above/bdrv_co_is_allocated_above distinction List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: pl@kamp.de, qemu-devel@nongnu.org, stefanha@redhat.com Am 29.07.2013 um 15:59 hat Paolo Bonzini geschrieben: > Il 29/07/2013 15:34, Kevin Wolf ha scritto: > > Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben: > >> Now that bdrv_is_allocated detects coroutine context, the two can > >> use the same code. > >> > >> Reviewed-by: Eric Blake > >> Signed-off-by: Paolo Bonzini > >> --- > >> block.c | 46 ++++------------------------------------------ > >> block/commit.c | 6 +++--- > >> block/mirror.c | 4 ++-- > >> block/stream.c | 4 ++-- > >> include/block/block.h | 4 ---- > >> 5 files changed, 11 insertions(+), 53 deletions(-) > >> > >> diff --git a/block.c b/block.c > >> index dd4c570..1ee1d93 100644 > >> --- a/block.c > >> +++ b/block.c > >> @@ -3058,10 +3058,10 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors, > >> * allocated/unallocated state. > >> * > >> */ > >> -int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top, > >> - BlockDriverState *base, > >> - int64_t sector_num, > >> - int nb_sectors, int *pnum) > >> +int coroutine_fn bdrv_is_allocated_above(BlockDriverState *top, > >> + BlockDriverState *base, > >> + int64_t sector_num, > >> + int nb_sectors, int *pnum) > > > > This is no longer a coroutine_fn. > > I'm always confused about must-yield vs. may-yield. :( A coroutine_fn may yield without checking whether it runs in a coroutine. Or phrased differently, coroutine_fns may only be called from coroutine context. > > However, if this was the only reason for making bdrv_is_allocated() a > > hybrid function, it may be easier to simply let the only caller > > (img_compare in qemu-img) run in a coroutine and drop the synchronous > > version entirely, keeping only a coroutine_fn. > > The reason is to avoid having the same (IMHO pretty gratuitous) > distinction in get_block_status. "qemu-img map" will also add another > synchronous user of get_block_status. > > If we want to keep only the coroutine_fn, we should make all of qemu-img > run in a coroutine. I think in this case that's really a good option, because qemu-img is the only caller for the synchronous version (if I didn't miss any other). It may turn out useful for other functions as well. And letting qemu-img's main() call into a coroutine doesn't really seem too bad compared to doing the optional coroutine dance for each block.c function. And it finally gets us a main loop in qemu-img, too. Kevin