* [Qemu-devel] [PATCH v2 0/8] block: Integrate bdrv_file_open() into bdrv_open()
@ 2014-02-08 17:39 Max Reitz
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 1/8] block: Change BDS parameter of bdrv_open() to ** Max Reitz
` (8 more replies)
0 siblings, 9 replies; 25+ messages in thread
From: Max Reitz @ 2014-02-08 17:39 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Benoît Canet, Stefan Hajnoczi,
Max Reitz
bdrv_file_open() is now nearly a subset of bdrv_open(), except for the
fact that bdrv_file_open() is for protocols and bdrv_open() for block
drivers. It is possible to use bdrv_file_open() with a block driver, but
in that case that block driver must be explicitly specified.
Due to these great similarities, bdrv_file_open() can be integrated and
made a special case of bdrv_open(). If the flag BDRV_O_PROTOCOL is
specified, bdrv_open() will now do what bdrv_file_open() used to do:
Auto-detecting a protocol instead of a block driver.
This series implements this and changes all calls to bdrv_file_open() to
bdrv_open() calls with BDRV_O_PROTOCOL specified.
Note that this flag cannot be discerned automatically since it is
impossible for bdrv_open() to know by itself whether a given file should
be opened with or without the format layer involved: Both are valid
alternatives. Therefore, it still has to be specified by the user.
v2:
- patch 1:
- added appropriate assertions before every bdrv_open() and
bdrv_open_image() call where the BDS should be NULL (i.e., where a
new BDS should be created) and it is not obvious
- don't initialize BDS pointers to NULL if that is necessary for a
bdrv_open(); rather, set them to NULL directly before calling
bdrv_open() in order to make the expected behavior of bdrv_open()
(create a new BDS) more obvious
- make bdrv_open_image() behave the same way as bdrv_open() in regard
to BDS creation/reuse (completed in patch 8)
- keep "options == NULL" check in bdrv_open() together with the block
for setting "bs->options"
- patch 2:
- due to options now possibly being NULL in the reference handling
block of bdrv_open(), the condition for "options_non_empty" has to
be adjusted
- contextual changes due to patch 1
- patch 3:
- adjust comment for BDRV_O_PROTOCOL
- contextual changes due to the previous patches
- patch 4 (previously 5):
- contextual changes due to the previous patches
- patch 5 (previously 6):
- bdrv_close() is unnecessary if the BDS could not be opened
- patch 6:
- melded together from previous patches 4, 7 and 8
- patch 7 (previously 9):
- make the "options" parameter of bdrv_file_open() an indirect
pointer; if bdrv_file_open() takes the reference, it will be set to
NULL; otherwise, the caller is responsible for freeing the QDict
- patch 8 (previously 10):
- make bdrv_open_image() behave the same way as bdrv_open() in regard
to BDS creation/reuse
- contextual changes due to the previous patches
Max Reitz (8):
block: Change BDS parameter of bdrv_open() to **
block: Add reference parameter to bdrv_open()
block: Make bdrv_file_open() static
block: Reuse reference handling from bdrv_open()
block: Remove bdrv_new() from bdrv_file_open()
block: Handle bs->options in bdrv_open() only
block: Reuse success path from bdrv_open()
block: Remove bdrv_open_image()'s force_raw option
block.c | 229 +++++++++++++++++++++++++-------------------------
block/blkdebug.c | 3 +-
block/blkverify.c | 6 +-
block/cow.c | 6 +-
block/qcow.c | 6 +-
block/qcow2.c | 19 +++--
block/qed.c | 5 +-
block/sheepdog.c | 8 +-
block/vhdx.c | 5 +-
block/vmdk.c | 17 ++--
block/vvfat.c | 6 +-
blockdev.c | 22 +++--
hw/block/xen_disk.c | 4 +-
include/block/block.h | 13 +--
qemu-img.c | 12 ++-
qemu-io.c | 8 +-
qemu-nbd.c | 2 +-
17 files changed, 194 insertions(+), 177 deletions(-)
--
1.8.5.4
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 1/8] block: Change BDS parameter of bdrv_open() to **
2014-02-08 17:39 [Qemu-devel] [PATCH v2 0/8] block: Integrate bdrv_file_open() into bdrv_open() Max Reitz
@ 2014-02-08 17:39 ` Max Reitz
2014-02-10 12:42 ` Kevin Wolf
2014-02-10 13:17 ` Benoît Canet
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 2/8] block: Add reference parameter to bdrv_open() Max Reitz
` (7 subsequent siblings)
8 siblings, 2 replies; 25+ messages in thread
From: Max Reitz @ 2014-02-08 17:39 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Benoît Canet, Stefan Hajnoczi,
Max Reitz
Make bdrv_open() take a pointer to a BDS pointer, similarly to
bdrv_file_open(). If a pointer to a NULL pointer is given, bdrv_open()
will create a new BDS with an empty name; if the BDS pointer is not
NULL, that existing BDS will be reused (in the same way as bdrv_open()
already did).
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block.c | 64 +++++++++++++++++++++++++++++++--------------------
block/blkdebug.c | 1 +
block/blkverify.c | 2 ++
block/qcow2.c | 14 +++++++----
block/vmdk.c | 5 ++--
block/vvfat.c | 6 ++---
blockdev.c | 20 ++++++++--------
hw/block/xen_disk.c | 2 +-
include/block/block.h | 2 +-
qemu-img.c | 10 ++++----
qemu-io.c | 2 +-
qemu-nbd.c | 2 +-
12 files changed, 72 insertions(+), 58 deletions(-)
diff --git a/block.c b/block.c
index 636aa11..40a585a 100644
--- a/block.c
+++ b/block.c
@@ -1040,7 +1040,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
}
if (!drv->bdrv_file_open) {
- ret = bdrv_open(bs, filename, options, flags, drv, &local_err);
+ ret = bdrv_open(&bs, filename, options, flags, drv, &local_err);
options = NULL;
} else {
ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
@@ -1109,8 +1109,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
sizeof(backing_filename));
}
- bs->backing_hd = bdrv_new("");
-
if (bs->backing_format[0] != '\0') {
back_drv = bdrv_find_format(bs->backing_format);
}
@@ -1119,11 +1117,11 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT |
BDRV_O_COPY_ON_READ);
- ret = bdrv_open(bs->backing_hd,
+ assert(bs->backing_hd == NULL);
+ 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",
@@ -1160,6 +1158,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
* BlockdevRef.
*
* The BlockdevRef will be removed from the options QDict.
+ *
+ * As with bdrv_open(), if *pbs is NULL, a new BDS will be created with a
+ * pointer to it stored there. If it is not NULL, the referenced BDS will
+ * be reused.
*/
int bdrv_open_image(BlockDriverState **pbs, const char *filename,
QDict *options, const char *bdref_key, int flags,
@@ -1190,8 +1192,6 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
/* If a filename is given and the block driver should be detected
automatically (instead of using none), use bdrv_open() in order to do
that auto-detection. */
- BlockDriverState *bs;
-
if (reference) {
error_setg(errp, "Cannot reference an existing block device while "
"giving a filename");
@@ -1199,13 +1199,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
goto done;
}
- bs = bdrv_new("");
- ret = bdrv_open(bs, filename, image_options, flags, NULL, errp);
- if (ret < 0) {
- bdrv_unref(bs);
- } else {
- *pbs = bs;
- }
+ ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp);
} else {
ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
errp);
@@ -1223,22 +1217,32 @@ done:
* empty set of options. The reference to the QDict belongs to the block layer
* after the call (even on failure), so if the caller intends to reuse the
* dictionary, it needs to use QINCREF() before calling bdrv_open.
+ *
+ * If *pbs is NULL, a new BDS will be created with a pointer to it stored there.
+ * If it is not NULL, the referenced BDS will be reused.
*/
-int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
+int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
int flags, BlockDriver *drv, Error **errp)
{
int ret;
/* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
char tmp_filename[PATH_MAX + 1];
- BlockDriverState *file = NULL;
+ BlockDriverState *file = NULL, *bs;
const char *drvname;
Error *local_err = NULL;
+ assert(pbs);
+
+ if (*pbs) {
+ bs = *pbs;
+ } else {
+ bs = bdrv_new("");
+ }
+
/* NULL means an empty set of options */
if (options == NULL) {
options = qdict_new();
}
-
bs->options = options;
options = qdict_clone_shallow(options);
@@ -1254,12 +1258,11 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
instead of opening 'filename' directly */
/* Get the required size from the image */
- bs1 = bdrv_new("");
QINCREF(options);
- ret = bdrv_open(bs1, filename, options, BDRV_O_NO_BACKING,
+ bs1 = NULL;
+ ret = bdrv_open(&bs1, filename, options, BDRV_O_NO_BACKING,
drv, &local_err);
if (ret < 0) {
- bdrv_unref(bs1);
goto fail;
}
total_size = bdrv_getlength(bs1) & BDRV_SECTOR_MASK;
@@ -1316,6 +1319,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
flags |= BDRV_O_ALLOW_RDWR;
}
+ assert(file == NULL);
ret = bdrv_open_image(&file, filename, options, "file",
bdrv_open_flags(bs, flags | BDRV_O_UNMAP), true, true,
&local_err);
@@ -1387,6 +1391,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
bdrv_dev_change_media_cb(bs, true);
}
+ *pbs = bs;
return 0;
unlink_and_fail:
@@ -1400,13 +1405,24 @@ fail:
QDECREF(bs->options);
QDECREF(options);
bs->options = NULL;
+ if (!*pbs) {
+ /* If *pbs is NULL, a new BDS has been created in this function and
+ needs to be freed now. Otherwise, it does not need to be closed,
+ since it has not really been opened yet. */
+ bdrv_unref(bs);
+ }
if (error_is_set(&local_err)) {
error_propagate(errp, local_err);
}
return ret;
close_and_fail:
- bdrv_close(bs);
+ /* See fail path, but now the BDS has to be always closed */
+ if (*pbs) {
+ bdrv_close(bs);
+ } else {
+ bdrv_unref(bs);
+ }
QDECREF(options);
if (error_is_set(&local_err)) {
error_propagate(errp, local_err);
@@ -5288,9 +5304,8 @@ void bdrv_img_create(const char *filename, const char *fmt,
back_flags =
flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
- bs = bdrv_new("");
-
- ret = bdrv_open(bs, backing_file->value.s, NULL, back_flags,
+ bs = NULL;
+ ret = bdrv_open(&bs, backing_file->value.s, NULL, back_flags,
backing_drv, &local_err);
if (ret < 0) {
error_setg_errno(errp, -ret, "Could not open '%s': %s",
@@ -5298,7 +5313,6 @@ void bdrv_img_create(const char *filename, const char *fmt,
error_get_pretty(local_err));
error_free(local_err);
local_err = NULL;
- bdrv_unref(bs);
goto out;
}
bdrv_get_geometry(bs, &size);
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 8eb0db0..053fa4c 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -410,6 +410,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
s->state = 1;
/* Open the backing file */
+ assert(bs->file == NULL);
ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-image"), options, "image",
flags, true, false, &local_err);
if (ret < 0) {
diff --git a/block/blkverify.c b/block/blkverify.c
index cfcbcf4..86585e7 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -135,6 +135,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
}
/* Open the raw file */
+ assert(bs->file == NULL);
ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-raw"), options,
"raw", flags, true, false, &local_err);
if (ret < 0) {
@@ -143,6 +144,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
}
/* Open the test file */
+ assert(s->test_file == NULL);
ret = bdrv_open_image(&s->test_file, qemu_opt_get(opts, "x-image"), options,
"test", flags, false, false, &local_err);
if (ret < 0) {
diff --git a/block/qcow2.c b/block/qcow2.c
index 0b4335c..c8e8ba7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1543,7 +1543,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
goto out;
}
- bdrv_close(bs);
+ bdrv_unref(bs);
+ bs = NULL;
/*
* And now open the image and make it consistent first (i.e. increase the
@@ -1552,7 +1553,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
*/
BlockDriver* drv = bdrv_find_format("qcow2");
assert(drv != NULL);
- ret = bdrv_open(bs, filename, NULL,
+ ret = bdrv_open(&bs, filename, NULL,
BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, drv, &local_err);
if (ret < 0) {
error_propagate(errp, local_err);
@@ -1599,10 +1600,11 @@ static int qcow2_create2(const char *filename, int64_t total_size,
}
}
- bdrv_close(bs);
+ bdrv_unref(bs);
+ bs = NULL;
/* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning */
- ret = bdrv_open(bs, filename, NULL,
+ ret = bdrv_open(&bs, filename, NULL,
BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING,
drv, &local_err);
if (error_is_set(&local_err)) {
@@ -1612,7 +1614,9 @@ static int qcow2_create2(const char *filename, int64_t total_size,
ret = 0;
out:
- bdrv_unref(bs);
+ if (bs) {
+ bdrv_unref(bs);
+ }
return ret;
}
diff --git a/block/vmdk.c b/block/vmdk.c
index e809e2e..52cd12a 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1755,10 +1755,9 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
goto exit;
}
if (backing_file) {
- BlockDriverState *bs = bdrv_new("");
- ret = bdrv_open(bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp);
+ BlockDriverState *bs = NULL;
+ ret = bdrv_open(&bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp);
if (ret != 0) {
- bdrv_unref(bs);
goto exit;
}
if (strcmp(bs->drv->format_name, "vmdk")) {
diff --git a/block/vvfat.c b/block/vvfat.c
index 664941c..ae7bc6f 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2936,15 +2936,13 @@ static int enable_write_target(BDRVVVFATState *s)
goto err;
}
- s->qcow = bdrv_new("");
-
- ret = bdrv_open(s->qcow, s->qcow_filename, NULL,
+ s->qcow = NULL;
+ ret = bdrv_open(&s->qcow, s->qcow_filename, NULL,
BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow,
&local_err);
if (ret < 0) {
qerror_report_err(local_err);
error_free(local_err);
- bdrv_unref(s->qcow);
goto err;
}
diff --git a/blockdev.c b/blockdev.c
index 36ceece..6dd351c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -510,7 +510,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
QINCREF(bs_opts);
- ret = bdrv_open(dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
+ ret = bdrv_open(&dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
if (ret < 0) {
error_setg(errp, "could not open disk image %s: %s",
@@ -1301,12 +1301,12 @@ static void external_snapshot_prepare(BlkTransactionState *common,
qstring_from_str(snapshot_node_name));
}
- /* We will manually add the backing_hd field to the bs later */
- state->new_bs = bdrv_new("");
/* TODO Inherit bs->options or only take explicit options with an
* extended QMP command? */
- ret = bdrv_open(state->new_bs, new_image_file, options,
+ assert(state->new_bs == NULL);
+ ret = bdrv_open(&state->new_bs, new_image_file, options,
flags | BDRV_O_NO_BACKING, drv, &local_err);
+ /* We will manually add the backing_hd field to the bs later */
if (ret != 0) {
error_propagate(errp, local_err);
}
@@ -1555,7 +1555,7 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
Error *local_err = NULL;
int ret;
- ret = bdrv_open(bs, filename, NULL, bdrv_flags, drv, &local_err);
+ ret = bdrv_open(&bs, filename, NULL, bdrv_flags, drv, &local_err);
if (ret < 0) {
error_propagate(errp, local_err);
return;
@@ -1991,10 +1991,9 @@ void qmp_drive_backup(const char *device, const char *target,
return;
}
- target_bs = bdrv_new("");
- ret = bdrv_open(target_bs, target, NULL, flags, drv, &local_err);
+ target_bs = NULL;
+ ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err);
if (ret < 0) {
- bdrv_unref(target_bs);
error_propagate(errp, local_err);
return;
}
@@ -2135,11 +2134,10 @@ void qmp_drive_mirror(const char *device, const char *target,
/* Mirroring takes care of copy-on-write using the source's backing
* file.
*/
- target_bs = bdrv_new("");
- ret = bdrv_open(target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
+ target_bs = NULL;
+ ret = bdrv_open(&target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
&local_err);
if (ret < 0) {
- bdrv_unref(target_bs);
error_propagate(errp, local_err);
return;
}
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 098f6c6..14e6d0b 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -813,7 +813,7 @@ static int blk_connect(struct XenDevice *xendev)
Error *local_err = NULL;
BlockDriver *drv = bdrv_find_whitelisted_format(blkdev->fileproto,
readonly);
- if (bdrv_open(blkdev->bs,
+ if (bdrv_open(&blkdev->bs,
blkdev->filename, NULL, qflags, drv, &local_err) != 0)
{
xen_be_printf(&blkdev->xendev, 0, "error: %s\n",
diff --git a/include/block/block.h b/include/block/block.h
index 963a61f..980869d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -190,7 +190,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
QDict *options, const char *bdref_key, int flags,
bool force_raw, bool allow_none, Error **errp);
int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
-int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
+int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
int flags, BlockDriver *drv, Error **errp);
BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
BlockDriverState *bs, int flags);
diff --git a/qemu-img.c b/qemu-img.c
index c989850..897aa56 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -289,7 +289,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
drv = NULL;
}
- ret = bdrv_open(bs, filename, NULL, flags, drv, &local_err);
+ ret = bdrv_open(&bs, filename, NULL, flags, drv, &local_err);
if (ret < 0) {
error_report("Could not open '%s': %s", filename,
error_get_pretty(local_err));
@@ -310,9 +310,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
}
return bs;
fail:
- if (bs) {
- bdrv_unref(bs);
- }
+ bdrv_unref(bs);
return NULL;
}
@@ -2314,7 +2312,7 @@ static int img_rebase(int argc, char **argv)
bs_old_backing = bdrv_new("old_backing");
bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
- ret = bdrv_open(bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
+ ret = bdrv_open(&bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
old_backing_drv, &local_err);
if (ret) {
error_report("Could not open old backing file '%s': %s",
@@ -2324,7 +2322,7 @@ static int img_rebase(int argc, char **argv)
}
if (out_baseimg[0]) {
bs_new_backing = bdrv_new("new_backing");
- ret = bdrv_open(bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
+ ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
new_backing_drv, &local_err);
if (ret) {
error_report("Could not open new backing file '%s': %s",
diff --git a/qemu-io.c b/qemu-io.c
index 7f459d8..8da8f6e 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -68,7 +68,7 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
} else {
qemuio_bs = bdrv_new("hda");
- if (bdrv_open(qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
+ if (bdrv_open(&qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
error_get_pretty(local_err));
error_free(local_err);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 136e8c9..0cf123c 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -597,7 +597,7 @@ int main(int argc, char **argv)
bs = bdrv_new("hda");
srcpath = argv[optind];
- ret = bdrv_open(bs, srcpath, NULL, flags, drv, &local_err);
+ ret = bdrv_open(&bs, srcpath, NULL, flags, drv, &local_err);
if (ret < 0) {
errno = -ret;
err(EXIT_FAILURE, "Failed to bdrv_open '%s': %s", argv[optind],
--
1.8.5.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 2/8] block: Add reference parameter to bdrv_open()
2014-02-08 17:39 [Qemu-devel] [PATCH v2 0/8] block: Integrate bdrv_file_open() into bdrv_open() Max Reitz
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 1/8] block: Change BDS parameter of bdrv_open() to ** Max Reitz
@ 2014-02-08 17:39 ` Max Reitz
2014-02-10 13:30 ` Benoît Canet
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 3/8] block: Make bdrv_file_open() static Max Reitz
` (6 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Max Reitz @ 2014-02-08 17:39 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Benoît Canet, Stefan Hajnoczi,
Max Reitz
Allow bdrv_open() to handle references to existing block devices just as
bdrv_file_open() is already capable of.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block.c | 41 ++++++++++++++++++++++++++++++++++-------
block/qcow2.c | 4 ++--
block/vmdk.c | 3 ++-
block/vvfat.c | 2 +-
blockdev.c | 12 ++++++------
hw/block/xen_disk.c | 4 ++--
include/block/block.h | 5 +++--
qemu-img.c | 8 ++++----
qemu-io.c | 4 +++-
qemu-nbd.c | 2 +-
10 files changed, 58 insertions(+), 27 deletions(-)
diff --git a/block.c b/block.c
index 40a585a..3a32c37 100644
--- a/block.c
+++ b/block.c
@@ -1040,7 +1040,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
}
if (!drv->bdrv_file_open) {
- ret = bdrv_open(&bs, filename, options, flags, drv, &local_err);
+ ret = bdrv_open(&bs, filename, NULL, options, flags, drv, &local_err);
options = NULL;
} else {
ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
@@ -1119,7 +1119,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
assert(bs->backing_hd == NULL);
ret = bdrv_open(&bs->backing_hd,
- *backing_filename ? backing_filename : NULL, options,
+ *backing_filename ? backing_filename : NULL, NULL, options,
back_flags, back_drv, &local_err);
if (ret < 0) {
bs->backing_hd = NULL;
@@ -1199,7 +1199,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
goto done;
}
- ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp);
+ ret = bdrv_open(pbs, filename, NULL, image_options, flags, NULL, errp);
} else {
ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
errp);
@@ -1221,8 +1221,9 @@ done:
* If *pbs is NULL, a new BDS will be created with a pointer to it stored there.
* If it is not NULL, the referenced BDS will be reused.
*/
-int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
- int flags, BlockDriver *drv, Error **errp)
+int bdrv_open(BlockDriverState **pbs, const char *filename,
+ const char *reference, QDict *options, int flags,
+ BlockDriver *drv, Error **errp)
{
int ret;
/* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
@@ -1233,6 +1234,32 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
assert(pbs);
+ if (reference) {
+ bool options_non_empty = options ? qdict_size(options) : false;
+ QDECREF(options);
+
+ if (*pbs) {
+ error_setg(errp, "Cannot reuse an existing BDS when referencing "
+ "another block device");
+ return -EINVAL;
+ }
+
+ if (filename || options_non_empty) {
+ error_setg(errp, "Cannot reference an existing block device with "
+ "additional options or a new filename");
+ return -EINVAL;
+ }
+
+ bs = bdrv_find(reference);
+ if (!bs) {
+ error_setg(errp, "Cannot find block device '%s'", reference);
+ return -ENODEV;
+ }
+ bdrv_ref(bs);
+ *pbs = bs;
+ return 0;
+ }
+
if (*pbs) {
bs = *pbs;
} else {
@@ -1260,7 +1287,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
/* Get the required size from the image */
QINCREF(options);
bs1 = NULL;
- ret = bdrv_open(&bs1, filename, options, BDRV_O_NO_BACKING,
+ ret = bdrv_open(&bs1, filename, NULL, options, BDRV_O_NO_BACKING,
drv, &local_err);
if (ret < 0) {
goto fail;
@@ -5305,7 +5332,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
bs = NULL;
- ret = bdrv_open(&bs, backing_file->value.s, NULL, back_flags,
+ ret = bdrv_open(&bs, backing_file->value.s, NULL, NULL, back_flags,
backing_drv, &local_err);
if (ret < 0) {
error_setg_errno(errp, -ret, "Could not open '%s': %s",
diff --git a/block/qcow2.c b/block/qcow2.c
index c8e8ba7..6996276 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1553,7 +1553,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
*/
BlockDriver* drv = bdrv_find_format("qcow2");
assert(drv != NULL);
- ret = bdrv_open(&bs, filename, NULL,
+ ret = bdrv_open(&bs, filename, NULL, NULL,
BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, drv, &local_err);
if (ret < 0) {
error_propagate(errp, local_err);
@@ -1604,7 +1604,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
bs = NULL;
/* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning */
- ret = bdrv_open(&bs, filename, NULL,
+ ret = bdrv_open(&bs, filename, NULL, NULL,
BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING,
drv, &local_err);
if (error_is_set(&local_err)) {
diff --git a/block/vmdk.c b/block/vmdk.c
index 52cd12a..e007793 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1756,7 +1756,8 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
}
if (backing_file) {
BlockDriverState *bs = NULL;
- ret = bdrv_open(&bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp);
+ ret = bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_NO_BACKING, NULL,
+ errp);
if (ret != 0) {
goto exit;
}
diff --git a/block/vvfat.c b/block/vvfat.c
index ae7bc6f..d7a830e 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2937,7 +2937,7 @@ static int enable_write_target(BDRVVVFATState *s)
}
s->qcow = NULL;
- ret = bdrv_open(&s->qcow, s->qcow_filename, NULL,
+ ret = bdrv_open(&s->qcow, s->qcow_filename, NULL, NULL,
BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow,
&local_err);
if (ret < 0) {
diff --git a/blockdev.c b/blockdev.c
index 6dd351c..7b7e349 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -510,7 +510,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
QINCREF(bs_opts);
- ret = bdrv_open(&dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
+ ret = bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, drv, &error);
if (ret < 0) {
error_setg(errp, "could not open disk image %s: %s",
@@ -1304,7 +1304,7 @@ static void external_snapshot_prepare(BlkTransactionState *common,
/* TODO Inherit bs->options or only take explicit options with an
* extended QMP command? */
assert(state->new_bs == NULL);
- ret = bdrv_open(&state->new_bs, new_image_file, options,
+ ret = bdrv_open(&state->new_bs, new_image_file, NULL, options,
flags | BDRV_O_NO_BACKING, drv, &local_err);
/* We will manually add the backing_hd field to the bs later */
if (ret != 0) {
@@ -1555,7 +1555,7 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
Error *local_err = NULL;
int ret;
- ret = bdrv_open(&bs, filename, NULL, bdrv_flags, drv, &local_err);
+ ret = bdrv_open(&bs, filename, NULL, NULL, bdrv_flags, drv, &local_err);
if (ret < 0) {
error_propagate(errp, local_err);
return;
@@ -1992,7 +1992,7 @@ void qmp_drive_backup(const char *device, const char *target,
}
target_bs = NULL;
- ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err);
+ ret = bdrv_open(&target_bs, target, NULL, NULL, flags, drv, &local_err);
if (ret < 0) {
error_propagate(errp, local_err);
return;
@@ -2135,8 +2135,8 @@ void qmp_drive_mirror(const char *device, const char *target,
* file.
*/
target_bs = NULL;
- ret = bdrv_open(&target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
- &local_err);
+ ret = bdrv_open(&target_bs, target, NULL, NULL, flags | BDRV_O_NO_BACKING,
+ drv, &local_err);
if (ret < 0) {
error_propagate(errp, local_err);
return;
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 14e6d0b..61e6ff3 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -813,8 +813,8 @@ static int blk_connect(struct XenDevice *xendev)
Error *local_err = NULL;
BlockDriver *drv = bdrv_find_whitelisted_format(blkdev->fileproto,
readonly);
- if (bdrv_open(&blkdev->bs,
- blkdev->filename, NULL, qflags, drv, &local_err) != 0)
+ if (bdrv_open(&blkdev->bs, blkdev->filename, NULL, NULL, qflags,
+ drv, &local_err) != 0)
{
xen_be_printf(&blkdev->xendev, 0, "error: %s\n",
error_get_pretty(local_err));
diff --git a/include/block/block.h b/include/block/block.h
index 980869d..a421041 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -190,8 +190,9 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
QDict *options, const char *bdref_key, int flags,
bool force_raw, bool allow_none, Error **errp);
int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
-int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
- int flags, BlockDriver *drv, Error **errp);
+int bdrv_open(BlockDriverState **pbs, const char *filename,
+ const char *reference, QDict *options, int flags,
+ BlockDriver *drv, Error **errp);
BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
BlockDriverState *bs, int flags);
int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
diff --git a/qemu-img.c b/qemu-img.c
index 897aa56..b834d52 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -289,7 +289,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
drv = NULL;
}
- ret = bdrv_open(&bs, filename, NULL, flags, drv, &local_err);
+ ret = bdrv_open(&bs, filename, NULL, NULL, flags, drv, &local_err);
if (ret < 0) {
error_report("Could not open '%s': %s", filename,
error_get_pretty(local_err));
@@ -2312,7 +2312,7 @@ static int img_rebase(int argc, char **argv)
bs_old_backing = bdrv_new("old_backing");
bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
- ret = bdrv_open(&bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
+ ret = bdrv_open(&bs_old_backing, backing_name, NULL, NULL, BDRV_O_FLAGS,
old_backing_drv, &local_err);
if (ret) {
error_report("Could not open old backing file '%s': %s",
@@ -2322,8 +2322,8 @@ static int img_rebase(int argc, char **argv)
}
if (out_baseimg[0]) {
bs_new_backing = bdrv_new("new_backing");
- ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
- new_backing_drv, &local_err);
+ ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, NULL,
+ BDRV_O_FLAGS, new_backing_drv, &local_err);
if (ret) {
error_report("Could not open new backing file '%s': %s",
out_baseimg, error_get_pretty(local_err));
diff --git a/qemu-io.c b/qemu-io.c
index 8da8f6e..61d54c0 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -68,7 +68,9 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
} else {
qemuio_bs = bdrv_new("hda");
- if (bdrv_open(&qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
+ if (bdrv_open(&qemuio_bs, name, NULL, opts, flags, NULL, &local_err)
+ < 0)
+ {
fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
error_get_pretty(local_err));
error_free(local_err);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 0cf123c..711162c 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -597,7 +597,7 @@ int main(int argc, char **argv)
bs = bdrv_new("hda");
srcpath = argv[optind];
- ret = bdrv_open(&bs, srcpath, NULL, flags, drv, &local_err);
+ ret = bdrv_open(&bs, srcpath, NULL, NULL, flags, drv, &local_err);
if (ret < 0) {
errno = -ret;
err(EXIT_FAILURE, "Failed to bdrv_open '%s': %s", argv[optind],
--
1.8.5.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 3/8] block: Make bdrv_file_open() static
2014-02-08 17:39 [Qemu-devel] [PATCH v2 0/8] block: Integrate bdrv_file_open() into bdrv_open() Max Reitz
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 1/8] block: Change BDS parameter of bdrv_open() to ** Max Reitz
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 2/8] block: Add reference parameter to bdrv_open() Max Reitz
@ 2014-02-08 17:39 ` Max Reitz
2014-02-10 13:40 ` Benoît Canet
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 4/8] block: Reuse reference handling from bdrv_open() Max Reitz
` (5 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Max Reitz @ 2014-02-08 17:39 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Benoît Canet, Stefan Hajnoczi,
Max Reitz
Add the bdrv_open() option BDRV_O_PROTOCOL which results in passing the
call to bdrv_file_open(). Additionally, make bdrv_file_open() static and
therefore bdrv_open() the only way to call it.
Consequently, all existing calls to bdrv_file_open() have to be adjusted
to use bdrv_open() with the BDRV_O_PROTOCOL flag instead.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block.c | 17 ++++++++++++-----
block/cow.c | 6 +++---
block/qcow.c | 6 +++---
block/qcow2.c | 5 +++--
block/qed.c | 5 +++--
block/sheepdog.c | 8 +++++---
block/vhdx.c | 5 +++--
block/vmdk.c | 11 +++++++----
include/block/block.h | 6 +++---
qemu-io.c | 4 +++-
10 files changed, 45 insertions(+), 28 deletions(-)
diff --git a/block.c b/block.c
index 3a32c37..432d659 100644
--- a/block.c
+++ b/block.c
@@ -953,9 +953,9 @@ free_and_fail:
* after the call (even on failure), so if the caller intends to reuse the
* dictionary, it needs to use QINCREF() before calling bdrv_file_open.
*/
-int bdrv_file_open(BlockDriverState **pbs, const char *filename,
- const char *reference, QDict *options, int flags,
- Error **errp)
+static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
+ const char *reference, QDict *options, int flags,
+ Error **errp)
{
BlockDriverState *bs = NULL;
BlockDriver *drv;
@@ -1201,8 +1201,9 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
ret = bdrv_open(pbs, filename, NULL, image_options, flags, NULL, errp);
} else {
- ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
- errp);
+ *pbs = NULL;
+ ret = bdrv_open(pbs, filename, reference, image_options,
+ flags | BDRV_O_PROTOCOL, NULL, errp);
}
done:
@@ -1234,6 +1235,12 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
assert(pbs);
+ if (flags & BDRV_O_PROTOCOL) {
+ assert(!drv);
+ return bdrv_file_open(pbs, filename, reference, options,
+ flags & ~BDRV_O_PROTOCOL, errp);
+ }
+
if (reference) {
bool options_non_empty = options ? qdict_size(options) : false;
QDECREF(options);
diff --git a/block/cow.c b/block/cow.c
index 7fc0b12..f0748a2 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -332,7 +332,7 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
const char *image_filename = NULL;
Error *local_err = NULL;
int ret;
- BlockDriverState *cow_bs;
+ BlockDriverState *cow_bs = NULL;
/* Read out options */
while (options && options->name) {
@@ -351,8 +351,8 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
return ret;
}
- ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR,
- &local_err);
+ ret = bdrv_open(&cow_bs, filename, NULL, NULL,
+ BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err);
if (ret < 0) {
qerror_report_err(local_err);
error_free(local_err);
diff --git a/block/qcow.c b/block/qcow.c
index 948b0c5..4881d84 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -670,7 +670,7 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options,
int flags = 0;
Error *local_err = NULL;
int ret;
- BlockDriverState *qcow_bs;
+ BlockDriverState *qcow_bs = NULL;
/* Read out options */
while (options && options->name) {
@@ -691,8 +691,8 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options,
return ret;
}
- ret = bdrv_file_open(&qcow_bs, filename, NULL, NULL, BDRV_O_RDWR,
- &local_err);
+ ret = bdrv_open(&qcow_bs, filename, NULL, NULL,
+ BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err);
if (ret < 0) {
qerror_report_err(local_err);
error_free(local_err);
diff --git a/block/qcow2.c b/block/qcow2.c
index 6996276..bd72172 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1481,7 +1481,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
* 2 GB for 64k clusters, and we don't want to have a 2 GB initial file
* size for any qcow2 image.
*/
- BlockDriverState* bs;
+ BlockDriverState *bs = NULL;
QCowHeader *header;
uint8_t* refcount_table;
Error *local_err = NULL;
@@ -1493,7 +1493,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
return ret;
}
- ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
+ ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
+ NULL, &local_err);
if (ret < 0) {
error_propagate(errp, local_err);
return ret;
diff --git a/block/qed.c b/block/qed.c
index b9ca7ac..33fbce1 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -571,8 +571,9 @@ static int qed_create(const char *filename, uint32_t cluster_size,
return ret;
}
- ret = bdrv_file_open(&bs, filename, NULL, NULL,
- BDRV_O_RDWR | BDRV_O_CACHE_WB, &local_err);
+ ret = bdrv_open(&bs, filename, NULL, NULL,
+ BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_PROTOCOL, NULL,
+ &local_err);
if (ret < 0) {
qerror_report_err(local_err);
error_free(local_err);
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 672b9c9..6f2ec57 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1534,7 +1534,8 @@ static int sd_prealloc(const char *filename)
Error *local_err = NULL;
int ret;
- ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
+ ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
+ NULL, &local_err);
if (ret < 0) {
qerror_report_err(local_err);
error_free(local_err);
@@ -1683,7 +1684,7 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
}
if (backing_file) {
- BlockDriverState *bs;
+ BlockDriverState *bs = NULL;
BDRVSheepdogState *base;
BlockDriver *drv;
@@ -1695,7 +1696,8 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
goto out;
}
- ret = bdrv_file_open(&bs, backing_file, NULL, NULL, 0, &local_err);
+ ret = bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_PROTOCOL, NULL,
+ &local_err);
if (ret < 0) {
qerror_report_err(local_err);
error_free(local_err);
diff --git a/block/vhdx.c b/block/vhdx.c
index 55689cf..38d949c 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1724,7 +1724,7 @@ static int vhdx_create(const char *filename, QEMUOptionParameter *options,
gunichar2 *creator = NULL;
glong creator_items;
- BlockDriverState *bs;
+ BlockDriverState *bs = NULL;
const char *type = NULL;
VHDXImageType image_type;
Error *local_err = NULL;
@@ -1797,7 +1797,8 @@ static int vhdx_create(const char *filename, QEMUOptionParameter *options,
goto exit;
}
- ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
+ ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
+ NULL, &local_err);
if (ret < 0) {
error_propagate(errp, local_err);
goto exit;
diff --git a/block/vmdk.c b/block/vmdk.c
index e007793..70bf011 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -776,8 +776,9 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
path_combine(extent_path, sizeof(extent_path),
desc_file_path, fname);
- ret = bdrv_file_open(&extent_file, extent_path, NULL, NULL,
- bs->open_flags, errp);
+ extent_file = NULL;
+ ret = bdrv_open(&extent_file, extent_path, NULL, NULL,
+ bs->open_flags | BDRV_O_PROTOCOL, NULL, errp);
if (ret) {
return ret;
}
@@ -1493,7 +1494,8 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
goto exit;
}
- ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
+ ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
+ NULL, &local_err);
if (ret < 0) {
error_propagate(errp, local_err);
goto exit;
@@ -1831,7 +1833,8 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
goto exit;
}
}
- ret = bdrv_file_open(&new_bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
+ ret = bdrv_open(&new_bs, filename, NULL, NULL,
+ BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err);
if (ret < 0) {
error_setg_errno(errp, -ret, "Could not write description");
goto exit;
diff --git a/include/block/block.h b/include/block/block.h
index a421041..bf78db5 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -102,6 +102,9 @@ typedef enum {
#define BDRV_O_CHECK 0x1000 /* open solely for consistency check */
#define BDRV_O_ALLOW_RDWR 0x2000 /* allow reopen to change from r/o to r/w */
#define BDRV_O_UNMAP 0x4000 /* execute guest UNMAP/TRIM operations */
+#define BDRV_O_PROTOCOL 0x8000 /* if no block driver is explicitly given:
+ select an appropriate protocol driver,
+ ignoring the format layer */
#define BDRV_O_CACHE_MASK (BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)
@@ -183,9 +186,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
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,
- const char *reference, QDict *options, int flags,
- Error **errp);
int bdrv_open_image(BlockDriverState **pbs, const char *filename,
QDict *options, const char *bdref_key, int flags,
bool force_raw, bool allow_none, Error **errp);
diff --git a/qemu-io.c b/qemu-io.c
index 61d54c0..71d7806 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -59,7 +59,9 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
}
if (growable) {
- if (bdrv_file_open(&qemuio_bs, name, NULL, opts, flags, &local_err)) {
+ if (bdrv_open(&qemuio_bs, name, NULL, opts, flags | BDRV_O_PROTOCOL,
+ NULL, &local_err))
+ {
fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
error_get_pretty(local_err));
error_free(local_err);
--
1.8.5.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 4/8] block: Reuse reference handling from bdrv_open()
2014-02-08 17:39 [Qemu-devel] [PATCH v2 0/8] block: Integrate bdrv_file_open() into bdrv_open() Max Reitz
` (2 preceding siblings ...)
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 3/8] block: Make bdrv_file_open() static Max Reitz
@ 2014-02-08 17:39 ` Max Reitz
2014-02-10 13:42 ` Benoît Canet
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 5/8] block: Remove bdrv_new() from bdrv_file_open() Max Reitz
` (4 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Max Reitz @ 2014-02-08 17:39 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Benoît Canet, Stefan Hajnoczi,
Max Reitz
Remove the reference parameter and the related handling code from
bdrv_file_open(), since it exists in bdrv_open() now as well.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
block.c | 33 +++++++--------------------------
1 file changed, 7 insertions(+), 26 deletions(-)
diff --git a/block.c b/block.c
index 432d659..4c105bb 100644
--- a/block.c
+++ b/block.c
@@ -954,8 +954,7 @@ free_and_fail:
* dictionary, it needs to use QINCREF() before calling bdrv_file_open.
*/
static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
- const char *reference, QDict *options, int flags,
- Error **errp)
+ QDict *options, int flags, Error **errp)
{
BlockDriverState *bs = NULL;
BlockDriver *drv;
@@ -969,24 +968,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
options = qdict_new();
}
- if (reference) {
- if (filename || qdict_size(options)) {
- error_setg(errp, "Cannot reference an existing block device with "
- "additional options or a new filename");
- return -EINVAL;
- }
- QDECREF(options);
-
- bs = bdrv_find(reference);
- if (!bs) {
- error_setg(errp, "Cannot find block device '%s'", reference);
- return -ENODEV;
- }
- bdrv_ref(bs);
- *pbs = bs;
- return 0;
- }
-
bs = bdrv_new("");
bs->options = options;
options = qdict_clone_shallow(options);
@@ -1235,12 +1216,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
assert(pbs);
- if (flags & BDRV_O_PROTOCOL) {
- assert(!drv);
- return bdrv_file_open(pbs, filename, reference, options,
- flags & ~BDRV_O_PROTOCOL, errp);
- }
-
if (reference) {
bool options_non_empty = options ? qdict_size(options) : false;
QDECREF(options);
@@ -1267,6 +1242,12 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
return 0;
}
+ if (flags & BDRV_O_PROTOCOL) {
+ assert(!drv);
+ return bdrv_file_open(pbs, filename, options, flags & ~BDRV_O_PROTOCOL,
+ errp);
+ }
+
if (*pbs) {
bs = *pbs;
} else {
--
1.8.5.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 5/8] block: Remove bdrv_new() from bdrv_file_open()
2014-02-08 17:39 [Qemu-devel] [PATCH v2 0/8] block: Integrate bdrv_file_open() into bdrv_open() Max Reitz
` (3 preceding siblings ...)
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 4/8] block: Reuse reference handling from bdrv_open() Max Reitz
@ 2014-02-08 17:39 ` Max Reitz
2014-02-10 13:49 ` Benoît Canet
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 6/8] block: Handle bs->options in bdrv_open() only Max Reitz
` (3 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Max Reitz @ 2014-02-08 17:39 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Benoît Canet, Stefan Hajnoczi,
Max Reitz
Change bdrv_file_open() to take a simple pointer to an already existing
BDS instead of an indirect one. The BDS will be created in bdrv_open()
if necessary.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/block.c b/block.c
index 4c105bb..c57ad2c 100644
--- a/block.c
+++ b/block.c
@@ -953,10 +953,9 @@ free_and_fail:
* after the call (even on failure), so if the caller intends to reuse the
* dictionary, it needs to use QINCREF() before calling bdrv_file_open.
*/
-static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
+static int bdrv_file_open(BlockDriverState *bs, const char *filename,
QDict *options, int flags, Error **errp)
{
- BlockDriverState *bs = NULL;
BlockDriver *drv;
const char *drvname;
bool allow_protocol_prefix = false;
@@ -968,7 +967,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
options = qdict_new();
}
- bs = bdrv_new("");
bs->options = options;
options = qdict_clone_shallow(options);
@@ -1042,7 +1040,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
QDECREF(options);
bs->growable = 1;
- *pbs = bs;
return 0;
fail:
@@ -1050,7 +1047,6 @@ fail:
if (!bs->drv) {
QDECREF(bs->options);
}
- bdrv_unref(bs);
return ret;
}
@@ -1242,18 +1238,24 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
return 0;
}
- if (flags & BDRV_O_PROTOCOL) {
- assert(!drv);
- return bdrv_file_open(pbs, filename, options, flags & ~BDRV_O_PROTOCOL,
- errp);
- }
-
if (*pbs) {
bs = *pbs;
} else {
bs = bdrv_new("");
}
+ if (flags & BDRV_O_PROTOCOL) {
+ assert(!drv);
+ ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
+ errp);
+ if (ret && !*pbs) {
+ bdrv_unref(bs);
+ } else if (!ret) {
+ *pbs = bs;
+ }
+ return ret;
+ }
+
/* NULL means an empty set of options */
if (options == NULL) {
options = qdict_new();
--
1.8.5.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 6/8] block: Handle bs->options in bdrv_open() only
2014-02-08 17:39 [Qemu-devel] [PATCH v2 0/8] block: Integrate bdrv_file_open() into bdrv_open() Max Reitz
` (4 preceding siblings ...)
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 5/8] block: Remove bdrv_new() from bdrv_file_open() Max Reitz
@ 2014-02-08 17:39 ` Max Reitz
2014-02-10 16:23 ` Benoît Canet
2014-02-10 16:23 ` Benoît Canet
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 7/8] block: Reuse success path from bdrv_open() Max Reitz
` (2 subsequent siblings)
8 siblings, 2 replies; 25+ messages in thread
From: Max Reitz @ 2014-02-08 17:39 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Benoît Canet, Stefan Hajnoczi,
Max Reitz
The fail paths of bdrv_file_open() and bdrv_open() naturally exhibit
similarities, thus it is possible to reuse the one from bdrv_open() and
shorten the one in bdrv_file_open() accordingly.
Also, setting bs->options in bdrv_file_open() is not necessary if it is
already done in bdrv_open().
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block.c | 38 +++++++++++++++-----------------------
1 file changed, 15 insertions(+), 23 deletions(-)
diff --git a/block.c b/block.c
index c57ad2c..1fcd7e0 100644
--- a/block.c
+++ b/block.c
@@ -962,14 +962,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
Error *local_err = NULL;
int ret;
- /* NULL means an empty set of options */
- if (options == NULL) {
- options = qdict_new();
- }
-
- bs->options = options;
- options = qdict_clone_shallow(options);
-
/* Fetch the file name from the options QDict if necessary */
if (!filename) {
filename = qdict_get_try_str(options, "filename");
@@ -1044,9 +1036,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
fail:
QDECREF(options);
- if (!bs->drv) {
- QDECREF(bs->options);
- }
return ret;
}
@@ -1244,18 +1233,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
bs = bdrv_new("");
}
- if (flags & BDRV_O_PROTOCOL) {
- assert(!drv);
- ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
- errp);
- if (ret && !*pbs) {
- bdrv_unref(bs);
- } else if (!ret) {
- *pbs = bs;
- }
- return ret;
- }
-
/* NULL means an empty set of options */
if (options == NULL) {
options = qdict_new();
@@ -1263,6 +1240,21 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
bs->options = options;
options = qdict_clone_shallow(options);
+ if (flags & BDRV_O_PROTOCOL) {
+ assert(!drv);
+ ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
+ &local_err);
+ options = NULL;
+ if (!ret) {
+ *pbs = bs;
+ return 0;
+ } else if (bs->drv) {
+ goto close_and_fail;
+ } else {
+ goto fail;
+ }
+ }
+
/* For snapshot=on, create a temporary qcow2 overlay */
if (flags & BDRV_O_SNAPSHOT) {
BlockDriverState *bs1;
--
1.8.5.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 7/8] block: Reuse success path from bdrv_open()
2014-02-08 17:39 [Qemu-devel] [PATCH v2 0/8] block: Integrate bdrv_file_open() into bdrv_open() Max Reitz
` (5 preceding siblings ...)
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 6/8] block: Handle bs->options in bdrv_open() only Max Reitz
@ 2014-02-08 17:39 ` Max Reitz
2014-02-10 14:56 ` Kevin Wolf
2014-02-10 16:28 ` Benoît Canet
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 8/8] block: Remove bdrv_open_image()'s force_raw option Max Reitz
2014-02-10 15:01 ` [Qemu-devel] [PATCH v2 0/8] block: Integrate bdrv_file_open() into bdrv_open() Kevin Wolf
8 siblings, 2 replies; 25+ messages in thread
From: Max Reitz @ 2014-02-08 17:39 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Benoît Canet, Stefan Hajnoczi,
Max Reitz
The fail and success paths of bdrv_file_open() may be further shortened
by reusing code already existent in bdrv_open(). This includes
bdrv_file_open() not taking the reference to options which allows the
removal of QDECREF(options) in that function.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block.c | 68 ++++++++++++++++++++++++++++++++---------------------------------
1 file changed, 34 insertions(+), 34 deletions(-)
diff --git a/block.c b/block.c
index 1fcd7e0..22e5a44 100644
--- a/block.c
+++ b/block.c
@@ -948,13 +948,15 @@ free_and_fail:
/*
* Opens a file using a protocol (file, host_device, nbd, ...)
*
- * 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 belongs to the block layer
- * after the call (even on failure), so if the caller intends to reuse the
- * dictionary, it needs to use QINCREF() before calling bdrv_file_open.
+ * options is an indirect pointer to a QDict of options to pass to the block
+ * drivers, or pointer to NULL for an empty set of options. If this function
+ * takes ownership of the QDict reference, it will set *options to NULL;
+ * otherwise, it will contain unused/unrecognized options after this function
+ * returns. Then, the caller is responsible for freeing it. If it intends to
+ * reuse the QDict, QINCREF() should be called beforehand.
*/
static int bdrv_file_open(BlockDriverState *bs, const char *filename,
- QDict *options, int flags, Error **errp)
+ QDict **options, int flags, Error **errp)
{
BlockDriver *drv;
const char *drvname;
@@ -964,9 +966,9 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
/* Fetch the file name from the options QDict if necessary */
if (!filename) {
- filename = qdict_get_try_str(options, "filename");
- } else if (filename && !qdict_haskey(options, "filename")) {
- qdict_put(options, "filename", qstring_from_str(filename));
+ filename = qdict_get_try_str(*options, "filename");
+ } else if (filename && !qdict_haskey(*options, "filename")) {
+ qdict_put(*options, "filename", qstring_from_str(filename));
allow_protocol_prefix = true;
} else {
error_setg(errp, "Can't specify 'file' and 'filename' options at the "
@@ -976,13 +978,13 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
}
/* Find the right block driver */
- drvname = qdict_get_try_str(options, "driver");
+ drvname = qdict_get_try_str(*options, "driver");
if (drvname) {
drv = bdrv_find_format(drvname);
if (!drv) {
error_setg(errp, "Unknown driver '%s'", drvname);
}
- qdict_del(options, "driver");
+ qdict_del(*options, "driver");
} else if (filename) {
drv = bdrv_find_protocol(filename, allow_protocol_prefix);
if (!drv) {
@@ -1001,41 +1003,35 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
/* Parse the filename and open it */
if (drv->bdrv_parse_filename && filename) {
- drv->bdrv_parse_filename(filename, options, &local_err);
+ drv->bdrv_parse_filename(filename, *options, &local_err);
if (error_is_set(&local_err)) {
error_propagate(errp, local_err);
ret = -EINVAL;
goto fail;
}
- qdict_del(options, "filename");
+ qdict_del(*options, "filename");
+ } else if (drv->bdrv_needs_filename && !filename) {
+ error_setg(errp, "The '%s' block driver requires a file name",
+ drv->format_name);
+ ret = -EINVAL;
+ goto fail;
}
if (!drv->bdrv_file_open) {
- ret = bdrv_open(&bs, filename, NULL, options, flags, drv, &local_err);
- options = NULL;
+ ret = bdrv_open(&bs, filename, NULL, *options, flags, drv, &local_err);
+ *options = NULL;
} else {
- ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
+ ret = bdrv_open_common(bs, NULL, *options, flags, drv, &local_err);
}
if (ret < 0) {
error_propagate(errp, local_err);
goto fail;
}
- /* Check if any unknown options were used */
- if (options && (qdict_size(options) != 0)) {
- const QDictEntry *entry = qdict_first(options);
- error_setg(errp, "Block protocol '%s' doesn't support the option '%s'",
- drv->format_name, entry->key);
- ret = -EINVAL;
- goto fail;
- }
- QDECREF(options);
-
bs->growable = 1;
return 0;
fail:
- QDECREF(options);
return ret;
}
@@ -1242,12 +1238,10 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
if (flags & BDRV_O_PROTOCOL) {
assert(!drv);
- ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
+ ret = bdrv_file_open(bs, filename, &options, flags & ~BDRV_O_PROTOCOL,
&local_err);
- options = NULL;
if (!ret) {
- *pbs = bs;
- return 0;
+ goto done;
} else if (bs->drv) {
goto close_and_fail;
} else {
@@ -1384,12 +1378,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
}
}
+done:
/* Check if any unknown options were used */
- if (qdict_size(options) != 0) {
+ if (options && (qdict_size(options) != 0)) {
const QDictEntry *entry = qdict_first(options);
- error_setg(errp, "Block format '%s' used by device '%s' doesn't "
- "support the option '%s'", drv->format_name, bs->device_name,
- entry->key);
+ if (flags & BDRV_O_PROTOCOL) {
+ error_setg(errp, "Block protocol '%s' doesn't support the option "
+ "'%s'", drv->format_name, entry->key);
+ } else {
+ error_setg(errp, "Block format '%s' used by device '%s' doesn't "
+ "support the option '%s'", drv->format_name,
+ bs->device_name, entry->key);
+ }
ret = -EINVAL;
goto close_and_fail;
--
1.8.5.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 8/8] block: Remove bdrv_open_image()'s force_raw option
2014-02-08 17:39 [Qemu-devel] [PATCH v2 0/8] block: Integrate bdrv_file_open() into bdrv_open() Max Reitz
` (6 preceding siblings ...)
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 7/8] block: Reuse success path from bdrv_open() Max Reitz
@ 2014-02-08 17:39 ` Max Reitz
2014-02-10 16:31 ` Benoît Canet
2014-02-10 15:01 ` [Qemu-devel] [PATCH v2 0/8] block: Integrate bdrv_file_open() into bdrv_open() Kevin Wolf
8 siblings, 1 reply; 25+ messages in thread
From: Max Reitz @ 2014-02-08 17:39 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Benoît Canet, Stefan Hajnoczi,
Max Reitz
This option is now unnecessary since specifying BDRV_O_PROTOCOL as flag
will do exactly the same.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block.c | 28 ++++------------------------
block/blkdebug.c | 2 +-
block/blkverify.c | 4 ++--
include/block/block.h | 2 +-
4 files changed, 8 insertions(+), 28 deletions(-)
diff --git a/block.c b/block.c
index 22e5a44..a1630e9 100644
--- a/block.c
+++ b/block.c
@@ -1107,10 +1107,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
* Opens a disk image whose options are given as BlockdevRef in another block
* device's options.
*
- * If force_raw is true, bdrv_file_open() will be used, thereby preventing any
- * image format auto-detection. If it is false and a filename is given,
- * bdrv_open() will be used for auto-detection.
- *
* If allow_none is true, no image will be opened if filename is false and no
* BlockdevRef is given. *pbs will remain unchanged and 0 will be returned.
*
@@ -1127,7 +1123,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
*/
int bdrv_open_image(BlockDriverState **pbs, const char *filename,
QDict *options, const char *bdref_key, int flags,
- bool force_raw, bool allow_none, Error **errp)
+ bool allow_none, Error **errp)
{
QDict *image_options;
int ret;
@@ -1150,23 +1146,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
goto done;
}
- if (filename && !force_raw) {
- /* If a filename is given and the block driver should be detected
- automatically (instead of using none), use bdrv_open() in order to do
- that auto-detection. */
- if (reference) {
- error_setg(errp, "Cannot reference an existing block device while "
- "giving a filename");
- ret = -EINVAL;
- goto done;
- }
-
- ret = bdrv_open(pbs, filename, NULL, image_options, flags, NULL, errp);
- } else {
- *pbs = NULL;
- ret = bdrv_open(pbs, filename, reference, image_options,
- flags | BDRV_O_PROTOCOL, NULL, errp);
- }
+ ret = bdrv_open(pbs, filename, reference, image_options, flags, NULL, errp);
done:
qdict_del(options, bdref_key);
@@ -1324,8 +1304,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
assert(file == NULL);
ret = bdrv_open_image(&file, filename, options, "file",
- bdrv_open_flags(bs, flags | BDRV_O_UNMAP), true, true,
- &local_err);
+ bdrv_open_flags(bs, flags | BDRV_O_UNMAP) |
+ BDRV_O_PROTOCOL, true, &local_err);
if (ret < 0) {
goto fail;
}
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 053fa4c..6707f49 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -412,7 +412,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
/* Open the backing file */
assert(bs->file == NULL);
ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-image"), options, "image",
- flags, true, false, &local_err);
+ flags | BDRV_O_PROTOCOL, false, &local_err);
if (ret < 0) {
error_propagate(errp, local_err);
goto out;
diff --git a/block/blkverify.c b/block/blkverify.c
index 86585e7..b57da59 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -137,7 +137,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
/* Open the raw file */
assert(bs->file == NULL);
ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-raw"), options,
- "raw", flags, true, false, &local_err);
+ "raw", flags | BDRV_O_PROTOCOL, false, &local_err);
if (ret < 0) {
error_propagate(errp, local_err);
goto fail;
@@ -146,7 +146,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
/* Open the test file */
assert(s->test_file == NULL);
ret = bdrv_open_image(&s->test_file, qemu_opt_get(opts, "x-image"), options,
- "test", flags, false, false, &local_err);
+ "test", flags, false, &local_err);
if (ret < 0) {
error_propagate(errp, local_err);
s->test_file = NULL;
diff --git a/include/block/block.h b/include/block/block.h
index bf78db5..780f48b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -188,7 +188,7 @@ int bdrv_parse_cache_flags(const char *mode, int *flags);
int bdrv_parse_discard_flags(const char *mode, int *flags);
int bdrv_open_image(BlockDriverState **pbs, const char *filename,
QDict *options, const char *bdref_key, int flags,
- bool force_raw, bool allow_none, Error **errp);
+ bool allow_none, Error **errp);
int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
int bdrv_open(BlockDriverState **pbs, const char *filename,
const char *reference, QDict *options, int flags,
--
1.8.5.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/8] block: Change BDS parameter of bdrv_open() to **
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 1/8] block: Change BDS parameter of bdrv_open() to ** Max Reitz
@ 2014-02-10 12:42 ` Kevin Wolf
2014-02-12 0:10 ` Max Reitz
2014-02-10 13:17 ` Benoît Canet
1 sibling, 1 reply; 25+ messages in thread
From: Kevin Wolf @ 2014-02-10 12:42 UTC (permalink / raw)
To: Max Reitz; +Cc: Jeff Cody, qemu-devel, Stefan Hajnoczi, Benoît Canet
Am 08.02.2014 um 18:39 hat Max Reitz geschrieben:
> Make bdrv_open() take a pointer to a BDS pointer, similarly to
> bdrv_file_open(). If a pointer to a NULL pointer is given, bdrv_open()
> will create a new BDS with an empty name; if the BDS pointer is not
> NULL, that existing BDS will be reused (in the same way as bdrv_open()
> already did).
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block.c | 64 +++++++++++++++++++++++++++++++--------------------
> block/blkdebug.c | 1 +
> block/blkverify.c | 2 ++
> block/qcow2.c | 14 +++++++----
> block/vmdk.c | 5 ++--
> block/vvfat.c | 6 ++---
> blockdev.c | 20 ++++++++--------
> hw/block/xen_disk.c | 2 +-
> include/block/block.h | 2 +-
> qemu-img.c | 10 ++++----
> qemu-io.c | 2 +-
> qemu-nbd.c | 2 +-
> 12 files changed, 72 insertions(+), 58 deletions(-)
> @@ -1160,6 +1158,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
> * BlockdevRef.
> *
> * The BlockdevRef will be removed from the options QDict.
> + *
> + * As with bdrv_open(), if *pbs is NULL, a new BDS will be created with a
> + * pointer to it stored there. If it is not NULL, the referenced BDS will
> + * be reused.
> */
> int bdrv_open_image(BlockDriverState **pbs, const char *filename,
> QDict *options, const char *bdref_key, int flags,
There are no callers that make use of *pbs != NULL. Are you planning to
add such users? Otherwise, we could just assert() it here instead of
documenting behaviour that is never used.
Kevin
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/8] block: Change BDS parameter of bdrv_open() to **
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 1/8] block: Change BDS parameter of bdrv_open() to ** Max Reitz
2014-02-10 12:42 ` Kevin Wolf
@ 2014-02-10 13:17 ` Benoît Canet
2014-02-12 0:15 ` Max Reitz
1 sibling, 1 reply; 25+ messages in thread
From: Benoît Canet @ 2014-02-10 13:17 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, Jeff Cody, qemu-devel, Stefan Hajnoczi
Le Saturday 08 Feb 2014 à 18:39:12 (+0100), Max Reitz a écrit :
> Make bdrv_open() take a pointer to a BDS pointer, similarly to
> bdrv_file_open(). If a pointer to a NULL pointer is given, bdrv_open()
> will create a new BDS with an empty name; if the BDS pointer is not
> NULL, that existing BDS will be reused (in the same way as bdrv_open()
> already did).
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block.c | 64 +++++++++++++++++++++++++++++++--------------------
> block/blkdebug.c | 1 +
> block/blkverify.c | 2 ++
> block/qcow2.c | 14 +++++++----
> block/vmdk.c | 5 ++--
> block/vvfat.c | 6 ++---
> blockdev.c | 20 ++++++++--------
> hw/block/xen_disk.c | 2 +-
> include/block/block.h | 2 +-
> qemu-img.c | 10 ++++----
> qemu-io.c | 2 +-
> qemu-nbd.c | 2 +-
> 12 files changed, 72 insertions(+), 58 deletions(-)
>
> diff --git a/block.c b/block.c
> index 636aa11..40a585a 100644
> --- a/block.c
> +++ b/block.c
> @@ -1040,7 +1040,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> }
>
> if (!drv->bdrv_file_open) {
> - ret = bdrv_open(bs, filename, options, flags, drv, &local_err);
> + ret = bdrv_open(&bs, filename, options, flags, drv, &local_err);
> options = NULL;
> } else {
> ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
> @@ -1109,8 +1109,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
> sizeof(backing_filename));
> }
>
> - bs->backing_hd = bdrv_new("");
> -
> if (bs->backing_format[0] != '\0') {
> back_drv = bdrv_find_format(bs->backing_format);
> }
> @@ -1119,11 +1117,11 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
> back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT |
> BDRV_O_COPY_ON_READ);
>
> - ret = bdrv_open(bs->backing_hd,
> + assert(bs->backing_hd == NULL);
> + 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",
> @@ -1160,6 +1158,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
> * BlockdevRef.
> *
> * The BlockdevRef will be removed from the options QDict.
> + *
> + * As with bdrv_open(), if *pbs is NULL, a new BDS will be created with a
> + * pointer to it stored there. If it is not NULL, the referenced BDS will
> + * be reused.
> */
> int bdrv_open_image(BlockDriverState **pbs, const char *filename,
> QDict *options, const char *bdref_key, int flags,
> @@ -1190,8 +1192,6 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
> /* If a filename is given and the block driver should be detected
> automatically (instead of using none), use bdrv_open() in order to do
> that auto-detection. */
> - BlockDriverState *bs;
> -
> if (reference) {
> error_setg(errp, "Cannot reference an existing block device while "
> "giving a filename");
> @@ -1199,13 +1199,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
> goto done;
> }
>
> - bs = bdrv_new("");
> - ret = bdrv_open(bs, filename, image_options, flags, NULL, errp);
> - if (ret < 0) {
> - bdrv_unref(bs);
> - } else {
> - *pbs = bs;
> - }
> + ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp);
> } else {
> ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
> errp);
> @@ -1223,22 +1217,32 @@ done:
> * empty set of options. The reference to the QDict belongs to the block layer
> * after the call (even on failure), so if the caller intends to reuse the
> * dictionary, it needs to use QINCREF() before calling bdrv_open.
> + *
> + * If *pbs is NULL, a new BDS will be created with a pointer to it stored there.
> + * If it is not NULL, the referenced BDS will be reused.
> */
> -int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> +int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
> int flags, BlockDriver *drv, Error **errp)
> {
> int ret;
> /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
> char tmp_filename[PATH_MAX + 1];
> - BlockDriverState *file = NULL;
> + BlockDriverState *file = NULL, *bs;
> const char *drvname;
> Error *local_err = NULL;
>
> + assert(pbs);
> +
> + if (*pbs) {
> + bs = *pbs;
> + } else {
> + bs = bdrv_new("");
> + }
> +
> /* NULL means an empty set of options */
> if (options == NULL) {
> options = qdict_new();
> }
> -
nitpicking: spurious line removal.
> bs->options = options;
> options = qdict_clone_shallow(options);
>
> @@ -1254,12 +1258,11 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> instead of opening 'filename' directly */
>
> /* Get the required size from the image */
> - bs1 = bdrv_new("");
> QINCREF(options);
> - ret = bdrv_open(bs1, filename, options, BDRV_O_NO_BACKING,
> + bs1 = NULL;
> + ret = bdrv_open(&bs1, filename, options, BDRV_O_NO_BACKING,
> drv, &local_err);
> if (ret < 0) {
> - bdrv_unref(bs1);
> goto fail;
> }
> total_size = bdrv_getlength(bs1) & BDRV_SECTOR_MASK;
> @@ -1316,6 +1319,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> flags |= BDRV_O_ALLOW_RDWR;
> }
>
> + assert(file == NULL);
> ret = bdrv_open_image(&file, filename, options, "file",
> bdrv_open_flags(bs, flags | BDRV_O_UNMAP), true, true,
> &local_err);
is_temporary can already be set to one the goto after the bdrv_open_image should
be unlink_and_fail yet I don't know if the fix belong to this patch.
> @@ -1387,6 +1391,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> bdrv_dev_change_media_cb(bs, true);
> }
>
> + *pbs = bs;
> return 0;
>
> unlink_and_fail:
> @@ -1400,13 +1405,24 @@ fail:
> QDECREF(bs->options);
> QDECREF(options);
> bs->options = NULL;
> + if (!*pbs) {
> + /* If *pbs is NULL, a new BDS has been created in this function and
> + needs to be freed now. Otherwise, it does not need to be closed,
> + since it has not really been opened yet. */
> + bdrv_unref(bs);
> + }
> if (error_is_set(&local_err)) {
> error_propagate(errp, local_err);
> }
> return ret;
>
> close_and_fail:
> - bdrv_close(bs);
> + /* See fail path, but now the BDS has to be always closed */
> + if (*pbs) {
> + bdrv_close(bs);
> + } else {
> + bdrv_unref(bs);
> + }
> QDECREF(options);
> if (error_is_set(&local_err)) {
> error_propagate(errp, local_err);
> @@ -5288,9 +5304,8 @@ void bdrv_img_create(const char *filename, const char *fmt,
> back_flags =
> flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
>
> - bs = bdrv_new("");
> -
> - ret = bdrv_open(bs, backing_file->value.s, NULL, back_flags,
> + bs = NULL;
> + ret = bdrv_open(&bs, backing_file->value.s, NULL, back_flags,
> backing_drv, &local_err);
> if (ret < 0) {
> error_setg_errno(errp, -ret, "Could not open '%s': %s",
> @@ -5298,7 +5313,6 @@ void bdrv_img_create(const char *filename, const char *fmt,
> error_get_pretty(local_err));
> error_free(local_err);
> local_err = NULL;
> - bdrv_unref(bs);
> goto out;
> }
> bdrv_get_geometry(bs, &size);
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 8eb0db0..053fa4c 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -410,6 +410,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
> s->state = 1;
>
> /* Open the backing file */
> + assert(bs->file == NULL);
> ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-image"), options, "image",
> flags, true, false, &local_err);
> if (ret < 0) {
> diff --git a/block/blkverify.c b/block/blkverify.c
> index cfcbcf4..86585e7 100644
> --- a/block/blkverify.c
> +++ b/block/blkverify.c
> @@ -135,6 +135,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
> }
>
> /* Open the raw file */
> + assert(bs->file == NULL);
> ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-raw"), options,
> "raw", flags, true, false, &local_err);
> if (ret < 0) {
> @@ -143,6 +144,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
> }
>
> /* Open the test file */
> + assert(s->test_file == NULL);
> ret = bdrv_open_image(&s->test_file, qemu_opt_get(opts, "x-image"), options,
> "test", flags, false, false, &local_err);
> if (ret < 0) {
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 0b4335c..c8e8ba7 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1543,7 +1543,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
> goto out;
> }
>
> - bdrv_close(bs);
> + bdrv_unref(bs);
> + bs = NULL;
>
> /*
> * And now open the image and make it consistent first (i.e. increase the
> @@ -1552,7 +1553,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
> */
> BlockDriver* drv = bdrv_find_format("qcow2");
> assert(drv != NULL);
> - ret = bdrv_open(bs, filename, NULL,
> + ret = bdrv_open(&bs, filename, NULL,
> BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, drv, &local_err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> @@ -1599,10 +1600,11 @@ static int qcow2_create2(const char *filename, int64_t total_size,
> }
> }
>
> - bdrv_close(bs);
> + bdrv_unref(bs);
> + bs = NULL;
>
> /* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning */
> - ret = bdrv_open(bs, filename, NULL,
> + ret = bdrv_open(&bs, filename, NULL,
> BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING,
> drv, &local_err);
> if (error_is_set(&local_err)) {
> @@ -1612,7 +1614,9 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>
> ret = 0;
> out:
> - bdrv_unref(bs);
> + if (bs) {
> + bdrv_unref(bs);
> + }
> return ret;
> }
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index e809e2e..52cd12a 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1755,10 +1755,9 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
> goto exit;
> }
> if (backing_file) {
> - BlockDriverState *bs = bdrv_new("");
> - ret = bdrv_open(bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp);
> + BlockDriverState *bs = NULL;
> + ret = bdrv_open(&bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp);
> if (ret != 0) {
> - bdrv_unref(bs);
> goto exit;
> }
> if (strcmp(bs->drv->format_name, "vmdk")) {
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 664941c..ae7bc6f 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -2936,15 +2936,13 @@ static int enable_write_target(BDRVVVFATState *s)
> goto err;
> }
>
> - s->qcow = bdrv_new("");
> -
> - ret = bdrv_open(s->qcow, s->qcow_filename, NULL,
> + s->qcow = NULL;
> + ret = bdrv_open(&s->qcow, s->qcow_filename, NULL,
> BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow,
> &local_err);
> if (ret < 0) {
> qerror_report_err(local_err);
> error_free(local_err);
> - bdrv_unref(s->qcow);
> goto err;
> }
>
> diff --git a/blockdev.c b/blockdev.c
> index 36ceece..6dd351c 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -510,7 +510,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
> bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>
> QINCREF(bs_opts);
> - ret = bdrv_open(dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
> + ret = bdrv_open(&dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
>
> if (ret < 0) {
> error_setg(errp, "could not open disk image %s: %s",
> @@ -1301,12 +1301,12 @@ static void external_snapshot_prepare(BlkTransactionState *common,
> qstring_from_str(snapshot_node_name));
> }
>
> - /* We will manually add the backing_hd field to the bs later */
> - state->new_bs = bdrv_new("");
> /* TODO Inherit bs->options or only take explicit options with an
> * extended QMP command? */
> - ret = bdrv_open(state->new_bs, new_image_file, options,
> + assert(state->new_bs == NULL);
> + ret = bdrv_open(&state->new_bs, new_image_file, options,
> flags | BDRV_O_NO_BACKING, drv, &local_err);
> + /* We will manually add the backing_hd field to the bs later */
> if (ret != 0) {
> error_propagate(errp, local_err);
> }
> @@ -1555,7 +1555,7 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
> Error *local_err = NULL;
> int ret;
>
> - ret = bdrv_open(bs, filename, NULL, bdrv_flags, drv, &local_err);
> + ret = bdrv_open(&bs, filename, NULL, bdrv_flags, drv, &local_err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> return;
> @@ -1991,10 +1991,9 @@ void qmp_drive_backup(const char *device, const char *target,
> return;
> }
>
> - target_bs = bdrv_new("");
> - ret = bdrv_open(target_bs, target, NULL, flags, drv, &local_err);
> + target_bs = NULL;
> + ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err);
> if (ret < 0) {
> - bdrv_unref(target_bs);
> error_propagate(errp, local_err);
> return;
> }
> @@ -2135,11 +2134,10 @@ void qmp_drive_mirror(const char *device, const char *target,
> /* Mirroring takes care of copy-on-write using the source's backing
> * file.
> */
> - target_bs = bdrv_new("");
> - ret = bdrv_open(target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
> + target_bs = NULL;
> + ret = bdrv_open(&target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
> &local_err);
> if (ret < 0) {
> - bdrv_unref(target_bs);
> error_propagate(errp, local_err);
> return;
> }
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 098f6c6..14e6d0b 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -813,7 +813,7 @@ static int blk_connect(struct XenDevice *xendev)
> Error *local_err = NULL;
> BlockDriver *drv = bdrv_find_whitelisted_format(blkdev->fileproto,
> readonly);
> - if (bdrv_open(blkdev->bs,
> + if (bdrv_open(&blkdev->bs,
> blkdev->filename, NULL, qflags, drv, &local_err) != 0)
> {
> xen_be_printf(&blkdev->xendev, 0, "error: %s\n",
> diff --git a/include/block/block.h b/include/block/block.h
> index 963a61f..980869d 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -190,7 +190,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
> QDict *options, const char *bdref_key, int flags,
> bool force_raw, bool allow_none, Error **errp);
> int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
> -int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> +int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
> int flags, BlockDriver *drv, Error **errp);
> BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
> BlockDriverState *bs, int flags);
> diff --git a/qemu-img.c b/qemu-img.c
> index c989850..897aa56 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -289,7 +289,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
> drv = NULL;
> }
>
> - ret = bdrv_open(bs, filename, NULL, flags, drv, &local_err);
> + ret = bdrv_open(&bs, filename, NULL, flags, drv, &local_err);
> if (ret < 0) {
> error_report("Could not open '%s': %s", filename,
> error_get_pretty(local_err));
> @@ -310,9 +310,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
> }
> return bs;
> fail:
> - if (bs) {
> - bdrv_unref(bs);
> - }
> + bdrv_unref(bs);
> return NULL;
> }
>
> @@ -2314,7 +2312,7 @@ static int img_rebase(int argc, char **argv)
>
> bs_old_backing = bdrv_new("old_backing");
> bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
> - ret = bdrv_open(bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
> + ret = bdrv_open(&bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
> old_backing_drv, &local_err);
> if (ret) {
> error_report("Could not open old backing file '%s': %s",
> @@ -2324,7 +2322,7 @@ static int img_rebase(int argc, char **argv)
> }
> if (out_baseimg[0]) {
> bs_new_backing = bdrv_new("new_backing");
> - ret = bdrv_open(bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
> + ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
> new_backing_drv, &local_err);
> if (ret) {
> error_report("Could not open new backing file '%s': %s",
> diff --git a/qemu-io.c b/qemu-io.c
> index 7f459d8..8da8f6e 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -68,7 +68,7 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
> } else {
> qemuio_bs = bdrv_new("hda");
>
> - if (bdrv_open(qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
> + if (bdrv_open(&qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
> fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
> error_get_pretty(local_err));
> error_free(local_err);
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 136e8c9..0cf123c 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -597,7 +597,7 @@ int main(int argc, char **argv)
>
> bs = bdrv_new("hda");
> srcpath = argv[optind];
> - ret = bdrv_open(bs, srcpath, NULL, flags, drv, &local_err);
> + ret = bdrv_open(&bs, srcpath, NULL, flags, drv, &local_err);
> if (ret < 0) {
> errno = -ret;
> err(EXIT_FAILURE, "Failed to bdrv_open '%s': %s", argv[optind],
> --
> 1.8.5.4
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/8] block: Add reference parameter to bdrv_open()
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 2/8] block: Add reference parameter to bdrv_open() Max Reitz
@ 2014-02-10 13:30 ` Benoît Canet
2014-02-12 0:17 ` Max Reitz
0 siblings, 1 reply; 25+ messages in thread
From: Benoît Canet @ 2014-02-10 13:30 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, Jeff Cody, qemu-devel, Stefan Hajnoczi
Le Saturday 08 Feb 2014 à 18:39:13 (+0100), Max Reitz a écrit :
> Allow bdrv_open() to handle references to existing block devices just as
> bdrv_file_open() is already capable of.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block.c | 41 ++++++++++++++++++++++++++++++++++-------
> block/qcow2.c | 4 ++--
> block/vmdk.c | 3 ++-
> block/vvfat.c | 2 +-
> blockdev.c | 12 ++++++------
> hw/block/xen_disk.c | 4 ++--
> include/block/block.h | 5 +++--
> qemu-img.c | 8 ++++----
> qemu-io.c | 4 +++-
> qemu-nbd.c | 2 +-
> 10 files changed, 58 insertions(+), 27 deletions(-)
>
> diff --git a/block.c b/block.c
> index 40a585a..3a32c37 100644
> --- a/block.c
> +++ b/block.c
> @@ -1040,7 +1040,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> }
>
> if (!drv->bdrv_file_open) {
> - ret = bdrv_open(&bs, filename, options, flags, drv, &local_err);
> + ret = bdrv_open(&bs, filename, NULL, options, flags, drv, &local_err);
> options = NULL;
> } else {
> ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
> @@ -1119,7 +1119,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>
> assert(bs->backing_hd == NULL);
> ret = bdrv_open(&bs->backing_hd,
> - *backing_filename ? backing_filename : NULL, options,
> + *backing_filename ? backing_filename : NULL, NULL, options,
> back_flags, back_drv, &local_err);
> if (ret < 0) {
> bs->backing_hd = NULL;
> @@ -1199,7 +1199,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
> goto done;
> }
>
> - ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp);
> + ret = bdrv_open(pbs, filename, NULL, image_options, flags, NULL, errp);
> } else {
> ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
> errp);
> @@ -1221,8 +1221,9 @@ done:
> * If *pbs is NULL, a new BDS will be created with a pointer to it stored there.
> * If it is not NULL, the referenced BDS will be reused.
> */
> -int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
> - int flags, BlockDriver *drv, Error **errp)
> +int bdrv_open(BlockDriverState **pbs, const char *filename,
> + const char *reference, QDict *options, int flags,
> + BlockDriver *drv, Error **errp)
Maybe a little reference to the reference parameters in the comments would help
?
Aside from that:
Reviewed-by: Benoit Canet <benoit@irqsave.net>
> {
> int ret;
> /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
> @@ -1233,6 +1234,32 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
>
> assert(pbs);
>
> + if (reference) {
> + bool options_non_empty = options ? qdict_size(options) : false;
> + QDECREF(options);
> +
> + if (*pbs) {
> + error_setg(errp, "Cannot reuse an existing BDS when referencing "
> + "another block device");
> + return -EINVAL;
> + }
> +
> + if (filename || options_non_empty) {
> + error_setg(errp, "Cannot reference an existing block device with "
> + "additional options or a new filename");
> + return -EINVAL;
> + }
> +
> + bs = bdrv_find(reference);
> + if (!bs) {
> + error_setg(errp, "Cannot find block device '%s'", reference);
> + return -ENODEV;
> + }
> + bdrv_ref(bs);
> + *pbs = bs;
> + return 0;
> + }
> +
> if (*pbs) {
> bs = *pbs;
> } else {
> @@ -1260,7 +1287,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
> /* Get the required size from the image */
> QINCREF(options);
> bs1 = NULL;
> - ret = bdrv_open(&bs1, filename, options, BDRV_O_NO_BACKING,
> + ret = bdrv_open(&bs1, filename, NULL, options, BDRV_O_NO_BACKING,
> drv, &local_err);
> if (ret < 0) {
> goto fail;
> @@ -5305,7 +5332,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
> flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
>
> bs = NULL;
> - ret = bdrv_open(&bs, backing_file->value.s, NULL, back_flags,
> + ret = bdrv_open(&bs, backing_file->value.s, NULL, NULL, back_flags,
> backing_drv, &local_err);
> if (ret < 0) {
> error_setg_errno(errp, -ret, "Could not open '%s': %s",
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c8e8ba7..6996276 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1553,7 +1553,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
> */
> BlockDriver* drv = bdrv_find_format("qcow2");
> assert(drv != NULL);
> - ret = bdrv_open(&bs, filename, NULL,
> + ret = bdrv_open(&bs, filename, NULL, NULL,
> BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, drv, &local_err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> @@ -1604,7 +1604,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
> bs = NULL;
>
> /* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning */
> - ret = bdrv_open(&bs, filename, NULL,
> + ret = bdrv_open(&bs, filename, NULL, NULL,
> BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING,
> drv, &local_err);
> if (error_is_set(&local_err)) {
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 52cd12a..e007793 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1756,7 +1756,8 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
> }
> if (backing_file) {
> BlockDriverState *bs = NULL;
> - ret = bdrv_open(&bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp);
> + ret = bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_NO_BACKING, NULL,
> + errp);
> if (ret != 0) {
> goto exit;
> }
> diff --git a/block/vvfat.c b/block/vvfat.c
> index ae7bc6f..d7a830e 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -2937,7 +2937,7 @@ static int enable_write_target(BDRVVVFATState *s)
> }
>
> s->qcow = NULL;
> - ret = bdrv_open(&s->qcow, s->qcow_filename, NULL,
> + ret = bdrv_open(&s->qcow, s->qcow_filename, NULL, NULL,
> BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow,
> &local_err);
> if (ret < 0) {
> diff --git a/blockdev.c b/blockdev.c
> index 6dd351c..7b7e349 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -510,7 +510,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
> bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>
> QINCREF(bs_opts);
> - ret = bdrv_open(&dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
> + ret = bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, drv, &error);
>
> if (ret < 0) {
> error_setg(errp, "could not open disk image %s: %s",
> @@ -1304,7 +1304,7 @@ static void external_snapshot_prepare(BlkTransactionState *common,
> /* TODO Inherit bs->options or only take explicit options with an
> * extended QMP command? */
> assert(state->new_bs == NULL);
> - ret = bdrv_open(&state->new_bs, new_image_file, options,
> + ret = bdrv_open(&state->new_bs, new_image_file, NULL, options,
> flags | BDRV_O_NO_BACKING, drv, &local_err);
> /* We will manually add the backing_hd field to the bs later */
> if (ret != 0) {
> @@ -1555,7 +1555,7 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
> Error *local_err = NULL;
> int ret;
>
> - ret = bdrv_open(&bs, filename, NULL, bdrv_flags, drv, &local_err);
> + ret = bdrv_open(&bs, filename, NULL, NULL, bdrv_flags, drv, &local_err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> return;
> @@ -1992,7 +1992,7 @@ void qmp_drive_backup(const char *device, const char *target,
> }
>
> target_bs = NULL;
> - ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err);
> + ret = bdrv_open(&target_bs, target, NULL, NULL, flags, drv, &local_err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> return;
> @@ -2135,8 +2135,8 @@ void qmp_drive_mirror(const char *device, const char *target,
> * file.
> */
> target_bs = NULL;
> - ret = bdrv_open(&target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
> - &local_err);
> + ret = bdrv_open(&target_bs, target, NULL, NULL, flags | BDRV_O_NO_BACKING,
> + drv, &local_err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> return;
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 14e6d0b..61e6ff3 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -813,8 +813,8 @@ static int blk_connect(struct XenDevice *xendev)
> Error *local_err = NULL;
> BlockDriver *drv = bdrv_find_whitelisted_format(blkdev->fileproto,
> readonly);
> - if (bdrv_open(&blkdev->bs,
> - blkdev->filename, NULL, qflags, drv, &local_err) != 0)
> + if (bdrv_open(&blkdev->bs, blkdev->filename, NULL, NULL, qflags,
> + drv, &local_err) != 0)
> {
> xen_be_printf(&blkdev->xendev, 0, "error: %s\n",
> error_get_pretty(local_err));
> diff --git a/include/block/block.h b/include/block/block.h
> index 980869d..a421041 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -190,8 +190,9 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
> QDict *options, const char *bdref_key, int flags,
> bool force_raw, bool allow_none, Error **errp);
> int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
> -int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
> - int flags, BlockDriver *drv, Error **errp);
> +int bdrv_open(BlockDriverState **pbs, const char *filename,
> + const char *reference, QDict *options, int flags,
> + BlockDriver *drv, Error **errp);
> BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
> BlockDriverState *bs, int flags);
> int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
> diff --git a/qemu-img.c b/qemu-img.c
> index 897aa56..b834d52 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -289,7 +289,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
> drv = NULL;
> }
>
> - ret = bdrv_open(&bs, filename, NULL, flags, drv, &local_err);
> + ret = bdrv_open(&bs, filename, NULL, NULL, flags, drv, &local_err);
> if (ret < 0) {
> error_report("Could not open '%s': %s", filename,
> error_get_pretty(local_err));
> @@ -2312,7 +2312,7 @@ static int img_rebase(int argc, char **argv)
>
> bs_old_backing = bdrv_new("old_backing");
> bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
> - ret = bdrv_open(&bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
> + ret = bdrv_open(&bs_old_backing, backing_name, NULL, NULL, BDRV_O_FLAGS,
> old_backing_drv, &local_err);
> if (ret) {
> error_report("Could not open old backing file '%s': %s",
> @@ -2322,8 +2322,8 @@ static int img_rebase(int argc, char **argv)
> }
> if (out_baseimg[0]) {
> bs_new_backing = bdrv_new("new_backing");
> - ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
> - new_backing_drv, &local_err);
> + ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, NULL,
> + BDRV_O_FLAGS, new_backing_drv, &local_err);
> if (ret) {
> error_report("Could not open new backing file '%s': %s",
> out_baseimg, error_get_pretty(local_err));
> diff --git a/qemu-io.c b/qemu-io.c
> index 8da8f6e..61d54c0 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -68,7 +68,9 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
> } else {
> qemuio_bs = bdrv_new("hda");
>
> - if (bdrv_open(&qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
> + if (bdrv_open(&qemuio_bs, name, NULL, opts, flags, NULL, &local_err)
> + < 0)
> + {
> fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
> error_get_pretty(local_err));
> error_free(local_err);
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 0cf123c..711162c 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -597,7 +597,7 @@ int main(int argc, char **argv)
>
> bs = bdrv_new("hda");
> srcpath = argv[optind];
> - ret = bdrv_open(&bs, srcpath, NULL, flags, drv, &local_err);
> + ret = bdrv_open(&bs, srcpath, NULL, NULL, flags, drv, &local_err);
> if (ret < 0) {
> errno = -ret;
> err(EXIT_FAILURE, "Failed to bdrv_open '%s': %s", argv[optind],
> --
> 1.8.5.4
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/8] block: Make bdrv_file_open() static
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 3/8] block: Make bdrv_file_open() static Max Reitz
@ 2014-02-10 13:40 ` Benoît Canet
0 siblings, 0 replies; 25+ messages in thread
From: Benoît Canet @ 2014-02-10 13:40 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, Jeff Cody, qemu-devel, Stefan Hajnoczi
Le Saturday 08 Feb 2014 à 18:39:14 (+0100), Max Reitz a écrit :
> Add the bdrv_open() option BDRV_O_PROTOCOL which results in passing the
> call to bdrv_file_open(). Additionally, make bdrv_file_open() static and
> therefore bdrv_open() the only way to call it.
>
> Consequently, all existing calls to bdrv_file_open() have to be adjusted
> to use bdrv_open() with the BDRV_O_PROTOCOL flag instead.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block.c | 17 ++++++++++++-----
> block/cow.c | 6 +++---
> block/qcow.c | 6 +++---
> block/qcow2.c | 5 +++--
> block/qed.c | 5 +++--
> block/sheepdog.c | 8 +++++---
> block/vhdx.c | 5 +++--
> block/vmdk.c | 11 +++++++----
> include/block/block.h | 6 +++---
> qemu-io.c | 4 +++-
> 10 files changed, 45 insertions(+), 28 deletions(-)
>
> diff --git a/block.c b/block.c
> index 3a32c37..432d659 100644
> --- a/block.c
> +++ b/block.c
> @@ -953,9 +953,9 @@ free_and_fail:
> * after the call (even on failure), so if the caller intends to reuse the
> * dictionary, it needs to use QINCREF() before calling bdrv_file_open.
> */
> -int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> - const char *reference, QDict *options, int flags,
> - Error **errp)
> +static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> + const char *reference, QDict *options, int flags,
> + Error **errp)
> {
> BlockDriverState *bs = NULL;
> BlockDriver *drv;
> @@ -1201,8 +1201,9 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>
> ret = bdrv_open(pbs, filename, NULL, image_options, flags, NULL, errp);
> } else {
> - ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
> - errp);
> + *pbs = NULL;
> + ret = bdrv_open(pbs, filename, reference, image_options,
> + flags | BDRV_O_PROTOCOL, NULL, errp);
> }
>
> done:
> @@ -1234,6 +1235,12 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>
> assert(pbs);
>
> + if (flags & BDRV_O_PROTOCOL) {
> + assert(!drv);
> + return bdrv_file_open(pbs, filename, reference, options,
> + flags & ~BDRV_O_PROTOCOL, errp);
> + }
> +
> if (reference) {
> bool options_non_empty = options ? qdict_size(options) : false;
> QDECREF(options);
> diff --git a/block/cow.c b/block/cow.c
> index 7fc0b12..f0748a2 100644
> --- a/block/cow.c
> +++ b/block/cow.c
> @@ -332,7 +332,7 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
> const char *image_filename = NULL;
> Error *local_err = NULL;
> int ret;
> - BlockDriverState *cow_bs;
> + BlockDriverState *cow_bs = NULL;
>
> /* Read out options */
> while (options && options->name) {
> @@ -351,8 +351,8 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
> return ret;
> }
>
> - ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR,
> - &local_err);
> + ret = bdrv_open(&cow_bs, filename, NULL, NULL,
> + BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err);
> if (ret < 0) {
> qerror_report_err(local_err);
> error_free(local_err);
> diff --git a/block/qcow.c b/block/qcow.c
> index 948b0c5..4881d84 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -670,7 +670,7 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options,
> int flags = 0;
> Error *local_err = NULL;
> int ret;
> - BlockDriverState *qcow_bs;
> + BlockDriverState *qcow_bs = NULL;
>
> /* Read out options */
> while (options && options->name) {
> @@ -691,8 +691,8 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options,
> return ret;
> }
>
> - ret = bdrv_file_open(&qcow_bs, filename, NULL, NULL, BDRV_O_RDWR,
> - &local_err);
> + ret = bdrv_open(&qcow_bs, filename, NULL, NULL,
> + BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err);
> if (ret < 0) {
> qerror_report_err(local_err);
> error_free(local_err);
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 6996276..bd72172 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1481,7 +1481,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
> * 2 GB for 64k clusters, and we don't want to have a 2 GB initial file
> * size for any qcow2 image.
> */
> - BlockDriverState* bs;
> + BlockDriverState *bs = NULL;
> QCowHeader *header;
> uint8_t* refcount_table;
> Error *local_err = NULL;
> @@ -1493,7 +1493,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
> return ret;
> }
>
> - ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
> + ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
> + NULL, &local_err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> return ret;
> diff --git a/block/qed.c b/block/qed.c
> index b9ca7ac..33fbce1 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -571,8 +571,9 @@ static int qed_create(const char *filename, uint32_t cluster_size,
> return ret;
> }
>
> - ret = bdrv_file_open(&bs, filename, NULL, NULL,
> - BDRV_O_RDWR | BDRV_O_CACHE_WB, &local_err);
> + ret = bdrv_open(&bs, filename, NULL, NULL,
> + BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_PROTOCOL, NULL,
> + &local_err);
> if (ret < 0) {
> qerror_report_err(local_err);
> error_free(local_err);
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 672b9c9..6f2ec57 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1534,7 +1534,8 @@ static int sd_prealloc(const char *filename)
> Error *local_err = NULL;
> int ret;
>
> - ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
> + ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
> + NULL, &local_err);
> if (ret < 0) {
> qerror_report_err(local_err);
> error_free(local_err);
> @@ -1683,7 +1684,7 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
> }
>
> if (backing_file) {
> - BlockDriverState *bs;
> + BlockDriverState *bs = NULL;
> BDRVSheepdogState *base;
> BlockDriver *drv;
>
> @@ -1695,7 +1696,8 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
> goto out;
> }
>
> - ret = bdrv_file_open(&bs, backing_file, NULL, NULL, 0, &local_err);
> + ret = bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_PROTOCOL, NULL,
> + &local_err);
> if (ret < 0) {
> qerror_report_err(local_err);
> error_free(local_err);
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 55689cf..38d949c 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -1724,7 +1724,7 @@ static int vhdx_create(const char *filename, QEMUOptionParameter *options,
>
> gunichar2 *creator = NULL;
> glong creator_items;
> - BlockDriverState *bs;
> + BlockDriverState *bs = NULL;
> const char *type = NULL;
> VHDXImageType image_type;
> Error *local_err = NULL;
> @@ -1797,7 +1797,8 @@ static int vhdx_create(const char *filename, QEMUOptionParameter *options,
> goto exit;
> }
>
> - ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
> + ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
> + NULL, &local_err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> goto exit;
> diff --git a/block/vmdk.c b/block/vmdk.c
> index e007793..70bf011 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -776,8 +776,9 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>
> path_combine(extent_path, sizeof(extent_path),
> desc_file_path, fname);
> - ret = bdrv_file_open(&extent_file, extent_path, NULL, NULL,
> - bs->open_flags, errp);
> + extent_file = NULL;
> + ret = bdrv_open(&extent_file, extent_path, NULL, NULL,
> + bs->open_flags | BDRV_O_PROTOCOL, NULL, errp);
> if (ret) {
> return ret;
> }
> @@ -1493,7 +1494,8 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
> goto exit;
> }
>
> - ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
> + ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
> + NULL, &local_err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> goto exit;
> @@ -1831,7 +1833,8 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
> goto exit;
> }
> }
> - ret = bdrv_file_open(&new_bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
> + ret = bdrv_open(&new_bs, filename, NULL, NULL,
> + BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err);
> if (ret < 0) {
> error_setg_errno(errp, -ret, "Could not write description");
> goto exit;
> diff --git a/include/block/block.h b/include/block/block.h
> index a421041..bf78db5 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -102,6 +102,9 @@ typedef enum {
> #define BDRV_O_CHECK 0x1000 /* open solely for consistency check */
> #define BDRV_O_ALLOW_RDWR 0x2000 /* allow reopen to change from r/o to r/w */
> #define BDRV_O_UNMAP 0x4000 /* execute guest UNMAP/TRIM operations */
> +#define BDRV_O_PROTOCOL 0x8000 /* if no block driver is explicitly given:
> + select an appropriate protocol driver,
> + ignoring the format layer */
>
> #define BDRV_O_CACHE_MASK (BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)
>
> @@ -183,9 +186,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
> void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
> 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,
> - const char *reference, QDict *options, int flags,
> - Error **errp);
> int bdrv_open_image(BlockDriverState **pbs, const char *filename,
> QDict *options, const char *bdref_key, int flags,
> bool force_raw, bool allow_none, Error **errp);
> diff --git a/qemu-io.c b/qemu-io.c
> index 61d54c0..71d7806 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -59,7 +59,9 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
> }
>
> if (growable) {
> - if (bdrv_file_open(&qemuio_bs, name, NULL, opts, flags, &local_err)) {
> + if (bdrv_open(&qemuio_bs, name, NULL, opts, flags | BDRV_O_PROTOCOL,
> + NULL, &local_err))
> + {
> fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
> error_get_pretty(local_err));
> error_free(local_err);
> --
> 1.8.5.4
>
>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/8] block: Reuse reference handling from bdrv_open()
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 4/8] block: Reuse reference handling from bdrv_open() Max Reitz
@ 2014-02-10 13:42 ` Benoît Canet
0 siblings, 0 replies; 25+ messages in thread
From: Benoît Canet @ 2014-02-10 13:42 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, Jeff Cody, qemu-devel, Stefan Hajnoczi
Le Saturday 08 Feb 2014 à 18:39:15 (+0100), Max Reitz a écrit :
> Remove the reference parameter and the related handling code from
> bdrv_file_open(), since it exists in bdrv_open() now as well.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Benoit Canet <benoit@irqsave.net>
> ---
> block.c | 33 +++++++--------------------------
> 1 file changed, 7 insertions(+), 26 deletions(-)
>
> diff --git a/block.c b/block.c
> index 432d659..4c105bb 100644
> --- a/block.c
> +++ b/block.c
> @@ -954,8 +954,7 @@ free_and_fail:
> * dictionary, it needs to use QINCREF() before calling bdrv_file_open.
> */
> static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> - const char *reference, QDict *options, int flags,
> - Error **errp)
> + QDict *options, int flags, Error **errp)
> {
> BlockDriverState *bs = NULL;
> BlockDriver *drv;
> @@ -969,24 +968,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> options = qdict_new();
> }
>
> - if (reference) {
> - if (filename || qdict_size(options)) {
> - error_setg(errp, "Cannot reference an existing block device with "
> - "additional options or a new filename");
> - return -EINVAL;
> - }
> - QDECREF(options);
> -
> - bs = bdrv_find(reference);
> - if (!bs) {
> - error_setg(errp, "Cannot find block device '%s'", reference);
> - return -ENODEV;
> - }
> - bdrv_ref(bs);
> - *pbs = bs;
> - return 0;
> - }
> -
> bs = bdrv_new("");
> bs->options = options;
> options = qdict_clone_shallow(options);
> @@ -1235,12 +1216,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>
> assert(pbs);
>
> - if (flags & BDRV_O_PROTOCOL) {
> - assert(!drv);
> - return bdrv_file_open(pbs, filename, reference, options,
> - flags & ~BDRV_O_PROTOCOL, errp);
> - }
> -
> if (reference) {
> bool options_non_empty = options ? qdict_size(options) : false;
> QDECREF(options);
> @@ -1267,6 +1242,12 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
> return 0;
> }
>
> + if (flags & BDRV_O_PROTOCOL) {
> + assert(!drv);
> + return bdrv_file_open(pbs, filename, options, flags & ~BDRV_O_PROTOCOL,
> + errp);
> + }
> +
> if (*pbs) {
> bs = *pbs;
> } else {
> --
> 1.8.5.4
>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 5/8] block: Remove bdrv_new() from bdrv_file_open()
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 5/8] block: Remove bdrv_new() from bdrv_file_open() Max Reitz
@ 2014-02-10 13:49 ` Benoît Canet
0 siblings, 0 replies; 25+ messages in thread
From: Benoît Canet @ 2014-02-10 13:49 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, Jeff Cody, qemu-devel, Stefan Hajnoczi
Le Saturday 08 Feb 2014 à 18:39:16 (+0100), Max Reitz a écrit :
> Change bdrv_file_open() to take a simple pointer to an already existing
> BDS instead of an indirect one. The BDS will be created in bdrv_open()
> if necessary.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block.c | 24 +++++++++++++-----------
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/block.c b/block.c
> index 4c105bb..c57ad2c 100644
> --- a/block.c
> +++ b/block.c
> @@ -953,10 +953,9 @@ free_and_fail:
> * after the call (even on failure), so if the caller intends to reuse the
> * dictionary, it needs to use QINCREF() before calling bdrv_file_open.
> */
> -static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> +static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> QDict *options, int flags, Error **errp)
> {
> - BlockDriverState *bs = NULL;
> BlockDriver *drv;
> const char *drvname;
> bool allow_protocol_prefix = false;
> @@ -968,7 +967,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> options = qdict_new();
> }
>
> - bs = bdrv_new("");
> bs->options = options;
> options = qdict_clone_shallow(options);
>
> @@ -1042,7 +1040,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> QDECREF(options);
>
> bs->growable = 1;
> - *pbs = bs;
> return 0;
>
> fail:
> @@ -1050,7 +1047,6 @@ fail:
> if (!bs->drv) {
> QDECREF(bs->options);
> }
> - bdrv_unref(bs);
> return ret;
> }
>
> @@ -1242,18 +1238,24 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
> return 0;
> }
>
> - if (flags & BDRV_O_PROTOCOL) {
> - assert(!drv);
> - return bdrv_file_open(pbs, filename, options, flags & ~BDRV_O_PROTOCOL,
> - errp);
> - }
> -
> if (*pbs) {
> bs = *pbs;
> } else {
> bs = bdrv_new("");
> }
>
> + if (flags & BDRV_O_PROTOCOL) {
> + assert(!drv);
> + ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
> + errp);
> + if (ret && !*pbs) {
> + bdrv_unref(bs);
> + } else if (!ret) {
> + *pbs = bs;
> + }
> + return ret;
> + }
> +
> /* NULL means an empty set of options */
> if (options == NULL) {
> options = qdict_new();
> --
> 1.8.5.4
>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 7/8] block: Reuse success path from bdrv_open()
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 7/8] block: Reuse success path from bdrv_open() Max Reitz
@ 2014-02-10 14:56 ` Kevin Wolf
2014-02-12 0:26 ` Max Reitz
2014-02-10 16:28 ` Benoît Canet
1 sibling, 1 reply; 25+ messages in thread
From: Kevin Wolf @ 2014-02-10 14:56 UTC (permalink / raw)
To: Max Reitz; +Cc: Jeff Cody, qemu-devel, Stefan Hajnoczi, Benoît Canet
Am 08.02.2014 um 18:39 hat Max Reitz geschrieben:
> The fail and success paths of bdrv_file_open() may be further shortened
> by reusing code already existent in bdrv_open(). This includes
> bdrv_file_open() not taking the reference to options which allows the
> removal of QDECREF(options) in that function.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> @@ -1001,41 +1003,35 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>
> /* Parse the filename and open it */
> if (drv->bdrv_parse_filename && filename) {
> - drv->bdrv_parse_filename(filename, options, &local_err);
> + drv->bdrv_parse_filename(filename, *options, &local_err);
> if (error_is_set(&local_err)) {
> error_propagate(errp, local_err);
> ret = -EINVAL;
> goto fail;
> }
> - qdict_del(options, "filename");
> + qdict_del(*options, "filename");
> + } else if (drv->bdrv_needs_filename && !filename) {
> + error_setg(errp, "The '%s' block driver requires a file name",
> + drv->format_name);
> + ret = -EINVAL;
> + goto fail;
> }
How did this part end up in this patch? It doesn't look wrong, though I
think bdrv_open_common() should already catch it. In any case it's an
addition that the commit message didn't mention.
Kevin
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/8] block: Integrate bdrv_file_open() into bdrv_open()
2014-02-08 17:39 [Qemu-devel] [PATCH v2 0/8] block: Integrate bdrv_file_open() into bdrv_open() Max Reitz
` (7 preceding siblings ...)
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 8/8] block: Remove bdrv_open_image()'s force_raw option Max Reitz
@ 2014-02-10 15:01 ` Kevin Wolf
8 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2014-02-10 15:01 UTC (permalink / raw)
To: Max Reitz; +Cc: Jeff Cody, qemu-devel, Stefan Hajnoczi, Benoît Canet
Am 08.02.2014 um 18:39 hat Max Reitz geschrieben:
> bdrv_file_open() is now nearly a subset of bdrv_open(), except for the
> fact that bdrv_file_open() is for protocols and bdrv_open() for block
> drivers. It is possible to use bdrv_file_open() with a block driver, but
> in that case that block driver must be explicitly specified.
>
> Due to these great similarities, bdrv_file_open() can be integrated and
> made a special case of bdrv_open(). If the flag BDRV_O_PROTOCOL is
> specified, bdrv_open() will now do what bdrv_file_open() used to do:
> Auto-detecting a protocol instead of a block driver.
>
> This series implements this and changes all calls to bdrv_file_open() to
> bdrv_open() calls with BDRV_O_PROTOCOL specified.
>
> Note that this flag cannot be discerned automatically since it is
> impossible for bdrv_open() to know by itself whether a given file should
> be opened with or without the format layer involved: Both are valid
> alternatives. Therefore, it still has to be specified by the user.
I had two or three comments that can be addressed in a follow-up.
Series: Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 6/8] block: Handle bs->options in bdrv_open() only
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 6/8] block: Handle bs->options in bdrv_open() only Max Reitz
@ 2014-02-10 16:23 ` Benoît Canet
2014-02-10 16:23 ` Benoît Canet
1 sibling, 0 replies; 25+ messages in thread
From: Benoît Canet @ 2014-02-10 16:23 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, Jeff Cody, qemu-devel, Stefan Hajnoczi
Le Saturday 08 Feb 2014 à 18:39:17 (+0100), Max Reitz a écrit :
> The fail paths of bdrv_file_open() and bdrv_open() naturally exhibit
> similarities, thus it is possible to reuse the one from bdrv_open() and
> shorten the one in bdrv_file_open() accordingly.
>
> Also, setting bs->options in bdrv_file_open() is not necessary if it is
> already done in bdrv_open().
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block.c | 38 +++++++++++++++-----------------------
> 1 file changed, 15 insertions(+), 23 deletions(-)
>
> diff --git a/block.c b/block.c
> index c57ad2c..1fcd7e0 100644
> --- a/block.c
> +++ b/block.c
> @@ -962,14 +962,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> Error *local_err = NULL;
> int ret;
>
> - /* NULL means an empty set of options */
> - if (options == NULL) {
> - options = qdict_new();
> - }
> -
> - bs->options = options;
> - options = qdict_clone_shallow(options);
> -
> /* Fetch the file name from the options QDict if necessary */
> if (!filename) {
> filename = qdict_get_try_str(options, "filename");
> @@ -1044,9 +1036,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>
> fail:
> QDECREF(options);
> - if (!bs->drv) {
> - QDECREF(bs->options);
> - }
> return ret;
> }
>
> @@ -1244,18 +1233,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
> bs = bdrv_new("");
> }
>
> - if (flags & BDRV_O_PROTOCOL) {
> - assert(!drv);
> - ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
> - errp);
> - if (ret && !*pbs) {
> - bdrv_unref(bs);
> - } else if (!ret) {
> - *pbs = bs;
> - }
> - return ret;
> - }
> -
> /* NULL means an empty set of options */
> if (options == NULL) {
> options = qdict_new();
> @@ -1263,6 +1240,21 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
> bs->options = options;
> options = qdict_clone_shallow(options);
>
> + if (flags & BDRV_O_PROTOCOL) {
> + assert(!drv);
> + ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
> + &local_err);
> + options = NULL;
> + if (!ret) {
> + *pbs = bs;
> + return 0;
> + } else if (bs->drv) {
> + goto close_and_fail;
> + } else {
> + goto fail;
> + }
> + }
> +
> /* For snapshot=on, create a temporary qcow2 overlay */
> if (flags & BDRV_O_SNAPSHOT) {
> BlockDriverState *bs1;
> --
> 1.8.5.4
>
>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 6/8] block: Handle bs->options in bdrv_open() only
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 6/8] block: Handle bs->options in bdrv_open() only Max Reitz
2014-02-10 16:23 ` Benoît Canet
@ 2014-02-10 16:23 ` Benoît Canet
1 sibling, 0 replies; 25+ messages in thread
From: Benoît Canet @ 2014-02-10 16:23 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, Jeff Cody, qemu-devel, Stefan Hajnoczi
Le Saturday 08 Feb 2014 à 18:39:17 (+0100), Max Reitz a écrit :
> The fail paths of bdrv_file_open() and bdrv_open() naturally exhibit
> similarities, thus it is possible to reuse the one from bdrv_open() and
> shorten the one in bdrv_file_open() accordingly.
>
> Also, setting bs->options in bdrv_file_open() is not necessary if it is
> already done in bdrv_open().
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block.c | 38 +++++++++++++++-----------------------
> 1 file changed, 15 insertions(+), 23 deletions(-)
>
> diff --git a/block.c b/block.c
> index c57ad2c..1fcd7e0 100644
> --- a/block.c
> +++ b/block.c
> @@ -962,14 +962,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> Error *local_err = NULL;
> int ret;
>
> - /* NULL means an empty set of options */
> - if (options == NULL) {
> - options = qdict_new();
> - }
> -
> - bs->options = options;
> - options = qdict_clone_shallow(options);
> -
> /* Fetch the file name from the options QDict if necessary */
> if (!filename) {
> filename = qdict_get_try_str(options, "filename");
> @@ -1044,9 +1036,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>
> fail:
> QDECREF(options);
> - if (!bs->drv) {
> - QDECREF(bs->options);
> - }
> return ret;
> }
>
> @@ -1244,18 +1233,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
> bs = bdrv_new("");
> }
>
> - if (flags & BDRV_O_PROTOCOL) {
> - assert(!drv);
> - ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
> - errp);
> - if (ret && !*pbs) {
> - bdrv_unref(bs);
> - } else if (!ret) {
> - *pbs = bs;
> - }
> - return ret;
> - }
> -
> /* NULL means an empty set of options */
> if (options == NULL) {
> options = qdict_new();
> @@ -1263,6 +1240,21 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
> bs->options = options;
> options = qdict_clone_shallow(options);
>
> + if (flags & BDRV_O_PROTOCOL) {
> + assert(!drv);
> + ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
> + &local_err);
> + options = NULL;
> + if (!ret) {
> + *pbs = bs;
> + return 0;
> + } else if (bs->drv) {
> + goto close_and_fail;
> + } else {
> + goto fail;
> + }
> + }
> +
> /* For snapshot=on, create a temporary qcow2 overlay */
> if (flags & BDRV_O_SNAPSHOT) {
> BlockDriverState *bs1;
> --
> 1.8.5.4
>
>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 7/8] block: Reuse success path from bdrv_open()
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 7/8] block: Reuse success path from bdrv_open() Max Reitz
2014-02-10 14:56 ` Kevin Wolf
@ 2014-02-10 16:28 ` Benoît Canet
1 sibling, 0 replies; 25+ messages in thread
From: Benoît Canet @ 2014-02-10 16:28 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, Jeff Cody, qemu-devel, Stefan Hajnoczi
Le Saturday 08 Feb 2014 à 18:39:18 (+0100), Max Reitz a écrit :
> The fail and success paths of bdrv_file_open() may be further shortened
> by reusing code already existent in bdrv_open(). This includes
> bdrv_file_open() not taking the reference to options which allows the
> removal of QDECREF(options) in that function.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block.c | 68 ++++++++++++++++++++++++++++++++---------------------------------
> 1 file changed, 34 insertions(+), 34 deletions(-)
>
> diff --git a/block.c b/block.c
> index 1fcd7e0..22e5a44 100644
> --- a/block.c
> +++ b/block.c
> @@ -948,13 +948,15 @@ free_and_fail:
> /*
> * Opens a file using a protocol (file, host_device, nbd, ...)
> *
> - * 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 belongs to the block layer
> - * after the call (even on failure), so if the caller intends to reuse the
> - * dictionary, it needs to use QINCREF() before calling bdrv_file_open.
> + * options is an indirect pointer to a QDict of options to pass to the block
> + * drivers, or pointer to NULL for an empty set of options. If this function
> + * takes ownership of the QDict reference, it will set *options to NULL;
> + * otherwise, it will contain unused/unrecognized options after this function
> + * returns. Then, the caller is responsible for freeing it. If it intends to
> + * reuse the QDict, QINCREF() should be called beforehand.
> */
> static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> - QDict *options, int flags, Error **errp)
> + QDict **options, int flags, Error **errp)
> {
> BlockDriver *drv;
> const char *drvname;
> @@ -964,9 +966,9 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>
> /* Fetch the file name from the options QDict if necessary */
> if (!filename) {
> - filename = qdict_get_try_str(options, "filename");
> - } else if (filename && !qdict_haskey(options, "filename")) {
> - qdict_put(options, "filename", qstring_from_str(filename));
> + filename = qdict_get_try_str(*options, "filename");
> + } else if (filename && !qdict_haskey(*options, "filename")) {
> + qdict_put(*options, "filename", qstring_from_str(filename));
> allow_protocol_prefix = true;
> } else {
> error_setg(errp, "Can't specify 'file' and 'filename' options at the "
> @@ -976,13 +978,13 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> }
>
> /* Find the right block driver */
> - drvname = qdict_get_try_str(options, "driver");
> + drvname = qdict_get_try_str(*options, "driver");
> if (drvname) {
> drv = bdrv_find_format(drvname);
> if (!drv) {
> error_setg(errp, "Unknown driver '%s'", drvname);
> }
> - qdict_del(options, "driver");
> + qdict_del(*options, "driver");
> } else if (filename) {
> drv = bdrv_find_protocol(filename, allow_protocol_prefix);
> if (!drv) {
> @@ -1001,41 +1003,35 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>
> /* Parse the filename and open it */
> if (drv->bdrv_parse_filename && filename) {
> - drv->bdrv_parse_filename(filename, options, &local_err);
> + drv->bdrv_parse_filename(filename, *options, &local_err);
> if (error_is_set(&local_err)) {
> error_propagate(errp, local_err);
> ret = -EINVAL;
> goto fail;
> }
> - qdict_del(options, "filename");
> + qdict_del(*options, "filename");
> + } else if (drv->bdrv_needs_filename && !filename) {
> + error_setg(errp, "The '%s' block driver requires a file name",
> + drv->format_name);
> + ret = -EINVAL;
> + goto fail;
> }
>
> if (!drv->bdrv_file_open) {
> - ret = bdrv_open(&bs, filename, NULL, options, flags, drv, &local_err);
> - options = NULL;
> + ret = bdrv_open(&bs, filename, NULL, *options, flags, drv, &local_err);
> + *options = NULL;
> } else {
> - ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
> + ret = bdrv_open_common(bs, NULL, *options, flags, drv, &local_err);
> }
> if (ret < 0) {
> error_propagate(errp, local_err);
> goto fail;
> }
>
> - /* Check if any unknown options were used */
> - if (options && (qdict_size(options) != 0)) {
> - const QDictEntry *entry = qdict_first(options);
> - error_setg(errp, "Block protocol '%s' doesn't support the option '%s'",
> - drv->format_name, entry->key);
> - ret = -EINVAL;
> - goto fail;
> - }
> - QDECREF(options);
> -
> bs->growable = 1;
> return 0;
>
> fail:
> - QDECREF(options);
> return ret;
> }
>
> @@ -1242,12 +1238,10 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>
> if (flags & BDRV_O_PROTOCOL) {
> assert(!drv);
> - ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
> + ret = bdrv_file_open(bs, filename, &options, flags & ~BDRV_O_PROTOCOL,
> &local_err);
> - options = NULL;
> if (!ret) {
> - *pbs = bs;
> - return 0;
> + goto done;
> } else if (bs->drv) {
> goto close_and_fail;
> } else {
> @@ -1384,12 +1378,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
> }
> }
>
> +done:
> /* Check if any unknown options were used */
> - if (qdict_size(options) != 0) {
> + if (options && (qdict_size(options) != 0)) {
> const QDictEntry *entry = qdict_first(options);
> - error_setg(errp, "Block format '%s' used by device '%s' doesn't "
> - "support the option '%s'", drv->format_name, bs->device_name,
> - entry->key);
> + if (flags & BDRV_O_PROTOCOL) {
> + error_setg(errp, "Block protocol '%s' doesn't support the option "
> + "'%s'", drv->format_name, entry->key);
> + } else {
> + error_setg(errp, "Block format '%s' used by device '%s' doesn't "
> + "support the option '%s'", drv->format_name,
> + bs->device_name, entry->key);
> + }
>
> ret = -EINVAL;
> goto close_and_fail;
> --
> 1.8.5.4
>
>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 8/8] block: Remove bdrv_open_image()'s force_raw option
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 8/8] block: Remove bdrv_open_image()'s force_raw option Max Reitz
@ 2014-02-10 16:31 ` Benoît Canet
0 siblings, 0 replies; 25+ messages in thread
From: Benoît Canet @ 2014-02-10 16:31 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, Jeff Cody, qemu-devel, Stefan Hajnoczi
Le Saturday 08 Feb 2014 à 18:39:19 (+0100), Max Reitz a écrit :
> This option is now unnecessary since specifying BDRV_O_PROTOCOL as flag
> will do exactly the same.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block.c | 28 ++++------------------------
> block/blkdebug.c | 2 +-
> block/blkverify.c | 4 ++--
> include/block/block.h | 2 +-
> 4 files changed, 8 insertions(+), 28 deletions(-)
>
> diff --git a/block.c b/block.c
> index 22e5a44..a1630e9 100644
> --- a/block.c
> +++ b/block.c
> @@ -1107,10 +1107,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
> * Opens a disk image whose options are given as BlockdevRef in another block
> * device's options.
> *
> - * If force_raw is true, bdrv_file_open() will be used, thereby preventing any
> - * image format auto-detection. If it is false and a filename is given,
> - * bdrv_open() will be used for auto-detection.
> - *
> * If allow_none is true, no image will be opened if filename is false and no
> * BlockdevRef is given. *pbs will remain unchanged and 0 will be returned.
> *
> @@ -1127,7 +1123,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
> */
> int bdrv_open_image(BlockDriverState **pbs, const char *filename,
> QDict *options, const char *bdref_key, int flags,
> - bool force_raw, bool allow_none, Error **errp)
> + bool allow_none, Error **errp)
> {
> QDict *image_options;
> int ret;
> @@ -1150,23 +1146,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
> goto done;
> }
>
> - if (filename && !force_raw) {
> - /* If a filename is given and the block driver should be detected
> - automatically (instead of using none), use bdrv_open() in order to do
> - that auto-detection. */
> - if (reference) {
> - error_setg(errp, "Cannot reference an existing block device while "
> - "giving a filename");
> - ret = -EINVAL;
> - goto done;
> - }
> -
> - ret = bdrv_open(pbs, filename, NULL, image_options, flags, NULL, errp);
> - } else {
> - *pbs = NULL;
> - ret = bdrv_open(pbs, filename, reference, image_options,
> - flags | BDRV_O_PROTOCOL, NULL, errp);
> - }
> + ret = bdrv_open(pbs, filename, reference, image_options, flags, NULL, errp);
>
> done:
> qdict_del(options, bdref_key);
> @@ -1324,8 +1304,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>
> assert(file == NULL);
> ret = bdrv_open_image(&file, filename, options, "file",
> - bdrv_open_flags(bs, flags | BDRV_O_UNMAP), true, true,
> - &local_err);
> + bdrv_open_flags(bs, flags | BDRV_O_UNMAP) |
> + BDRV_O_PROTOCOL, true, &local_err);
> if (ret < 0) {
> goto fail;
> }
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 053fa4c..6707f49 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -412,7 +412,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
> /* Open the backing file */
> assert(bs->file == NULL);
> ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-image"), options, "image",
> - flags, true, false, &local_err);
> + flags | BDRV_O_PROTOCOL, false, &local_err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> goto out;
> diff --git a/block/blkverify.c b/block/blkverify.c
> index 86585e7..b57da59 100644
> --- a/block/blkverify.c
> +++ b/block/blkverify.c
> @@ -137,7 +137,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
> /* Open the raw file */
> assert(bs->file == NULL);
> ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-raw"), options,
> - "raw", flags, true, false, &local_err);
> + "raw", flags | BDRV_O_PROTOCOL, false, &local_err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> goto fail;
> @@ -146,7 +146,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
> /* Open the test file */
> assert(s->test_file == NULL);
> ret = bdrv_open_image(&s->test_file, qemu_opt_get(opts, "x-image"), options,
> - "test", flags, false, false, &local_err);
> + "test", flags, false, &local_err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> s->test_file = NULL;
> diff --git a/include/block/block.h b/include/block/block.h
> index bf78db5..780f48b 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -188,7 +188,7 @@ int bdrv_parse_cache_flags(const char *mode, int *flags);
> int bdrv_parse_discard_flags(const char *mode, int *flags);
> int bdrv_open_image(BlockDriverState **pbs, const char *filename,
> QDict *options, const char *bdref_key, int flags,
> - bool force_raw, bool allow_none, Error **errp);
> + bool allow_none, Error **errp);
> int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
> int bdrv_open(BlockDriverState **pbs, const char *filename,
> const char *reference, QDict *options, int flags,
> --
> 1.8.5.4
>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/8] block: Change BDS parameter of bdrv_open() to **
2014-02-10 12:42 ` Kevin Wolf
@ 2014-02-12 0:10 ` Max Reitz
0 siblings, 0 replies; 25+ messages in thread
From: Max Reitz @ 2014-02-12 0:10 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Jeff Cody, qemu-devel, Stefan Hajnoczi, Benoît Canet
On 10.02.2014 13:42, Kevin Wolf wrote:
> Am 08.02.2014 um 18:39 hat Max Reitz geschrieben:
>> Make bdrv_open() take a pointer to a BDS pointer, similarly to
>> bdrv_file_open(). If a pointer to a NULL pointer is given, bdrv_open()
>> will create a new BDS with an empty name; if the BDS pointer is not
>> NULL, that existing BDS will be reused (in the same way as bdrv_open()
>> already did).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> block.c | 64 +++++++++++++++++++++++++++++++--------------------
>> block/blkdebug.c | 1 +
>> block/blkverify.c | 2 ++
>> block/qcow2.c | 14 +++++++----
>> block/vmdk.c | 5 ++--
>> block/vvfat.c | 6 ++---
>> blockdev.c | 20 ++++++++--------
>> hw/block/xen_disk.c | 2 +-
>> include/block/block.h | 2 +-
>> qemu-img.c | 10 ++++----
>> qemu-io.c | 2 +-
>> qemu-nbd.c | 2 +-
>> 12 files changed, 72 insertions(+), 58 deletions(-)
>> @@ -1160,6 +1158,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>> * BlockdevRef.
>> *
>> * The BlockdevRef will be removed from the options QDict.
>> + *
>> + * As with bdrv_open(), if *pbs is NULL, a new BDS will be created with a
>> + * pointer to it stored there. If it is not NULL, the referenced BDS will
>> + * be reused.
>> */
>> int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>> QDict *options, const char *bdref_key, int flags,
> There are no callers that make use of *pbs != NULL. Are you planning to
> add such users? Otherwise, we could just assert() it here instead of
> documenting behaviour that is never used.
No, currently, there aren't. Since we're planning to eventually adjust
everything to give a pointer to a NULL pointer to these functions (i.e.,
nobody except bdrv_open() uses bdrv_new()), adding an assert() in order
to prevent anyone from "exploiting" this in a way which would
technically be fine but actually not what we want (i.e., reusing an
already existing BDS), is fine with me. I'll add it and adjust the comment.
Max
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/8] block: Change BDS parameter of bdrv_open() to **
2014-02-10 13:17 ` Benoît Canet
@ 2014-02-12 0:15 ` Max Reitz
0 siblings, 0 replies; 25+ messages in thread
From: Max Reitz @ 2014-02-12 0:15 UTC (permalink / raw)
To: Benoît Canet; +Cc: Kevin Wolf, Jeff Cody, qemu-devel, Stefan Hajnoczi
On 10.02.2014 14:17, Benoît Canet wrote:
> Le Saturday 08 Feb 2014 à 18:39:12 (+0100), Max Reitz a écrit :
>> Make bdrv_open() take a pointer to a BDS pointer, similarly to
>> bdrv_file_open(). If a pointer to a NULL pointer is given, bdrv_open()
>> will create a new BDS with an empty name; if the BDS pointer is not
>> NULL, that existing BDS will be reused (in the same way as bdrv_open()
>> already did).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> block.c | 64 +++++++++++++++++++++++++++++++--------------------
>> block/blkdebug.c | 1 +
>> block/blkverify.c | 2 ++
>> block/qcow2.c | 14 +++++++----
>> block/vmdk.c | 5 ++--
>> block/vvfat.c | 6 ++---
>> blockdev.c | 20 ++++++++--------
>> hw/block/xen_disk.c | 2 +-
>> include/block/block.h | 2 +-
>> qemu-img.c | 10 ++++----
>> qemu-io.c | 2 +-
>> qemu-nbd.c | 2 +-
>> 12 files changed, 72 insertions(+), 58 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 636aa11..40a585a 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1040,7 +1040,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>> }
>>
>> if (!drv->bdrv_file_open) {
>> - ret = bdrv_open(bs, filename, options, flags, drv, &local_err);
>> + ret = bdrv_open(&bs, filename, options, flags, drv, &local_err);
>> options = NULL;
>> } else {
>> ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
>> @@ -1109,8 +1109,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>> sizeof(backing_filename));
>> }
>>
>> - bs->backing_hd = bdrv_new("");
>> -
>> if (bs->backing_format[0] != '\0') {
>> back_drv = bdrv_find_format(bs->backing_format);
>> }
>> @@ -1119,11 +1117,11 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>> back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT |
>> BDRV_O_COPY_ON_READ);
>>
>> - ret = bdrv_open(bs->backing_hd,
>> + assert(bs->backing_hd == NULL);
>> + 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",
>> @@ -1160,6 +1158,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>> * BlockdevRef.
>> *
>> * The BlockdevRef will be removed from the options QDict.
>> + *
>> + * As with bdrv_open(), if *pbs is NULL, a new BDS will be created with a
>> + * pointer to it stored there. If it is not NULL, the referenced BDS will
>> + * be reused.
>> */
>> int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>> QDict *options, const char *bdref_key, int flags,
>> @@ -1190,8 +1192,6 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>> /* If a filename is given and the block driver should be detected
>> automatically (instead of using none), use bdrv_open() in order to do
>> that auto-detection. */
>> - BlockDriverState *bs;
>> -
>> if (reference) {
>> error_setg(errp, "Cannot reference an existing block device while "
>> "giving a filename");
>> @@ -1199,13 +1199,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>> goto done;
>> }
>>
>> - bs = bdrv_new("");
>> - ret = bdrv_open(bs, filename, image_options, flags, NULL, errp);
>> - if (ret < 0) {
>> - bdrv_unref(bs);
>> - } else {
>> - *pbs = bs;
>> - }
>> + ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp);
>> } else {
>> ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
>> errp);
>> @@ -1223,22 +1217,32 @@ done:
>> * empty set of options. The reference to the QDict belongs to the block layer
>> * after the call (even on failure), so if the caller intends to reuse the
>> * dictionary, it needs to use QINCREF() before calling bdrv_open.
>> + *
>> + * If *pbs is NULL, a new BDS will be created with a pointer to it stored there.
>> + * If it is not NULL, the referenced BDS will be reused.
>> */
>> -int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>> +int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
>> int flags, BlockDriver *drv, Error **errp)
>> {
>> int ret;
>> /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
>> char tmp_filename[PATH_MAX + 1];
>> - BlockDriverState *file = NULL;
>> + BlockDriverState *file = NULL, *bs;
>> const char *drvname;
>> Error *local_err = NULL;
>>
>> + assert(pbs);
>> +
>> + if (*pbs) {
>> + bs = *pbs;
>> + } else {
>> + bs = bdrv_new("");
>> + }
>> +
>> /* NULL means an empty set of options */
>> if (options == NULL) {
>> options = qdict_new();
>> }
>> -
> nitpicking: spurious line removal.
Right, I didn't notice because I mostly compared v2 against v1. :-)
>> bs->options = options;
>> options = qdict_clone_shallow(options);
>>
>> @@ -1254,12 +1258,11 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>> instead of opening 'filename' directly */
>>
>> /* Get the required size from the image */
>> - bs1 = bdrv_new("");
>> QINCREF(options);
>> - ret = bdrv_open(bs1, filename, options, BDRV_O_NO_BACKING,
>> + bs1 = NULL;
>> + ret = bdrv_open(&bs1, filename, options, BDRV_O_NO_BACKING,
>> drv, &local_err);
>> if (ret < 0) {
>> - bdrv_unref(bs1);
>> goto fail;
>> }
>> total_size = bdrv_getlength(bs1) & BDRV_SECTOR_MASK;
>> @@ -1316,6 +1319,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>> flags |= BDRV_O_ALLOW_RDWR;
>> }
>>
>> + assert(file == NULL);
>> ret = bdrv_open_image(&file, filename, options, "file",
>> bdrv_open_flags(bs, flags | BDRV_O_UNMAP), true, true,
>> &local_err);
> is_temporary can already be set to one the goto after the bdrv_open_image should
> be unlink_and_fail yet I don't know if the fix belong to this patch.
Oh, right, good catch. I don't think this belongs to this series,
though. Do you want to fix this or shall I?
Max
>> @@ -1387,6 +1391,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>> bdrv_dev_change_media_cb(bs, true);
>> }
>>
>> + *pbs = bs;
>> return 0;
>>
>> unlink_and_fail:
>> @@ -1400,13 +1405,24 @@ fail:
>> QDECREF(bs->options);
>> QDECREF(options);
>> bs->options = NULL;
>> + if (!*pbs) {
>> + /* If *pbs is NULL, a new BDS has been created in this function and
>> + needs to be freed now. Otherwise, it does not need to be closed,
>> + since it has not really been opened yet. */
>> + bdrv_unref(bs);
>> + }
>> if (error_is_set(&local_err)) {
>> error_propagate(errp, local_err);
>> }
>> return ret;
>>
>> close_and_fail:
>> - bdrv_close(bs);
>> + /* See fail path, but now the BDS has to be always closed */
>> + if (*pbs) {
>> + bdrv_close(bs);
>> + } else {
>> + bdrv_unref(bs);
>> + }
>> QDECREF(options);
>> if (error_is_set(&local_err)) {
>> error_propagate(errp, local_err);
>> @@ -5288,9 +5304,8 @@ void bdrv_img_create(const char *filename, const char *fmt,
>> back_flags =
>> flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
>>
>> - bs = bdrv_new("");
>> -
>> - ret = bdrv_open(bs, backing_file->value.s, NULL, back_flags,
>> + bs = NULL;
>> + ret = bdrv_open(&bs, backing_file->value.s, NULL, back_flags,
>> backing_drv, &local_err);
>> if (ret < 0) {
>> error_setg_errno(errp, -ret, "Could not open '%s': %s",
>> @@ -5298,7 +5313,6 @@ void bdrv_img_create(const char *filename, const char *fmt,
>> error_get_pretty(local_err));
>> error_free(local_err);
>> local_err = NULL;
>> - bdrv_unref(bs);
>> goto out;
>> }
>> bdrv_get_geometry(bs, &size);
>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>> index 8eb0db0..053fa4c 100644
>> --- a/block/blkdebug.c
>> +++ b/block/blkdebug.c
>> @@ -410,6 +410,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
>> s->state = 1;
>>
>> /* Open the backing file */
>> + assert(bs->file == NULL);
>> ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-image"), options, "image",
>> flags, true, false, &local_err);
>> if (ret < 0) {
>> diff --git a/block/blkverify.c b/block/blkverify.c
>> index cfcbcf4..86585e7 100644
>> --- a/block/blkverify.c
>> +++ b/block/blkverify.c
>> @@ -135,6 +135,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
>> }
>>
>> /* Open the raw file */
>> + assert(bs->file == NULL);
>> ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-raw"), options,
>> "raw", flags, true, false, &local_err);
>> if (ret < 0) {
>> @@ -143,6 +144,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
>> }
>>
>> /* Open the test file */
>> + assert(s->test_file == NULL);
>> ret = bdrv_open_image(&s->test_file, qemu_opt_get(opts, "x-image"), options,
>> "test", flags, false, false, &local_err);
>> if (ret < 0) {
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 0b4335c..c8e8ba7 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1543,7 +1543,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>> goto out;
>> }
>>
>> - bdrv_close(bs);
>> + bdrv_unref(bs);
>> + bs = NULL;
>>
>> /*
>> * And now open the image and make it consistent first (i.e. increase the
>> @@ -1552,7 +1553,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>> */
>> BlockDriver* drv = bdrv_find_format("qcow2");
>> assert(drv != NULL);
>> - ret = bdrv_open(bs, filename, NULL,
>> + ret = bdrv_open(&bs, filename, NULL,
>> BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, drv, &local_err);
>> if (ret < 0) {
>> error_propagate(errp, local_err);
>> @@ -1599,10 +1600,11 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>> }
>> }
>>
>> - bdrv_close(bs);
>> + bdrv_unref(bs);
>> + bs = NULL;
>>
>> /* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning */
>> - ret = bdrv_open(bs, filename, NULL,
>> + ret = bdrv_open(&bs, filename, NULL,
>> BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING,
>> drv, &local_err);
>> if (error_is_set(&local_err)) {
>> @@ -1612,7 +1614,9 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>>
>> ret = 0;
>> out:
>> - bdrv_unref(bs);
>> + if (bs) {
>> + bdrv_unref(bs);
>> + }
>> return ret;
>> }
>>
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index e809e2e..52cd12a 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -1755,10 +1755,9 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
>> goto exit;
>> }
>> if (backing_file) {
>> - BlockDriverState *bs = bdrv_new("");
>> - ret = bdrv_open(bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp);
>> + BlockDriverState *bs = NULL;
>> + ret = bdrv_open(&bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp);
>> if (ret != 0) {
>> - bdrv_unref(bs);
>> goto exit;
>> }
>> if (strcmp(bs->drv->format_name, "vmdk")) {
>> diff --git a/block/vvfat.c b/block/vvfat.c
>> index 664941c..ae7bc6f 100644
>> --- a/block/vvfat.c
>> +++ b/block/vvfat.c
>> @@ -2936,15 +2936,13 @@ static int enable_write_target(BDRVVVFATState *s)
>> goto err;
>> }
>>
>> - s->qcow = bdrv_new("");
>> -
>> - ret = bdrv_open(s->qcow, s->qcow_filename, NULL,
>> + s->qcow = NULL;
>> + ret = bdrv_open(&s->qcow, s->qcow_filename, NULL,
>> BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow,
>> &local_err);
>> if (ret < 0) {
>> qerror_report_err(local_err);
>> error_free(local_err);
>> - bdrv_unref(s->qcow);
>> goto err;
>> }
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 36ceece..6dd351c 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -510,7 +510,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>> bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>>
>> QINCREF(bs_opts);
>> - ret = bdrv_open(dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
>> + ret = bdrv_open(&dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
>>
>> if (ret < 0) {
>> error_setg(errp, "could not open disk image %s: %s",
>> @@ -1301,12 +1301,12 @@ static void external_snapshot_prepare(BlkTransactionState *common,
>> qstring_from_str(snapshot_node_name));
>> }
>>
>> - /* We will manually add the backing_hd field to the bs later */
>> - state->new_bs = bdrv_new("");
>> /* TODO Inherit bs->options or only take explicit options with an
>> * extended QMP command? */
>> - ret = bdrv_open(state->new_bs, new_image_file, options,
>> + assert(state->new_bs == NULL);
>> + ret = bdrv_open(&state->new_bs, new_image_file, options,
>> flags | BDRV_O_NO_BACKING, drv, &local_err);
>> + /* We will manually add the backing_hd field to the bs later */
>> if (ret != 0) {
>> error_propagate(errp, local_err);
>> }
>> @@ -1555,7 +1555,7 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
>> Error *local_err = NULL;
>> int ret;
>>
>> - ret = bdrv_open(bs, filename, NULL, bdrv_flags, drv, &local_err);
>> + ret = bdrv_open(&bs, filename, NULL, bdrv_flags, drv, &local_err);
>> if (ret < 0) {
>> error_propagate(errp, local_err);
>> return;
>> @@ -1991,10 +1991,9 @@ void qmp_drive_backup(const char *device, const char *target,
>> return;
>> }
>>
>> - target_bs = bdrv_new("");
>> - ret = bdrv_open(target_bs, target, NULL, flags, drv, &local_err);
>> + target_bs = NULL;
>> + ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err);
>> if (ret < 0) {
>> - bdrv_unref(target_bs);
>> error_propagate(errp, local_err);
>> return;
>> }
>> @@ -2135,11 +2134,10 @@ void qmp_drive_mirror(const char *device, const char *target,
>> /* Mirroring takes care of copy-on-write using the source's backing
>> * file.
>> */
>> - target_bs = bdrv_new("");
>> - ret = bdrv_open(target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
>> + target_bs = NULL;
>> + ret = bdrv_open(&target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
>> &local_err);
>> if (ret < 0) {
>> - bdrv_unref(target_bs);
>> error_propagate(errp, local_err);
>> return;
>> }
>> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
>> index 098f6c6..14e6d0b 100644
>> --- a/hw/block/xen_disk.c
>> +++ b/hw/block/xen_disk.c
>> @@ -813,7 +813,7 @@ static int blk_connect(struct XenDevice *xendev)
>> Error *local_err = NULL;
>> BlockDriver *drv = bdrv_find_whitelisted_format(blkdev->fileproto,
>> readonly);
>> - if (bdrv_open(blkdev->bs,
>> + if (bdrv_open(&blkdev->bs,
>> blkdev->filename, NULL, qflags, drv, &local_err) != 0)
>> {
>> xen_be_printf(&blkdev->xendev, 0, "error: %s\n",
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 963a61f..980869d 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -190,7 +190,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>> QDict *options, const char *bdref_key, int flags,
>> bool force_raw, bool allow_none, Error **errp);
>> int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
>> -int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>> +int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
>> int flags, BlockDriver *drv, Error **errp);
>> BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
>> BlockDriverState *bs, int flags);
>> diff --git a/qemu-img.c b/qemu-img.c
>> index c989850..897aa56 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -289,7 +289,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
>> drv = NULL;
>> }
>>
>> - ret = bdrv_open(bs, filename, NULL, flags, drv, &local_err);
>> + ret = bdrv_open(&bs, filename, NULL, flags, drv, &local_err);
>> if (ret < 0) {
>> error_report("Could not open '%s': %s", filename,
>> error_get_pretty(local_err));
>> @@ -310,9 +310,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
>> }
>> return bs;
>> fail:
>> - if (bs) {
>> - bdrv_unref(bs);
>> - }
>> + bdrv_unref(bs);
>> return NULL;
>> }
>>
>> @@ -2314,7 +2312,7 @@ static int img_rebase(int argc, char **argv)
>>
>> bs_old_backing = bdrv_new("old_backing");
>> bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
>> - ret = bdrv_open(bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
>> + ret = bdrv_open(&bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
>> old_backing_drv, &local_err);
>> if (ret) {
>> error_report("Could not open old backing file '%s': %s",
>> @@ -2324,7 +2322,7 @@ static int img_rebase(int argc, char **argv)
>> }
>> if (out_baseimg[0]) {
>> bs_new_backing = bdrv_new("new_backing");
>> - ret = bdrv_open(bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
>> + ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
>> new_backing_drv, &local_err);
>> if (ret) {
>> error_report("Could not open new backing file '%s': %s",
>> diff --git a/qemu-io.c b/qemu-io.c
>> index 7f459d8..8da8f6e 100644
>> --- a/qemu-io.c
>> +++ b/qemu-io.c
>> @@ -68,7 +68,7 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
>> } else {
>> qemuio_bs = bdrv_new("hda");
>>
>> - if (bdrv_open(qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
>> + if (bdrv_open(&qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
>> fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
>> error_get_pretty(local_err));
>> error_free(local_err);
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index 136e8c9..0cf123c 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -597,7 +597,7 @@ int main(int argc, char **argv)
>>
>> bs = bdrv_new("hda");
>> srcpath = argv[optind];
>> - ret = bdrv_open(bs, srcpath, NULL, flags, drv, &local_err);
>> + ret = bdrv_open(&bs, srcpath, NULL, flags, drv, &local_err);
>> if (ret < 0) {
>> errno = -ret;
>> err(EXIT_FAILURE, "Failed to bdrv_open '%s': %s", argv[optind],
>> --
>> 1.8.5.4
>>
>>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/8] block: Add reference parameter to bdrv_open()
2014-02-10 13:30 ` Benoît Canet
@ 2014-02-12 0:17 ` Max Reitz
0 siblings, 0 replies; 25+ messages in thread
From: Max Reitz @ 2014-02-12 0:17 UTC (permalink / raw)
To: Benoît Canet; +Cc: Kevin Wolf, Jeff Cody, qemu-devel, Stefan Hajnoczi
On 10.02.2014 14:30, Benoît Canet wrote:
> Le Saturday 08 Feb 2014 à 18:39:13 (+0100), Max Reitz a écrit :
>> Allow bdrv_open() to handle references to existing block devices just as
>> bdrv_file_open() is already capable of.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> block.c | 41 ++++++++++++++++++++++++++++++++++-------
>> block/qcow2.c | 4 ++--
>> block/vmdk.c | 3 ++-
>> block/vvfat.c | 2 +-
>> blockdev.c | 12 ++++++------
>> hw/block/xen_disk.c | 4 ++--
>> include/block/block.h | 5 +++--
>> qemu-img.c | 8 ++++----
>> qemu-io.c | 4 +++-
>> qemu-nbd.c | 2 +-
>> 10 files changed, 58 insertions(+), 27 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 40a585a..3a32c37 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1040,7 +1040,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>> }
>>
>> if (!drv->bdrv_file_open) {
>> - ret = bdrv_open(&bs, filename, options, flags, drv, &local_err);
>> + ret = bdrv_open(&bs, filename, NULL, options, flags, drv, &local_err);
>> options = NULL;
>> } else {
>> ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
>> @@ -1119,7 +1119,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>>
>> assert(bs->backing_hd == NULL);
>> ret = bdrv_open(&bs->backing_hd,
>> - *backing_filename ? backing_filename : NULL, options,
>> + *backing_filename ? backing_filename : NULL, NULL, options,
>> back_flags, back_drv, &local_err);
>> if (ret < 0) {
>> bs->backing_hd = NULL;
>> @@ -1199,7 +1199,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>> goto done;
>> }
>>
>> - ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp);
>> + ret = bdrv_open(pbs, filename, NULL, image_options, flags, NULL, errp);
>> } else {
>> ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
>> errp);
>> @@ -1221,8 +1221,9 @@ done:
>> * If *pbs is NULL, a new BDS will be created with a pointer to it stored there.
>> * If it is not NULL, the referenced BDS will be reused.
>> */
>> -int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
>> - int flags, BlockDriver *drv, Error **errp)
>> +int bdrv_open(BlockDriverState **pbs, const char *filename,
>> + const char *reference, QDict *options, int flags,
>> + BlockDriver *drv, Error **errp)
> Maybe a little reference to the reference parameters in the comments would help
> ?
Yes, that would probably be helpful.
Max
> Aside from that:
>
> Reviewed-by: Benoit Canet <benoit@irqsave.net>
>
>> {
>> int ret;
>> /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
>> @@ -1233,6 +1234,32 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
>>
>> assert(pbs);
>>
>> + if (reference) {
>> + bool options_non_empty = options ? qdict_size(options) : false;
>> + QDECREF(options);
>> +
>> + if (*pbs) {
>> + error_setg(errp, "Cannot reuse an existing BDS when referencing "
>> + "another block device");
>> + return -EINVAL;
>> + }
>> +
>> + if (filename || options_non_empty) {
>> + error_setg(errp, "Cannot reference an existing block device with "
>> + "additional options or a new filename");
>> + return -EINVAL;
>> + }
>> +
>> + bs = bdrv_find(reference);
>> + if (!bs) {
>> + error_setg(errp, "Cannot find block device '%s'", reference);
>> + return -ENODEV;
>> + }
>> + bdrv_ref(bs);
>> + *pbs = bs;
>> + return 0;
>> + }
>> +
>> if (*pbs) {
>> bs = *pbs;
>> } else {
>> @@ -1260,7 +1287,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
>> /* Get the required size from the image */
>> QINCREF(options);
>> bs1 = NULL;
>> - ret = bdrv_open(&bs1, filename, options, BDRV_O_NO_BACKING,
>> + ret = bdrv_open(&bs1, filename, NULL, options, BDRV_O_NO_BACKING,
>> drv, &local_err);
>> if (ret < 0) {
>> goto fail;
>> @@ -5305,7 +5332,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
>> flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
>>
>> bs = NULL;
>> - ret = bdrv_open(&bs, backing_file->value.s, NULL, back_flags,
>> + ret = bdrv_open(&bs, backing_file->value.s, NULL, NULL, back_flags,
>> backing_drv, &local_err);
>> if (ret < 0) {
>> error_setg_errno(errp, -ret, "Could not open '%s': %s",
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index c8e8ba7..6996276 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1553,7 +1553,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>> */
>> BlockDriver* drv = bdrv_find_format("qcow2");
>> assert(drv != NULL);
>> - ret = bdrv_open(&bs, filename, NULL,
>> + ret = bdrv_open(&bs, filename, NULL, NULL,
>> BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, drv, &local_err);
>> if (ret < 0) {
>> error_propagate(errp, local_err);
>> @@ -1604,7 +1604,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>> bs = NULL;
>>
>> /* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning */
>> - ret = bdrv_open(&bs, filename, NULL,
>> + ret = bdrv_open(&bs, filename, NULL, NULL,
>> BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING,
>> drv, &local_err);
>> if (error_is_set(&local_err)) {
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index 52cd12a..e007793 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -1756,7 +1756,8 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
>> }
>> if (backing_file) {
>> BlockDriverState *bs = NULL;
>> - ret = bdrv_open(&bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp);
>> + ret = bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_NO_BACKING, NULL,
>> + errp);
>> if (ret != 0) {
>> goto exit;
>> }
>> diff --git a/block/vvfat.c b/block/vvfat.c
>> index ae7bc6f..d7a830e 100644
>> --- a/block/vvfat.c
>> +++ b/block/vvfat.c
>> @@ -2937,7 +2937,7 @@ static int enable_write_target(BDRVVVFATState *s)
>> }
>>
>> s->qcow = NULL;
>> - ret = bdrv_open(&s->qcow, s->qcow_filename, NULL,
>> + ret = bdrv_open(&s->qcow, s->qcow_filename, NULL, NULL,
>> BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow,
>> &local_err);
>> if (ret < 0) {
>> diff --git a/blockdev.c b/blockdev.c
>> index 6dd351c..7b7e349 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -510,7 +510,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>> bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>>
>> QINCREF(bs_opts);
>> - ret = bdrv_open(&dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
>> + ret = bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, drv, &error);
>>
>> if (ret < 0) {
>> error_setg(errp, "could not open disk image %s: %s",
>> @@ -1304,7 +1304,7 @@ static void external_snapshot_prepare(BlkTransactionState *common,
>> /* TODO Inherit bs->options or only take explicit options with an
>> * extended QMP command? */
>> assert(state->new_bs == NULL);
>> - ret = bdrv_open(&state->new_bs, new_image_file, options,
>> + ret = bdrv_open(&state->new_bs, new_image_file, NULL, options,
>> flags | BDRV_O_NO_BACKING, drv, &local_err);
>> /* We will manually add the backing_hd field to the bs later */
>> if (ret != 0) {
>> @@ -1555,7 +1555,7 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
>> Error *local_err = NULL;
>> int ret;
>>
>> - ret = bdrv_open(&bs, filename, NULL, bdrv_flags, drv, &local_err);
>> + ret = bdrv_open(&bs, filename, NULL, NULL, bdrv_flags, drv, &local_err);
>> if (ret < 0) {
>> error_propagate(errp, local_err);
>> return;
>> @@ -1992,7 +1992,7 @@ void qmp_drive_backup(const char *device, const char *target,
>> }
>>
>> target_bs = NULL;
>> - ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err);
>> + ret = bdrv_open(&target_bs, target, NULL, NULL, flags, drv, &local_err);
>> if (ret < 0) {
>> error_propagate(errp, local_err);
>> return;
>> @@ -2135,8 +2135,8 @@ void qmp_drive_mirror(const char *device, const char *target,
>> * file.
>> */
>> target_bs = NULL;
>> - ret = bdrv_open(&target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
>> - &local_err);
>> + ret = bdrv_open(&target_bs, target, NULL, NULL, flags | BDRV_O_NO_BACKING,
>> + drv, &local_err);
>> if (ret < 0) {
>> error_propagate(errp, local_err);
>> return;
>> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
>> index 14e6d0b..61e6ff3 100644
>> --- a/hw/block/xen_disk.c
>> +++ b/hw/block/xen_disk.c
>> @@ -813,8 +813,8 @@ static int blk_connect(struct XenDevice *xendev)
>> Error *local_err = NULL;
>> BlockDriver *drv = bdrv_find_whitelisted_format(blkdev->fileproto,
>> readonly);
>> - if (bdrv_open(&blkdev->bs,
>> - blkdev->filename, NULL, qflags, drv, &local_err) != 0)
>> + if (bdrv_open(&blkdev->bs, blkdev->filename, NULL, NULL, qflags,
>> + drv, &local_err) != 0)
>> {
>> xen_be_printf(&blkdev->xendev, 0, "error: %s\n",
>> error_get_pretty(local_err));
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 980869d..a421041 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -190,8 +190,9 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>> QDict *options, const char *bdref_key, int flags,
>> bool force_raw, bool allow_none, Error **errp);
>> int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
>> -int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
>> - int flags, BlockDriver *drv, Error **errp);
>> +int bdrv_open(BlockDriverState **pbs, const char *filename,
>> + const char *reference, QDict *options, int flags,
>> + BlockDriver *drv, Error **errp);
>> BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
>> BlockDriverState *bs, int flags);
>> int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 897aa56..b834d52 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -289,7 +289,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
>> drv = NULL;
>> }
>>
>> - ret = bdrv_open(&bs, filename, NULL, flags, drv, &local_err);
>> + ret = bdrv_open(&bs, filename, NULL, NULL, flags, drv, &local_err);
>> if (ret < 0) {
>> error_report("Could not open '%s': %s", filename,
>> error_get_pretty(local_err));
>> @@ -2312,7 +2312,7 @@ static int img_rebase(int argc, char **argv)
>>
>> bs_old_backing = bdrv_new("old_backing");
>> bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
>> - ret = bdrv_open(&bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
>> + ret = bdrv_open(&bs_old_backing, backing_name, NULL, NULL, BDRV_O_FLAGS,
>> old_backing_drv, &local_err);
>> if (ret) {
>> error_report("Could not open old backing file '%s': %s",
>> @@ -2322,8 +2322,8 @@ static int img_rebase(int argc, char **argv)
>> }
>> if (out_baseimg[0]) {
>> bs_new_backing = bdrv_new("new_backing");
>> - ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
>> - new_backing_drv, &local_err);
>> + ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, NULL,
>> + BDRV_O_FLAGS, new_backing_drv, &local_err);
>> if (ret) {
>> error_report("Could not open new backing file '%s': %s",
>> out_baseimg, error_get_pretty(local_err));
>> diff --git a/qemu-io.c b/qemu-io.c
>> index 8da8f6e..61d54c0 100644
>> --- a/qemu-io.c
>> +++ b/qemu-io.c
>> @@ -68,7 +68,9 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
>> } else {
>> qemuio_bs = bdrv_new("hda");
>>
>> - if (bdrv_open(&qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
>> + if (bdrv_open(&qemuio_bs, name, NULL, opts, flags, NULL, &local_err)
>> + < 0)
>> + {
>> fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
>> error_get_pretty(local_err));
>> error_free(local_err);
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index 0cf123c..711162c 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -597,7 +597,7 @@ int main(int argc, char **argv)
>>
>> bs = bdrv_new("hda");
>> srcpath = argv[optind];
>> - ret = bdrv_open(&bs, srcpath, NULL, flags, drv, &local_err);
>> + ret = bdrv_open(&bs, srcpath, NULL, NULL, flags, drv, &local_err);
>> if (ret < 0) {
>> errno = -ret;
>> err(EXIT_FAILURE, "Failed to bdrv_open '%s': %s", argv[optind],
>> --
>> 1.8.5.4
>>
>>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 7/8] block: Reuse success path from bdrv_open()
2014-02-10 14:56 ` Kevin Wolf
@ 2014-02-12 0:26 ` Max Reitz
0 siblings, 0 replies; 25+ messages in thread
From: Max Reitz @ 2014-02-12 0:26 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Jeff Cody, qemu-devel, Stefan Hajnoczi, Benoît Canet
On 10.02.2014 15:56, Kevin Wolf wrote:
> Am 08.02.2014 um 18:39 hat Max Reitz geschrieben:
>> The fail and success paths of bdrv_file_open() may be further shortened
>> by reusing code already existent in bdrv_open(). This includes
>> bdrv_file_open() not taking the reference to options which allows the
>> removal of QDECREF(options) in that function.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> @@ -1001,41 +1003,35 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>>
>> /* Parse the filename and open it */
>> if (drv->bdrv_parse_filename && filename) {
>> - drv->bdrv_parse_filename(filename, options, &local_err);
>> + drv->bdrv_parse_filename(filename, *options, &local_err);
>> if (error_is_set(&local_err)) {
>> error_propagate(errp, local_err);
>> ret = -EINVAL;
>> goto fail;
>> }
>> - qdict_del(options, "filename");
>> + qdict_del(*options, "filename");
>> + } else if (drv->bdrv_needs_filename && !filename) {
>> + error_setg(errp, "The '%s' block driver requires a file name",
>> + drv->format_name);
>> + ret = -EINVAL;
>> + goto fail;
>> }
> How did this part end up in this patch? It doesn't look wrong, though I
> think bdrv_open_common() should already catch it. In any case it's an
> addition that the commit message didn't mention.
I wonder. It definitely doesn't belong here, since as of your commit
"block: Fail gracefully with missing filename" this check should be in
bdrv_open_common() and not here. I guess, I somehow ended up reverting
it to the old state here. I just hope there aren't any more such
reverts; I'll take a look. I remember some rebase conflict here (for
obvious reasons), so it's probably just in this case, though.
Max
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2014-02-12 0:24 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-02-08 17:39 [Qemu-devel] [PATCH v2 0/8] block: Integrate bdrv_file_open() into bdrv_open() Max Reitz
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 1/8] block: Change BDS parameter of bdrv_open() to ** Max Reitz
2014-02-10 12:42 ` Kevin Wolf
2014-02-12 0:10 ` Max Reitz
2014-02-10 13:17 ` Benoît Canet
2014-02-12 0:15 ` Max Reitz
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 2/8] block: Add reference parameter to bdrv_open() Max Reitz
2014-02-10 13:30 ` Benoît Canet
2014-02-12 0:17 ` Max Reitz
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 3/8] block: Make bdrv_file_open() static Max Reitz
2014-02-10 13:40 ` Benoît Canet
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 4/8] block: Reuse reference handling from bdrv_open() Max Reitz
2014-02-10 13:42 ` Benoît Canet
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 5/8] block: Remove bdrv_new() from bdrv_file_open() Max Reitz
2014-02-10 13:49 ` Benoît Canet
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 6/8] block: Handle bs->options in bdrv_open() only Max Reitz
2014-02-10 16:23 ` Benoît Canet
2014-02-10 16:23 ` Benoît Canet
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 7/8] block: Reuse success path from bdrv_open() Max Reitz
2014-02-10 14:56 ` Kevin Wolf
2014-02-12 0:26 ` Max Reitz
2014-02-10 16:28 ` Benoît Canet
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 8/8] block: Remove bdrv_open_image()'s force_raw option Max Reitz
2014-02-10 16:31 ` Benoît Canet
2014-02-10 15:01 ` [Qemu-devel] [PATCH v2 0/8] block: Integrate bdrv_file_open() into bdrv_open() Kevin Wolf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).