[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Charlie Shepherd
Subject: Re: [Qemu-devel] [PATCH 4/5] Convert block functions to coroutine versions
Date: Thu, 08 Aug 2013 02:17:43 +0100
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20130620 Thunderbird/17.0.7

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]