qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v3 06/19] block: remove bdrv_is_allocated_above/


From: Kevin Wolf
Subject: Re: [Qemu-devel] [PATCH v3 06/19] block: remove bdrv_is_allocated_above/bdrv_co_is_allocated_above distinction
Date: Mon, 29 Jul 2013 16:15:47 +0200
User-agent: Mutt/1.5.21 (2010-09-15)

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 <address@hidden>
> >> Signed-off-by: Paolo Bonzini <address@hidden>
> >> ---
> >>  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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]