Re: [PATCH 7/8] hw/9pfs: annotate V9fsTransport callbacks as coroutine_fn

[Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>]

Hi

On Tue, Jul 21, 2026 at 11:58 AM Marc-André Lureau
<marcandre.lureau@xxxxxxxxxx> wrote:
>
> Hi
>
> On Tue, Jul 21, 2026 at 12:54 AM Philippe Mathieu-Daudé
> <philmd@xxxxxxxxxxxxxxxx> wrote:
> >
> > On 20/7/26 22:42, Philippe Mathieu-Daudé wrote:
> > > On 20/7/26 16:02, Christian Schoenebeck wrote:
> > >> On Monday, 20 July 2026 09:55:36 CEST Marc-André Lureau wrote:
> > >>> All V9fsTransport callbacks are invoked exclusively from coroutine
> > >>> context (the v9fs_* PDU handlers). Annotate the function pointer
> > >>> types in V9fsTransport and all implementations (virtio and xen
> > >>> backends), as well as intermediate callers in 9p.c (pdu_marshal,
> > >>> pdu_unmarshal, v9fs_init_qiov_from_pdu, etc.).
> > >>>
> > >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> > >>> ---
> > >>>   hw/9pfs/9p.h               | 26 +++++++++++++++-----------
> > >>>   hw/9pfs/9p.c               | 31 +++++++++++++++++++------------
> > >>>   hw/9pfs/virtio-9p-device.c | 24 +++++++++++++-----------
> > >>>   hw/9pfs/xen-9p-backend.c   | 34 +++++++++++++++++-----------------
> > >>>   4 files changed, 64 insertions(+), 51 deletions(-)
> > >>>
> > >>> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> > >>> index 1a309664f6e..0e52ffbdf38 100644
> > >>> --- a/hw/9pfs/9p.h
> > >>> +++ b/hw/9pfs/9p.h
> > >>> @@ -472,17 +472,21 @@ void pdu_submit(V9fsPDU *pdu, P9MsgHeader *hdr);
> > >>>   void v9fs_reset(V9fsState *s);
> > >>>
> > >>>
> > >>>   struct V9fsTransport {
> > >>> -    ssize_t     (*pdu_vmarshal)(V9fsPDU *pdu, size_t offset, const
> > >>> char *fmt,
> > >>> -                                va_list ap);
> > >>> -    ssize_t     (*pdu_vunmarshal)(V9fsPDU *pdu, size_t offset, const
> > >>> char *fmt,
> > >>> -                                  va_list ap);
> > >>> -    void        (*init_in_iov_from_pdu)(V9fsPDU *pdu, struct iovec
> > >>> **piov,
> > >>> -                                        unsigned int *pniov, size_t
> > >>> size);
> > >>> -    void        (*init_out_iov_from_pdu)(V9fsPDU *pdu, struct iovec
> > >>> **piov,
> > >>> -                                         unsigned int *pniov, size_t
> > >>> size);
> > >>> -    void        (*push_and_notify)(V9fsPDU *pdu);
> > >>> -    size_t      (*msize_limit)(V9fsState *s);
> > >>> -    size_t      (*response_buffer_size)(V9fsPDU *pdu);
> > >>> +    ssize_t     coroutine_fn (*pdu_vmarshal)(V9fsPDU *pdu, size_t
> > >>> offset,
> > >>> +                                             const char *fmt,
> > >>> va_list ap);
> > >>> +    ssize_t     coroutine_fn (*pdu_vunmarshal)(V9fsPDU *pdu, size_t
> > >>> offset,
> > >>> +                                               const char *fmt,
> > >>> va_list ap);
> > >>> +    void        coroutine_fn (*init_in_iov_from_pdu)(V9fsPDU *pdu,
> > >>> +                                                     struct iovec
> > >>> **piov,
> > >>> +                                                     unsigned int
> > >>> *pniov,
> > >>> +                                                     size_t size);
> > >>> +    void        coroutine_fn (*init_out_iov_from_pdu)(V9fsPDU *pdu,
> > >>> +                                                      struct iovec
> > >>> **piov,
> > >>> +                                                      unsigned int
> > >>> *pniov,
> > >>> +                                                      size_t size);
> > >>> +    void        coroutine_fn (*push_and_notify)(V9fsPDU *pdu);
> > >>> +    size_t      coroutine_fn (*msize_limit)(V9fsState *s);
> > >>> +    size_t      coroutine_fn (*response_buffer_size)(V9fsPDU *pdu);
> > >>>   };
> > >>
> > >> I wonder whether that could be made more diff/blame-friendly.
> > >> Especially as
> > >> this currently only serves documentation purposes.
> > >>
> > >> They way you are positioning coroutine_fn it usually resolves to an
> > >> attribute
> > >> attached to the function's return type, not to the function itself, even
> > >> though clang/gcc handling it flexible. For attaching an attribute to the
> > >> function itself the attribute should either be attached before the
> > >> function
> > >> declaration or after the function declaration.
> > >
> > > I tend to aggree it would be clearer to have these attributes /before/
> > > the function prototype declaration.
> >
> > $ git grep -E '^(static )?coroutine_(mixed_)?fn' | wc -l
> >        90
>
> More like
> $  git grep -rnE '\b(coroutine_fn|co_wrapper[a-z_]*)\b' | wc -l
> 1343
>

That matching is not accurate, it's actually hard enough to match all
annotations instances.. but this should be a rough estimation.

> most of them are "return-type co_annotation fn(..)"
> we even have scripts/block-coroutine-wrapper.py that relies and
> produce this order..
> a bit late to change the style imho...
>
> At least, let's not make this a requirement for this series, please
>
> >> block/block-copy.c:47:static coroutine_fn int
> > block_copy_task_entry(AioTask *task);
> > block/copy-before-write.c:106:static coroutine_fn int
> > cbw_do_copy_before_write(BlockDriverState *bs,
> > block/nvme.c:1351:static coroutine_fn int nvme_co_flush(BlockDriverState
> > *bs)
> > block/ssh.c:1223:static coroutine_fn int ssh_flush(BDRVSSHState *s,
> > BlockDriverState *bs)
> > nbd/server.c:3077:static coroutine_fn void nbd_trip(void *opaque)
> > block/block-copy.c:581:static coroutine_fn int
> > block_copy_task_entry(AioTask *task)
> > ...
> >
> > >
> > >> In both cases you could then
> > >> add it to its own, separate line, avoiding a lot of noise.
> > >>
> > >> /Christian
> > >>
> > >>
> > >>
> > >>
> > >
> > >
> >