[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 03/10] block: Parse "backing" option to refer
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v7 03/10] block: Parse "backing" option to reference existing BDS |
Date: |
Thu, 12 Dec 2013 13:52:15 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) |
Fam Zheng <address@hidden> writes:
> Signed-off-by: Fam Zheng <address@hidden>
> ---
> block.c | 109
> +++++++++++++++++++++++++++++-----------------
> block/mirror.c | 2 +-
> include/block/block.h | 3 +-
> include/block/block_int.h | 3 ++
> 4 files changed, 76 insertions(+), 41 deletions(-)
>
> diff --git a/block.c b/block.c
> index c1a3576..41562fd 100644
> --- a/block.c
> +++ b/block.c
> @@ -961,63 +961,87 @@ fail:
> /*
> * Opens the backing file for a BlockDriverState if not yet open
> *
> + * If backing_bs is not NULL or empty, find the BDS by name and reference it
> as
> + * the backing_hd, in this case options is ignored.
> + *
The name "backing_bs" suggests this is a BlockDriverState, which it's
not. What about calling it "name", just like bdrv_find()'s parameter?
Or "device_name", like the BlockDriverState member?
> * options is a QDict of options to pass to the block drivers, or NULL for an
> * empty set of options. The reference to the QDict is transferred to this
> * function (even on failure), so if the caller intends to reuse the
> dictionary,
> * it needs to use QINCREF() before calling bdrv_file_open.
> */
> -int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error
> **errp)
> +int bdrv_open_backing_file(BlockDriverState *bs, const char *backing_bs,
> + QDict *options, Error **errp)
This function is now too overloaded for my taste.
Possibly cleaner:
* Have a new function to set backing_hd. Takes a device name argument,
or maybe a BDS. I'm leaning towards the latter.
* The new function and old bdrv_open_backing_file() have a common tail.
Factor it out into a helper.
> {
> char backing_filename[PATH_MAX];
> int back_flags, ret;
> BlockDriver *back_drv = NULL;
> Error *local_err = NULL;
>
> - if (bs->backing_hd != NULL) {
> - QDECREF(options);
> - return 0;
> - }
> + if (backing_bs && *backing_bs != '\0') {
> + bs->backing_hd = bdrv_find(backing_bs);
What if bs->backing_hd is already set?
> + if (!bs->backing_hd) {
> + error_setg(errp, "Backing device not found: %s", backing_bs);
> + return -ENOENT;
> + }
> + bdrv_ref(bs->backing_hd);
> + } else {
> + if (bs->backing_hd != NULL) {
> + QDECREF(options);
> + return 0;
> + }
>
> - /* NULL means an empty set of options */
> - if (options == NULL) {
> - options = qdict_new();
> - }
> + /* NULL means an empty set of options */
> + if (options == NULL) {
> + options = qdict_new();
> + }
>
> - bs->open_flags &= ~BDRV_O_NO_BACKING;
> - if (qdict_haskey(options, "file.filename")) {
> - backing_filename[0] = '\0';
> - } else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) {
> - QDECREF(options);
> - return 0;
> - } else {
> - bdrv_get_full_backing_filename(bs, backing_filename,
> - sizeof(backing_filename));
> - }
> + bs->open_flags &= ~BDRV_O_NO_BACKING;
> + if (qdict_haskey(options, "file.filename")) {
> + backing_filename[0] = '\0';
> + } else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) {
> + QDECREF(options);
> + return 0;
> + } else {
> + bdrv_get_full_backing_filename(bs, backing_filename,
> + sizeof(backing_filename));
> + }
>
> - bs->backing_hd = bdrv_new("");
> + bs->backing_hd = bdrv_new("");
>
> - if (bs->backing_format[0] != '\0') {
> - back_drv = bdrv_find_format(bs->backing_format);
> - }
> + if (bs->backing_format[0] != '\0') {
> + back_drv = bdrv_find_format(bs->backing_format);
> + }
>
> - /* backing files always opened read-only */
> - back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT |
> - BDRV_O_COPY_ON_READ);
> + /* backing files always opened read-only */
> + back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT |
> + BDRV_O_COPY_ON_READ);
>
> - ret = bdrv_open(bs->backing_hd,
> - *backing_filename ? backing_filename : NULL, options,
> - back_flags, back_drv, &local_err);
> - if (ret < 0) {
> - bdrv_unref(bs->backing_hd);
> - bs->backing_hd = NULL;
> - bs->open_flags |= BDRV_O_NO_BACKING;
> - error_setg(errp, "Could not open backing file: %s",
> - error_get_pretty(local_err));
> - error_free(local_err);
> - return ret;
> + ret = bdrv_open(bs->backing_hd,
> + *backing_filename ? backing_filename : NULL, options,
> + back_flags, back_drv, &local_err);
> + if (ret < 0) {
> + bdrv_unref(bs->backing_hd);
> + bs->backing_hd = NULL;
> + bs->open_flags |= BDRV_O_NO_BACKING;
> + error_setg(errp, "Could not open backing file: %s",
> + error_get_pretty(local_err));
> + error_free(local_err);
> + return ret;
> + }
> }
> + assert(bs->backing_hd);
> + assert(!bs->backing_blocker);
> + error_setg(&bs->backing_blocker,
> + "device is used as backing hd of '%s'",
> + bs->device_name);
> + bdrv_op_block_all(bs->backing_hd, bs->backing_blocker);
> + /* Otherwise we won't be able to commit due to check in bdrv_commit */
> + bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_COMMIT,
> + bs->backing_blocker);
> pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> bs->backing_hd->file->filename);
> + pstrcpy(bs->backing_format, sizeof(bs->backing_format),
> + bs->backing_hd->drv->format_name);
> return 0;
> }
>
This patch hunk is hard to review, because you had to indent the old
code. Same with whitespace differences ignored:
@@ -961,18 +961,30 @@
/*
* Opens the backing file for a BlockDriverState if not yet open
*
+ * If backing_bs is not NULL or empty, find the BDS by name and reference it
as
+ * the backing_hd, in this case options is ignored.
+ *
* options is a QDict of options to pass to the block drivers, or NULL for an
* empty set of options. The reference to the QDict is transferred to this
* function (even on failure), so if the caller intends to reuse the
dictionary,
* it needs to use QINCREF() before calling bdrv_file_open.
*/
-int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error
**errp)
+int bdrv_open_backing_file(BlockDriverState *bs, const char *backing_bs,
+ QDict *options, Error **errp)
{
char backing_filename[PATH_MAX];
int back_flags, ret;
BlockDriver *back_drv = NULL;
Error *local_err = NULL;
+ if (backing_bs && *backing_bs != '\0') {
+ bs->backing_hd = bdrv_find(backing_bs);
+ if (!bs->backing_hd) {
+ error_setg(errp, "Backing device not found: %s", backing_bs);
+ return -ENOENT;
+ }
+ bdrv_ref(bs->backing_hd);
+ } else {
if (bs->backing_hd != NULL) {
QDECREF(options);
return 0;
@@ -1016,8 +1028,20 @@
error_free(local_err);
return ret;
}
+ }
+ assert(bs->backing_hd);
+ assert(!bs->backing_blocker);
+ error_setg(&bs->backing_blocker,
+ "device is used as backing hd of '%s'",
+ bs->device_name);
+ bdrv_op_block_all(bs->backing_hd, bs->backing_blocker);
+ /* Otherwise we won't be able to commit due to check in bdrv_commit */
+ bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_COMMIT,
+ bs->backing_blocker);
pstrcpy(bs->backing_file, sizeof(bs->backing_file),
bs->backing_hd->file->filename);
+ pstrcpy(bs->backing_format, sizeof(bs->backing_format),
+ bs->backing_hd->drv->format_name);
return 0;
}
Looks like you do more than the stated subject in this patch: you also
add a blocker! Separate patch, please.
"Block all" followed by "unblock specific" is a bit awkward. A function
taking a bit set would let you do the masking on bits rather than an
array of lists. Suggestion, not a request, and it can be done in a
followup patch.
> @@ -1164,9 +1188,11 @@ int bdrv_open(BlockDriverState *bs, const char
> *filename, QDict *options,
> /* If there is a backing file, use it */
> if ((flags & BDRV_O_NO_BACKING) == 0) {
> QDict *backing_options;
> -
> qdict_extract_subqdict(options, &backing_options, "backing.");
> - ret = bdrv_open_backing_file(bs, backing_options, &local_err);
> + ret = bdrv_open_backing_file(bs, qdict_get_try_str(options,
> "backing"),
> + backing_options, &local_err);
> +
> + qdict_del(options, "backing");
Why do you have to delete "backing"?
> if (ret < 0) {
> goto close_and_fail;
> }
> @@ -1459,9 +1485,14 @@ void bdrv_close(BlockDriverState *bs)
>
> if (bs->drv) {
> if (bs->backing_hd) {
> + assert(bs->backing_blocker);
> + bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
> bdrv_unref(bs->backing_hd);
> bs->backing_hd = NULL;
> }
> + if (bs->backing_blocker) {
> + error_free(bs->backing_blocker);
> + }
Can bs->backing_blocker legitimately be non-null when bs->backing_hd is
null?
> bs->drv->bdrv_close(bs);
> g_free(bs->opaque);
> #ifdef _WIN32
> diff --git a/block/mirror.c b/block/mirror.c
> index 6dc27ad..b1596d3 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -511,7 +511,7 @@ static void mirror_complete(BlockJob *job, Error **errp)
> Error *local_err = NULL;
> int ret;
>
> - ret = bdrv_open_backing_file(s->target, NULL, &local_err);
> + ret = bdrv_open_backing_file(s->target, NULL, NULL, &local_err);
> if (ret < 0) {
> char backing_filename[PATH_MAX];
> bdrv_get_full_backing_filename(s->target, backing_filename,
> diff --git a/include/block/block.h b/include/block/block.h
> index 18397e0..24d06f9 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -185,7 +185,8 @@ 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,
> QDict *options, int flags, Error **errp);
> -int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error
> **errp);
> +int bdrv_open_backing_file(BlockDriverState *bs, const char *backing_bs,
> + QDict *options, Error **errp);
> int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> int flags, BlockDriver *drv, Error **errp);
> BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 458acd6..83dcf7d 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -342,6 +342,9 @@ struct BlockDriverState {
> BlockJob *job;
>
> QDict *options;
> +
> + /* For backing reference, block the operations of named backing device */
> + Error *backing_blocker;
> };
>
> int get_tmp_filename(char *filename, int size);
I find the comment confusing :)
What about:
/* The error object in use for blocking operations on backing_hd */
- [Qemu-devel] [PATCH v7 00/10] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD, Fam Zheng, 2013/12/12
- [Qemu-devel] [PATCH v7 01/10] qapi: Add BlockOperationType enum, Fam Zheng, 2013/12/12
- [Qemu-devel] [PATCH v7 02/10] block: Introduce op_blockers to BlockDriverState, Fam Zheng, 2013/12/12
- [Qemu-devel] [PATCH v7 03/10] block: Parse "backing" option to reference existing BDS, Fam Zheng, 2013/12/12
- Re: [Qemu-devel] [PATCH v7 03/10] block: Parse "backing" option to reference existing BDS,
Markus Armbruster <=
- [Qemu-devel] [PATCH v7 04/10] block: support dropping active in bdrv_drop_intermediate, Fam Zheng, 2013/12/12
- [Qemu-devel] [PATCH v7 05/10] stream: Use bdrv_drop_intermediate and drop close_unused_images, Fam Zheng, 2013/12/12
- [Qemu-devel] [PATCH v7 06/10] block: Replace in_use with operation blocker, Fam Zheng, 2013/12/12
- [Qemu-devel] [PATCH v7 08/10] block: Add checks of blocker in block operations, Fam Zheng, 2013/12/12