Re: [PATCH 02/26] block: add missing coroutine_fn annotations

2022-10-05 Thread Alberto Campinho Faria 
On Sat, Sep 24, 2022 at 2:42 PM Paolo Bonzini  wrote:
> Because at the time I wrote the patch, blk_pwrite_zeroes() was a
> coroutine_fn. :)
>
> It is called from bdrv_co_create_opts_simple which is coroutine_fn and
> performs I/O, so it should be a coroutine_fn. I have a few more patches
> to not go through the generated_co_wrappers, but I was curious to see
> if we could automate those changes through your tool.

The static analyzer [1] should find all such cases, e.g.:

./static-analyzer.py -c no_coroutine_fn build block

coroutine_fn and generated_co_wrapper must be adjusted for this to
work on master:

#define coroutine_fn __attribute__((__annotate__("coroutine_fn")))
#define generated_co_wrapper
__attribute__((__annotate__("no_coroutine_fn")))

[1] https://gitlab.com/albertofaria/qemu/-/tree/static-analysis

Re: [PATCH 02/26] block: add missing coroutine_fn annotations

2022-09-24 Thread Paolo Bonzini 
On Thu, Sep 22, 2022 at 5:12 PM Alberto Campinho Faria
 wrote:
>
> On Thu, Sep 22, 2022 at 9:49 AM Paolo Bonzini  wrote:
> > Callers of coroutine_fn must be coroutine_fn themselves, or the call
> > must be within "if (qemu_in_coroutine())".  Apply coroutine_fn to
> > functions where this holds.
> >
> > Signed-off-by: Paolo Bonzini 
> > ---
> >  block.c   |  6 +++---
> >  block/block-backend.c | 10 +-
> >  block/io.c| 22 +++---
> >  3 files changed, 19 insertions(+), 19 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index bc85f46eed..5c34ada53f 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -631,9 +631,9 @@ static int64_t 
> > create_file_fallback_truncate(BlockBackend *blk,
> >   * Helper function for bdrv_create_file_fallback(): Zero the first
> >   * sector to remove any potentially pre-existing image header.
> >   */
> > -static int create_file_fallback_zero_first_sector(BlockBackend *blk,
> > -  int64_t current_size,
> > -  Error **errp)
> > +static int coroutine_fn 
> > create_file_fallback_zero_first_sector(BlockBackend *blk,
> > +   int64_t 
> > current_size,
> > +   Error 
> > **errp)
>
> Why mark this coroutine_fn? Maybe the intention was to also replace
> the call to blk_pwrite_zeroes() with blk_co_pwrite_zeroes()?

Because at the time I wrote the patch, blk_pwrite_zeroes() was a
coroutine_fn. :)

It is called from bdrv_co_create_opts_simple which is coroutine_fn and
performs I/O, so it should be a coroutine_fn. I have a few more patches
to not go through the generated_co_wrappers, but I was curious to see
if we could automate those changes through your tool.

Paolo

>
> Regardless:
>
> Reviewed-by: Alberto Faria 
>

Re: [PATCH 02/26] block: add missing coroutine_fn annotations

2022-09-22 Thread Alberto Campinho Faria 
On Thu, Sep 22, 2022 at 9:49 AM Paolo Bonzini  wrote:
> Callers of coroutine_fn must be coroutine_fn themselves, or the call
> must be within "if (qemu_in_coroutine())".  Apply coroutine_fn to
> functions where this holds.
>
> Signed-off-by: Paolo Bonzini 
> ---
>  block.c   |  6 +++---
>  block/block-backend.c | 10 +-
>  block/io.c| 22 +++---
>  3 files changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/block.c b/block.c
> index bc85f46eed..5c34ada53f 100644
> --- a/block.c
> +++ b/block.c
> @@ -631,9 +631,9 @@ static int64_t create_file_fallback_truncate(BlockBackend 
> *blk,
>   * Helper function for bdrv_create_file_fallback(): Zero the first
>   * sector to remove any potentially pre-existing image header.
>   */
> -static int create_file_fallback_zero_first_sector(BlockBackend *blk,
> -  int64_t current_size,
> -  Error **errp)
> +static int coroutine_fn create_file_fallback_zero_first_sector(BlockBackend 
> *blk,
> +   int64_t 
> current_size,
> +   Error **errp)

Why mark this coroutine_fn? Maybe the intention was to also replace
the call to blk_pwrite_zeroes() with blk_co_pwrite_zeroes()?

Regardless:

Reviewed-by: Alberto Faria

[PATCH 02/26] block: add missing coroutine_fn annotations

2022-09-22 Thread Paolo Bonzini 
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())".  Apply coroutine_fn to
functions where this holds.

Signed-off-by: Paolo Bonzini 
---
 block.c   |  6 +++---
 block/block-backend.c | 10 +-
 block/io.c| 22 +++---
 3 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/block.c b/block.c
