Re: [PATCH v3 4/4] block: Mark 'block_resize' as coroutine

[Kevin Wolf]

Am 17.01.2020 um 06:44 hat Markus Armbruster geschrieben:
> Kevin Wolf <address@hidden> writes:
> 
> > Am 16.01.2020 um 16:13 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <address@hidden> writes:
> >> 
> >> > Am 16.01.2020 um 10:45 hat Markus Armbruster geschrieben:
> >> >> Kevin Wolf <address@hidden> writes:
> >> >> > block_resize is safe to run in a coroutine, so use it as an example 
> >> >> > for
> >> >> > the new 'coroutine': true annotation in the QAPI schema.
> >> >> >
> >> >> > Signed-off-by: Kevin Wolf <address@hidden>
> >> >> > Reviewed-by: Marc-André Lureau <address@hidden>
> >> >
> >> >> > diff --git a/blockdev.c b/blockdev.c
> >> >> > index 8e029e9c01..b5e5d1e072 100644
> >> >> > --- a/blockdev.c
> >> >> > +++ b/blockdev.c
> >> >> > @@ -3161,9 +3161,9 @@ void hmp_drive_del(Monitor *mon, const QDict 
> >> >> > *qdict)
> >> >> >      aio_context_release(aio_context);
> >> >> >  }
> >> >> >  
> >> >> > -void qmp_block_resize(bool has_device, const char *device,
> >> >> > -                      bool has_node_name, const char *node_name,
> >> >> > -                      int64_t size, Error **errp)
> >> >> > +void coroutine_fn qmp_block_resize(bool has_device, const char 
> >> >> > *device,
> >> >> > +                                   bool has_node_name, const char 
> >> >> > *node_name,
> >> >> > +                                   int64_t size, Error **errp)
> >> >> >  {
> >> >> >      Error *local_err = NULL;
> >> >> >      BlockBackend *blk = NULL;
> >> >> 
> >> >> Pardon my ignorant question: what exactly makes a function a
> >> >> coroutine_fn?
> >> >
> >> > When Stefan requested adding the coroutine_fn marker, it seemed to make
> >> > sense to me because the QMP dispatcher will always call it from
> >> > coroutine context now, and being always run in coroutine context makes a
> >> > function a coroutine_fn.
> >> >
> >> > However, it's also called from hmp_block_resize(), so at least for now
> >> > coroutine_fn is actually wrong.
> >> 
> >> This answers the question when we mark a function a coroutine_fn.  I
> >> meant to ask what conditions the function itself must satisfy to be
> >> eligible for this mark.
> >
> > The requirement is actually not about the function itself, it's about
> > the callers, as stated above.
> >
> > But being a coroutine_fn allows the function to call other functions
> > that only work in coroutine context (other coroutine_fns). In the end
> > the reason why a function only works in coroutine context is usually
> > that it (or any other coroutine_fns called by it) could yield, which
> > obviously doesn't work outside of coroutine contest.
> 
> Thanks.
> 
> I think "being always run in coroutine context makes a function a
> coroutine_fn" is inaccurate.  It's "calling a coroutine_fn without
> switching to coroutine context first when not already in coroutine
> context".  The induction terminates at basic coroutine_fn like
> qemu_coroutine_yield().

I think we would tend to mark things as coroutine_fn even if they don't
call other coroutine_fns (yet), but would be allowed to. But this is now
really splitting hairs.

Maybe I should just have referred to the documentation in the source:

/**
 * Mark a function that executes in coroutine context
 *
 * Functions that execute in coroutine context cannot be called directly from
 * normal functions.  In the future it would be nice to enable compiler or
 * static checker support for catching such errors.  This annotation might make
 * it possible and in the meantime it serves as documentation.
 *
 * For example:
 *
 *   static void coroutine_fn foo(void) {
 *       ....
 *   }
 */
#define coroutine_fn

Kevin