Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations

[Kevin Wolf]

Am 04.11.2022 um 09:44 hat Paolo Bonzini geschrieben:
> On 11/4/22 08:35, Emanuele Giuseppe Esposito wrote:
> > But isn't it a bug also not to mark a function _only_ called by
> > coroutine_fn? My point is that if this function is an implementation of
> > a BlockDriver callback marked as coroutine_fn (like in patch 6 with
> > vmdk), then it would make sense.
> 
> If a function implements a coroutine_fn callback but does not suspend, then
> it makes sense to mark it coroutine_fn.
> 
> In general it's not a bug.  In most cases it would only be a coincidence
> that the function is called from a coroutine_fn.  For example consider
> bdrv_round_to_clusters().  Marking it coroutine_fn signals that it may
> suspend now (it doesn't) or in the future.  However it's only doing some
> math based on the result of bdrv_get_info(), so it is extremely unlikely
> that this will happen.
> 
> In this case... oh wait.  block_copy_is_cluster_allocated is calling
> bdrv_is_allocated, and block_copy_reset_unallocated calls
> block_copy_is_cluster_allocated.  bdrv_is_allocated is a mixed
> coroutine/non-coroutine function, and in this case it is useful to document
> that bdrv_is_allocated will suspend.  The patch is correct, only the commit
> message is wrong.

Ah, right, now I remember that this is what I had discussed with
Emanuele. I knew that there was a reason for it...

What we probably should really do is a bdrv_co_is_allocated() that
doesn't go through the mixed function, but directly calls
bdrv_co_common_block_status_above(). bdrv_is_allocated() is only a
smaller wrapper anyway, so it wouldn't duplicate much code.

I seem to remember that Emanuele had a few more bdrv_is_allocated()
calls that actually took the coroutine path and could use the same new
function.

Kevin