Am 05.08.2013 um 20:44 hat Charlie Shepherd geschrieben:
> This patch converts the .bdrv_open, .bdrv_file_open and .bdrv_create members
> of struct BlockDriver
> to be explicitly annotated as coroutine_fn, rather than yielding dynamically
> depending on whether
> they are executed in a coroutine context or not.
I wonder whether this patch should be split into one for each converted
BlockDriver function. It would probably make review easier.
For those function where you actually just correct the coroutine_fn
annotation, but whose behaviour doesn't change, a single patch for all
might be enough.
> block.c | 16 +++++++--------
> block/blkdebug.c | 10 ++++-----
> block/blkverify.c | 4 ++--
> block/bochs.c | 8 ++++----
> block/cloop.c | 6 +++---
> block/cow.c | 12 +++++------
> block/curl.c | 12 +++++------
> block/dmg.c | 6 +++---
> block/nbd.c | 28 ++++++++++++-------------
> block/parallels.c | 6 +++---
> block/qcow.c | 8 ++++----
> block/qcow2-cluster.c | 8 ++++----
> block/qcow2.c | 48
> +++++++++++++++++++++++++++++++++++++------
> block/qcow2.h | 4 ++--
> block/qed.c | 8 ++++----
> block/raw-posix.c | 34 +++++++++++++++---------------
> block/raw.c | 8 ++++----
> block/sheepdog.c | 24 +++++++++++-----------
> block/snapshot.c | 32 ++++++++++++++++++++++++++++-
> block/ssh.c | 14 ++++++-------
> block/vdi.c | 12 +++++------
> block/vhdx.c | 4 ++--
> block/vmdk.c | 12 +++++------
> block/vpc.c | 12 +++++------
> block/vvfat.c | 12 +++++------
> include/block/block_int.h | 10 ++++-----
> include/block/coroutine.h | 4 ++--
> include/block/coroutine_int.h | 2 +-
> qemu-coroutine-lock.c | 4 ++--
> 30 files changed, 218 insertions(+), 152 deletions(-)
>
> diff --git a/block.c b/block.c
> index 6c493ad..aaa122c 100644
> --- a/block.c
> +++ b/block.c
> @@ -374,7 +374,7 @@ static void coroutine_fn bdrv_create_co_entry(void
> *opaque)
> CreateCo *cco = opaque;
> assert(cco->drv);
>
> - cco->ret = cco->drv->bdrv_create(cco->filename, cco->options);
> + cco->ret = cco->drv->bdrv_co_create(cco->filename, cco->options);
> }
>
> int bdrv_create(BlockDriver *drv, const char* filename,
> @@ -390,7 +390,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
> .ret = NOT_DONE,
> };
>
> - if (!drv->bdrv_create) {
> + if (!drv->bdrv_co_create) {
> ret = -ENOTSUP;
> goto out;
> }
> @@ -697,7 +697,7 @@ static int bdrv_open_common(BlockDriverState *bs,
> BlockDriverState *file,
> /* bdrv_open() with directly using a protocol as drv. This layer is
> already
> * opened, so assign it to bs (while file becomes a closed
> BlockDriverState)
> * and return immediately. */
> - if (file != NULL && drv->bdrv_file_open) {
> + if (file != NULL && drv->bdrv_co_file_open) {
> bdrv_swap(file, bs);
> return 0;
> }
> @@ -728,10 +728,10 @@ static int bdrv_open_common(BlockDriverState *bs,
> BlockDriverState *file,
> bs->enable_write_cache = !!(flags & BDRV_O_CACHE_WB);
>
> /* Open the image, either directly or using a protocol */
> - if (drv->bdrv_file_open) {
> + if (drv->bdrv_co_file_open) {
> assert(file == NULL);
> assert(drv->bdrv_parse_filename || filename != NULL);
> - ret = drv->bdrv_file_open(bs, options, open_flags);
> + ret = drv->bdrv_co_file_open(bs, options, open_flags);
bdrv_open_common() isn't coroutine_fn, though?
Ah well, I see that you change it in a later patch. Please make sure
that the code is in a consistent state after each single commit.
For this series, I think this suggests that you indeed split by converted
function, but put everything related to this function in one patch. For
example, the bdrv_open_common() conversion would be in the same patch as
this hunk.
> } else {
> if (file == NULL) {
> qerror_report(ERROR_CLASS_GENERIC_ERROR, "Can't use '%s' as a "
> @@ -742,7 +742,7 @@ static int bdrv_open_common(BlockDriverState *bs,
> BlockDriverState *file,
> }
> assert(file != NULL);
> bs->file = file;
> - ret = drv->bdrv_open(bs, options, open_flags);
> + ret = drv->bdrv_co_open(bs, options, open_flags);
> }
>
> if (ret < 0) {
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 0eceefe..2ed0bb6 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -58,6 +58,10 @@ typedef struct {
> #define QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
> #define QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
>
> +
> +#define NOT_DONE 0x7fffffff
> +
> +
> static int qcow2_probe(const uint8_t *buf, int buf_size, const char
> *filename)
> {
> const QCowHeader *cow_header = (const void *)buf;
> @@ -315,7 +319,7 @@ static QemuOptsList qcow2_runtime_opts = {
> },
> };
>
> -static int qcow2_open(BlockDriverState *bs, QDict *options, int flags)
> +static int coroutine_fn qcow2_co_open(BlockDriverState *bs, QDict *options,
> int flags)
> {
> BDRVQcowState *s = bs->opaque;
> int len, i, ret = 0;
> @@ -590,6 +594,38 @@ static int qcow2_open(BlockDriverState *bs, QDict
> *options, int flags)
> return ret;
> }
>
> +struct QOpenCo {
> + BlockDriverState *bs;
> + QDict *options;
> + int flags;
> + int ret;
> +};
> +
> +static void coroutine_fn qcow2_co_open_entry(void *opaque)
> +{
> + struct QOpenCo *qo = opaque;
> +
> + qo->ret = qcow2_co_open(qo->bs, qo->options, qo->flags);
> +}
> +
> +static int qcow2_open(BlockDriverState *bs, QDict *options, int flags)
> +{
> + Coroutine *co;
> + struct QOpenCo qo = {
> + .bs = bs,
> + .options = options,
> + .flags = flags,
> + .ret = NOT_DONE,
> + };
> +
> + co = qemu_coroutine_create(qcow2_co_open_entry);
> + qemu_coroutine_enter(co, &qo);
> + while (qo.ret == NOT_DONE) {
> + qemu_aio_wait();
> + }
> + return qo.ret;
> +}
I think it would be better to convert qcow2_invalidate_cache() to
coroutine_fn (which you apparently do in patch 4) so that it can
directly call qcow2_co_open, and to keep this coroutine wrapper in
the block.c function.
Kevin
