Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/block/block_int.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 15fa602150..93eb2a9528 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -133,15 +133,15 @@ struct BlockDriver {
     void (*bdrv_refresh_filename)(BlockDriverState *bs, QDict *options);
 
     /* aio */
-    BlockAIOCB *(*bdrv_aio_readv)(BlockDriverState *bs,
+    BlockAIOCB * coroutine_fn (*bdrv_aio_readv)(BlockDriverState *bs,
         int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
         BlockCompletionFunc *cb, void *opaque);
-    BlockAIOCB *(*bdrv_aio_writev)(BlockDriverState *bs,
+    BlockAIOCB * coroutine_fn (*bdrv_aio_writev)(BlockDriverState *bs,
         int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
         BlockCompletionFunc *cb, void *opaque);
-    BlockAIOCB *(*bdrv_aio_flush)(BlockDriverState *bs,
+    BlockAIOCB * coroutine_fn (*bdrv_aio_flush)(BlockDriverState *bs,
         BlockCompletionFunc *cb, void *opaque);
-    BlockAIOCB *(*bdrv_aio_pdiscard)(BlockDriverState *bs,
+    BlockAIOCB * coroutine_fn (*bdrv_aio_pdiscard)(BlockDriverState *bs,
         int64_t offset, int bytes,
         BlockCompletionFunc *cb, void *opaque);
 
@@ -247,7 +247,7 @@ struct BlockDriver {
     void (*bdrv_lock_medium)(BlockDriverState *bs, bool locked);
 
     /* to control generic scsi devices */
-    BlockAIOCB *(*bdrv_aio_ioctl)(BlockDriverState *bs,
+    BlockAIOCB * coroutine_fn (*bdrv_aio_ioctl)(BlockDriverState *bs,
         unsigned long int req, void *buf,
         BlockCompletionFunc *cb, void *opaque);
     int coroutine_fn (*bdrv_co_ioctl)(BlockDriverState *bs,
-- 
2.13.1.395.gf7b71de06

On 05/07/2017 00:03, Marc-André Lureau wrote:
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/block/block_int.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 15fa602150..93eb2a9528 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -133,15 +133,15 @@ struct BlockDriver {
>      void (*bdrv_refresh_filename)(BlockDriverState *bs, QDict *options);
>  
>      /* aio */
> -    BlockAIOCB *(*bdrv_aio_readv)(BlockDriverState *bs,
> +    BlockAIOCB * coroutine_fn (*bdrv_aio_readv)(BlockDriverState *bs,
>          int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
>          BlockCompletionFunc *cb, void *opaque);
> -    BlockAIOCB *(*bdrv_aio_writev)(BlockDriverState *bs,
> +    BlockAIOCB * coroutine_fn (*bdrv_aio_writev)(BlockDriverState *bs,
>          int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
>          BlockCompletionFunc *cb, void *opaque);
> -    BlockAIOCB *(*bdrv_aio_flush)(BlockDriverState *bs,
> +    BlockAIOCB * coroutine_fn (*bdrv_aio_flush)(BlockDriverState *bs,
>          BlockCompletionFunc *cb, void *opaque);
> -    BlockAIOCB *(*bdrv_aio_pdiscard)(BlockDriverState *bs,
> +    BlockAIOCB * coroutine_fn (*bdrv_aio_pdiscard)(BlockDriverState *bs,
>          int64_t offset, int bytes,
>          BlockCompletionFunc *cb, void *opaque);
>  
> @@ -247,7 +247,7 @@ struct BlockDriver {
>      void (*bdrv_lock_medium)(BlockDriverState *bs, bool locked);
>  
>      /* to control generic scsi devices */
> -    BlockAIOCB *(*bdrv_aio_ioctl)(BlockDriverState *bs,
> +    BlockAIOCB * coroutine_fn (*bdrv_aio_ioctl)(BlockDriverState *bs,
>          unsigned long int req, void *buf,
>          BlockCompletionFunc *cb, void *opaque);
>      int coroutine_fn (*bdrv_co_ioctl)(BlockDriverState *bs,
> 


They are, but it's an implementation detail.  Why is this patch necessary?

Thanks,

Paolo

----- Original Message -----
> On 05/07/2017 00:03, Marc-André Lureau wrote:
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  include/block/block_int.h | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index 15fa602150..93eb2a9528 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -133,15 +133,15 @@ struct BlockDriver {
> >      void (*bdrv_refresh_filename)(BlockDriverState *bs, QDict *options);
> >  
> >      /* aio */
> > -    BlockAIOCB *(*bdrv_aio_readv)(BlockDriverState *bs,
> > +    BlockAIOCB * coroutine_fn (*bdrv_aio_readv)(BlockDriverState *bs,
> >          int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
> >          BlockCompletionFunc *cb, void *opaque);
> > -    BlockAIOCB *(*bdrv_aio_writev)(BlockDriverState *bs,
> > +    BlockAIOCB * coroutine_fn (*bdrv_aio_writev)(BlockDriverState *bs,
> >          int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
> >          BlockCompletionFunc *cb, void *opaque);
> > -    BlockAIOCB *(*bdrv_aio_flush)(BlockDriverState *bs,
> > +    BlockAIOCB * coroutine_fn (*bdrv_aio_flush)(BlockDriverState *bs,
> >          BlockCompletionFunc *cb, void *opaque);
> > -    BlockAIOCB *(*bdrv_aio_pdiscard)(BlockDriverState *bs,
> > +    BlockAIOCB * coroutine_fn (*bdrv_aio_pdiscard)(BlockDriverState *bs,
> >          int64_t offset, int bytes,
> >          BlockCompletionFunc *cb, void *opaque);
> >  
> > @@ -247,7 +247,7 @@ struct BlockDriver {
> >      void (*bdrv_lock_medium)(BlockDriverState *bs, bool locked);
> >  
> >      /* to control generic scsi devices */
> > -    BlockAIOCB *(*bdrv_aio_ioctl)(BlockDriverState *bs,
> > +    BlockAIOCB * coroutine_fn (*bdrv_aio_ioctl)(BlockDriverState *bs,
> >          unsigned long int req, void *buf,
> >          BlockCompletionFunc *cb, void *opaque);
> >      int coroutine_fn (*bdrv_co_ioctl)(BlockDriverState *bs,
> > 
> 
> 
> They are, but it's an implementation detail.  Why is this patch necessary?

I didn't think this would be controversial :) well, the checks I added to clang verify function pointer share the coroutine attribute.

The function themself are/need to be coroutine_fn (as they will call coroutine_fn too)

On 05/07/2017 16:21, Marc-André Lureau wrote:
>>
>> They are, but it's an implementation detail.  Why is this patch necessary?
> I didn't think this would be controversial :) well, the checks I added to clang verify function pointer share the coroutine attribute.
> 
> The function themself are/need to be coroutine_fn (as they will call coroutine_fn too)

It's not controversial, I would not have expected the functions to call
coroutine_fn. :)  How do they do that?

Paolo

Hi

----- Original Message -----
> 
> 
> On 05/07/2017 16:21, Marc-André Lureau wrote:
> >>
> >> They are, but it's an implementation detail.  Why is this patch necessary?
> > I didn't think this would be controversial :) well, the checks I added to
> > clang verify function pointer share the coroutine attribute.
> > 
> > The function themself are/need to be coroutine_fn (as they will call
> > coroutine_fn too)
> 
> It's not controversial, I would not have expected the functions to call
> coroutine_fn. :)  How do they do that?
> 

For example,  null_co_readv() calls  null_co_common() which calls co_aio_sleep_ns()

On 05/07/2017 18:06, Marc-André Lureau wrote:
>>> coroutine_fn too)
>> It's not controversial, I would not have expected the functions to call
>> coroutine_fn. :)  How do they do that?
>>
> For example,  null_co_readv() calls  null_co_common() which calls co_aio_sleep_ns()

But these are bdrv_co_*, not bdrv_aio_*.

Paolo

Hi

----- Original Message -----
> 
> 
> On 05/07/2017 18:06, Marc-André Lureau wrote:
> >>> coroutine_fn too)
> >> It's not controversial, I would not have expected the functions to call
> >> coroutine_fn. :)  How do they do that?
> >>
> > For example,  null_co_readv() calls  null_co_common() which calls
> > co_aio_sleep_ns()
> 
> But these are bdrv_co_*, not bdrv_aio_*.

Oops, right.

Indeed, it's not needed, but to avoid coroutine annotation mismatch, we would have to remove a few:

static coroutine_fn BlockAIOCB *raw_aio_pdiscard(BlockDriverState *bs,

static coroutine_fn BlockAIOCB *hdev_aio_pdiscard(BlockDriverState *bs,

Only those 2, it seems.

On 05/07/2017 18:40, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
>>
>>
>> On 05/07/2017 18:06, Marc-André Lureau wrote:
>>>>> coroutine_fn too)
>>>> It's not controversial, I would not have expected the functions to call
>>>> coroutine_fn. :)  How do they do that?
>>>>
>>> For example,  null_co_readv() calls  null_co_common() which calls
>>> co_aio_sleep_ns()
>>
>> But these are bdrv_co_*, not bdrv_aio_*.
> 
> Oops, right.
> 
> Indeed, it's not needed, but to avoid coroutine annotation mismatch, we would have to remove a few:
> 
> static coroutine_fn BlockAIOCB *raw_aio_pdiscard(BlockDriverState *bs,
> 
> static coroutine_fn BlockAIOCB *hdev_aio_pdiscard(BlockDriverState *bs,
> 
> Only those 2, it seems.

Good!  If it's just those two, they are wrong indeed.  I'd be surprised
to see more (and even more surprised to see that the annotations were
right :)).

Paolo