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

[Charlie Shepherd]

On 06/08/2013 10:36, Kevin Wolf wrote:
> 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.

Yes it's not a great naming convention. I could rename bdrv_pwrite_sync 
to something like bdrv_pwrite_and_sync? Or is that too horrible?
An alternative would be bdrv_foo and bdrv_sync_foo? But bdrv_sync_pwrite 
and bdrv_pwrite_sync would be quite confusing in a hurry.

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

Yes my bad, hopefully splitting them as per your other email will catch 
these issues.