index bc85f46eed..5c34ada53f 100644
--- a/block.c
+++ b/block.c
@@ -631,9 +631,9 @@ static int64_t create_file_fallback_truncate(BlockBackend 
*blk,
  * Helper function for bdrv_create_file_fallback(): Zero the first
  * sector to remove any potentially pre-existing image header.
  */
-static int create_file_fallback_zero_first_sector(BlockBackend *blk,
-  int64_t current_size,
-  Error **errp)
+static int coroutine_fn create_file_fallback_zero_first_sector(BlockBackend 
*blk,
+   int64_t 
current_size,
+   Error **errp)
 {
 int64_t bytes_to_clear;
 int ret;
diff --git a/block/block-backend.c b/block/block-backend.c
index d4a5df2ac2..aa4adf06ae 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1546,7 +1546,7 @@ static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, 
int64_t offset,
 return >common;
 }
 
-static void blk_aio_read_entry(void *opaque)
+static void coroutine_fn blk_aio_read_entry(void *opaque)
 {
 BlkAioEmAIOCB *acb = opaque;
 BlkRwCo *rwco = >rwco;
@@ -1558,7 +1558,7 @@ static void blk_aio_read_entry(void *opaque)
 blk_aio_complete(acb);
 }
 
-static void blk_aio_write_entry(void *opaque)
+static void coroutine_fn blk_aio_write_entry(void *opaque)
 {
 BlkAioEmAIOCB *acb = opaque;
 BlkRwCo *rwco = >rwco;
@@ -1669,7 +1669,7 @@ int coroutine_fn blk_co_ioctl(BlockBackend *blk, unsigned 
long int req,
 return ret;
 }
 
-static void blk_aio_ioctl_entry(void *opaque)
+static void coroutine_fn blk_aio_ioctl_entry(void *opaque)
 {
 BlkAioEmAIOCB *acb = opaque;
 BlkRwCo *rwco = >rwco;
@@ -1703,7 +1703,7 @@ blk_co_do_pdiscard(BlockBackend *blk, int64_t offset, 
int64_t bytes)
 return bdrv_co_pdiscard(blk->root, offset, bytes);
 }
 
-static void blk_aio_pdiscard_entry(void *opaque)
+static void coroutine_fn blk_aio_pdiscard_entry(void *opaque)
 {
 BlkAioEmAIOCB *acb = opaque;
 BlkRwCo *rwco = >rwco;
@@ -1747,7 +1747,7 @@ static int coroutine_fn blk_co_do_flush(BlockBackend *blk)
 return bdrv_co_flush(blk_bs(blk));
 }
 
-static void blk_aio_flush_entry(void *opaque)
+static void coroutine_fn blk_aio_flush_entry(void *opaque)
 {
 BlkAioEmAIOCB *acb = opaque;
 BlkRwCo *rwco = >rwco;
diff --git a/block/io.c b/block/io.c
index 0a8cbefe86..e3dcb8e7e6 100644
--- a/block/io.c
+++ b/block/io.c
@@ -751,11 +751,11 @@ static void coroutine_fn 
tracked_request_end(BdrvTrackedRequest *req)
 /**
  * Add an active request to the tracked requests list
  */
-static void tracked_request_begin(BdrvTrackedRequest *req,
-  BlockDriverState *bs,
-  int64_t offset,
-  int64_t bytes,
-  enum BdrvTrackedRequestType type)
+static void coroutine_fn tracked_request_begin(BdrvTrackedRequest *req,
+   BlockDriverState *bs,
+   int64_t offset,
+   int64_t bytes,
+   enum BdrvTrackedRequestType 
type)
 {
 bdrv_check_request(offset, bytes, _abort);
 
@@ -794,7 +794,7 @@ static bool tracked_request_overlaps(BdrvTrackedRequest 
*req,
 }
 
 /* Called with self->bs->reqs_lock held */
-static BdrvTrackedRequest *
+static coroutine_fn BdrvTrackedRequest *
 bdrv_find_conflicting_request(BdrvTrackedRequest *self)
 {
 BdrvTrackedRequest *req;
@@ -1644,10 +1644,10 @@ static bool bdrv_init_padding(BlockDriverState *bs,
 return true;
 }
 
-static int bdrv_padding_rmw_read(BdrvChild *child,
- BdrvTrackedRequest *req,
- BdrvRequestPadding *pad,
- bool zero_middle)
+static coroutine_fn int bdrv_padding_rmw_read(BdrvChild *child,
+  BdrvTrackedRequest *req,
+  BdrvRequestPadding *pad,
+  bool zero_middle)
 {
 QEMUIOVector local_qiov;
 BlockDriverState *bs = child->bs;
@@ -3168,7 +3168,7 @@ out:
 return ret;
 }
 
-int bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf)
+int coroutine_fn bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf)
 {
 BlockDriver *drv = bs->drv;
 CoroutineIOCompletion co = {
--