Re: [Qemu-devel] [PATCH 4/5] Convert block functions to coroutine versions

[Kevin Wolf <kwolf@redhat.com>]

Am 05.08.2013 um 20:44 hat Charlie Shepherd geschrieben:
> This patch follows on from the previous one and converts some block layer functions to be
> explicitly annotated with coroutine_fn instead of yielding depending upon calling context.
> ---
>  block.c               | 235 ++++++++++++++++++++++++++------------------------
>  block/mirror.c        |   4 +-
>  include/block/block.h |  37 ++++----
>  3 files changed, 148 insertions(+), 128 deletions(-)
> 
> diff --git a/block.c b/block.c
> index aaa122c..e7011f9 100644
> --- a/block.c
> +++ b/block.c
> @@ -164,7 +164,7 @@ bool bdrv_io_limits_enabled(BlockDriverState *bs)
>           || io_limits->iops[BLOCK_IO_LIMIT_TOTAL];
>  }
>  
> -static void bdrv_io_limits_intercept(BlockDriverState *bs,
> +static void coroutine_fn bdrv_io_limits_intercept(BlockDriverState *bs,
>                                       bool is_write, int nb_sectors)
>  {
>      int64_t wait_time = -1;
> @@ -364,7 +364,7 @@ BlockDriver *bdrv_find_whitelisted_format(const char *format_name,
>  
>  typedef struct CreateCo {
>      BlockDriver *drv;
> -    char *filename;
> +    const char *filename;
>      QEMUOptionParameter *options;
>      int ret;
>  } CreateCo;

Like Gabriel said, this should be a separate patch. Keeping it in this
series is probably easiest.

> @@ -372,48 +372,48 @@ typedef struct CreateCo {
>  static void coroutine_fn bdrv_create_co_entry(void *opaque)
>  {
>      CreateCo *cco = opaque;
> -    assert(cco->drv);
> -
> -    cco->ret = cco->drv->bdrv_co_create(cco->filename, cco->options);
> +    cco->ret = bdrv_create(cco->drv, cco->filename, cco->options);
>  }
>  
> -int bdrv_create(BlockDriver *drv, const char* filename,
> +int coroutine_fn bdrv_create(BlockDriver *drv, const char* filename,
>      QEMUOptionParameter *options)
>  {
>      int ret;
> +    char *dup_fn;
> +
> +    assert(drv);
> +    if (!drv->bdrv_co_create) {
> +        return -ENOTSUP;
> +    }
>  
> +    dup_fn = g_strdup(filename);
> +    ret = drv->bdrv_co_create(dup_fn, options);
> +    g_free(dup_fn);
> +    return ret;
> +}
> +
> +
> +int bdrv_create_sync(BlockDriver *drv, const char* filename,
> +    QEMUOptionParameter *options)

bdrv_foo_sync is an unfortunate naming convention, because
bdrv_pwrite_sync already exists and means something totally different:
It's a write directly followed by a disk flush.

> diff --git a/include/block/block.h b/include/block/block.h
> index dd8eca1..57d8ba2 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -125,9 +125,9 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
>  void bdrv_delete(BlockDriverState *bs);
>  int bdrv_parse_cache_flags(const char *mode, int *flags);
>  int bdrv_parse_discard_flags(const char *mode, int *flags);
> -int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> +int coroutine_fn bdrv_file_open(BlockDriverState **pbs, const char *filename,
>                     QDict *options, int flags);
> -int bdrv_open_backing_file(BlockDriverState *bs, QDict *options);
> +int coroutine_fn bdrv_open_backing_file(BlockDriverState *bs, QDict *options);
>  int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>                int flags, BlockDriver *drv);
>  BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
> @@ -150,18 +150,24 @@ void bdrv_dev_eject_request(BlockDriverState *bs, bool force);
>  bool bdrv_dev_has_removable_media(BlockDriverState *bs);
>  bool bdrv_dev_is_tray_open(BlockDriverState *bs);
>  bool bdrv_dev_is_medium_locked(BlockDriverState *bs);
> -int bdrv_read(BlockDriverState *bs, int64_t sector_num,
> +int coroutine_fn bdrv_read(BlockDriverState *bs, int64_t sector_num,
>                uint8_t *buf, int nb_sectors);
> -int bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num,
> +int bdrv_read_sync(BlockDriverState *bs, int64_t sector_num,
> +              uint8_t *buf, int nb_sectors);
> +int coroutine_fn bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num,
>                            uint8_t *buf, int nb_sectors);
> -int bdrv_write(BlockDriverState *bs, int64_t sector_num,
> +int coroutine_fn bdrv_write(BlockDriverState *bs, int64_t sector_num,
> +               const uint8_t *buf, int nb_sectors);
> +int bdrv_write_sync(BlockDriverState *bs, int64_t sector_num,
>                 const uint8_t *buf, int nb_sectors);
> -int bdrv_writev(BlockDriverState *bs, int64_t sector_num, QEMUIOVector *qiov);
> -int bdrv_pread(BlockDriverState *bs, int64_t offset,
> +int coroutine_fn bdrv_writev(BlockDriverState *bs, int64_t sector_num, QEMUIOVector *qiov);
> +int coroutine_fn bdrv_pread(BlockDriverState *bs, int64_t offset,
> +               void *buf, int count);
> +int bdrv_pread_sync(BlockDriverState *bs, int64_t offset,
>                 void *buf, int count);

I haven't checked everything, but bdrv_pread_sync is an example of a
declaration without any user and without implementation.

Proper patch splitting will make review of such things easier, so I'll
defer thorough review until then.

Kevin