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

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 <[email protected]>
>>> ---
>>>   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

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
>>
>>
>>
>>
> 
>