Re: [PATCH 00/19] block: fix coroutine_fn annotations

[Stefan Hajnoczi]

On Fri, Apr 15, 2022 at 03:18:34PM +0200, Paolo Bonzini wrote:
> This is the initial result of reviving Marc-André's series at
> https://patchew.org/QEMU/20170704220346.29244-1-marcandre.lureau@redhat.com/.
> A lot of the patches are similar to the ones that Marc-André wrote,
> but due to the changes in the code it was easier to redo them.
> 
> For nbd, the patch is on top of "nbd: mark more coroutine_fns" that
> I sent a few days ago and that (AIUI) Eric has already queued; only
> one function was missing, much to my surprise.
> 
> Apart from this, I also identified the following functions that
> can be called both in coroutine context and outside:
> 
> - qmp_dispatch
> - schedule_next_request
> - nvme_get_free_req
> - bdrv_create
> - bdrv_remove_persistent_dirty_bitmap
> - bdrv_can_store_new_dirty_bitmap
> - bdrv_do_drained_begin
> - bdrv_do_drained_end
> - bdrv_drain_all_begin
> - qcow2_open
> - qcow2_has_zero_init
> - bdrv_qed_open
> - qio_channel_readv_full_all_eof
> - qio_channel_writev_full_all
> 
> besides, of course, everything that is generated by
> scripts/block-coroutine-wrapper.py.

This looks useful, thanks for bringing it back!

As Eric mentioned, the commits need justifications. The following cases
come to mind:

1. Add coroutine_fn because the function calls a function that is marked
   with coroutine_fn. This must be fixed because it can lead to crashes.

2. Remove coroutine_fn because the function does not call any functions
   marked with coroutine_fn. This is optional because it does not lead
   to crashes and maybe the author intended to be explicit that this
   function runs only in coroutine context even though it doesn't yield.

3. Variants of these cases but related to runtime qemu_in_coroutine()
   checks. Functions should not have coroutine_fn if they legitimately
   are called in both contexts. Any calls to coroutine_fn child
   functions must be conditional on qemu_in_coroutine() or something
   else that indicates whether we are running in coroutine context.

Stefan

signature.asc
Description: PGP signature