* [Qemu-devel] [PATCH 01/10] block: Change BDS parameter of bdrv_open() to **
2014-01-26 19:02 [Qemu-devel] [PATCH 00/10] block: Integrate bdrv_file_open() into bdrv_open() Max Reitz
@ 2014-01-26 19:02 ` Max Reitz
2014-01-27 2:38 ` Benoît Canet
` (2 more replies)
2014-01-26 19:02 ` [Qemu-devel] [PATCH 02/10] block: Add reference parameter to bdrv_open() Max Reitz
` (8 subsequent siblings)
9 siblings, 3 replies; 35+ messages in thread
From: Max Reitz @ 2014-01-26 19:02 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, 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 | 53 ++++++++++++++++++++++++++-------------------------
block/qcow2.c | 14 +++++++++-----
block/vmdk.c | 5 ++---
block/vvfat.c | 6 ++----
blockdev.c | 21 ++++++++------------
hw/block/xen_disk.c | 2 +-
include/block/block.h | 2 +-
qemu-img.c | 6 +++---
qemu-io.c | 2 +-
qemu-nbd.c | 2 +-
10 files changed, 55 insertions(+), 58 deletions(-)
diff --git a/block.c b/block.c
index cb21a5f..c660609 100644
--- a/block.c
+++ b/block.c
@@ -1039,7 +1039,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);
@@ -1108,8 +1108,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);
}
@@ -1118,11 +1116,10 @@ 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,
+ 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",
@@ -1189,8 +1186,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");
@@ -1198,13 +1193,8 @@ 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;
- }
+ *pbs = NULL;
+ ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp);
} else {
ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
errp);
@@ -1222,14 +1212,17 @@ 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 = NULL;
const char *drvname;
Error *local_err = NULL;
@@ -1238,12 +1231,17 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
options = qdict_new();
}
+ if (*pbs) {
+ bs = *pbs;
+ } else {
+ bs = bdrv_new("");
+ }
bs->options = options;
options = qdict_clone_shallow(options);
/* For snapshot=on, create a temporary qcow2 overlay */
if (flags & BDRV_O_SNAPSHOT) {
- BlockDriverState *bs1;
+ BlockDriverState *bs1 = NULL;
int64_t total_size;
BlockDriver *bdrv_qcow2;
QEMUOptionParameter *create_options;
@@ -1253,12 +1251,10 @@ 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,
+ 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;
@@ -1386,6 +1382,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:
@@ -1399,13 +1396,20 @@ fail:
QDECREF(bs->options);
QDECREF(options);
bs->options = NULL;
+ if (!*pbs) {
+ bdrv_unref(bs);
+ }
if (error_is_set(&local_err)) {
error_propagate(errp, local_err);
}
return ret;
close_and_fail:
- bdrv_close(bs);
+ if (*pbs) {
+ bdrv_close(bs);
+ } else {
+ bdrv_unref(bs);
+ }
QDECREF(options);
if (error_is_set(&local_err)) {
error_propagate(errp, local_err);
@@ -5276,7 +5280,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
size = get_option_parameter(param, BLOCK_OPT_SIZE);
if (size && size->value.n == -1) {
if (backing_file && backing_file->value.s) {
- BlockDriverState *bs;
+ BlockDriverState *bs = NULL;
uint64_t size;
char buf[32];
int back_flags;
@@ -5285,9 +5289,7 @@ 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,
+ 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",
@@ -5295,7 +5297,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/qcow2.c b/block/qcow2.c
index 2da62b8..9a25fbd 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1541,7 +1541,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
@@ -1550,7 +1551,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);
@@ -1597,10 +1598,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)) {
@@ -1610,7 +1612,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 99ca60f..0fbf230 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..42163f8 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,11 @@ 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,
+ 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 +1554,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;
@@ -1906,7 +1905,7 @@ void qmp_drive_backup(const char *device, const char *target,
Error **errp)
{
BlockDriverState *bs;
- BlockDriverState *target_bs;
+ BlockDriverState *target_bs = NULL;
BlockDriverState *source = NULL;
BlockDriver *drv = NULL;
Error *local_err = NULL;
@@ -1991,10 +1990,8 @@ 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);
+ ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err);
if (ret < 0) {
- bdrv_unref(target_bs);
error_propagate(errp, local_err);
return;
}
@@ -2027,7 +2024,7 @@ void qmp_drive_mirror(const char *device, const char *target,
Error **errp)
{
BlockDriverState *bs;
- BlockDriverState *source, *target_bs;
+ BlockDriverState *source, *target_bs = NULL;
BlockDriver *drv = NULL;
Error *local_err = NULL;
int flags;
@@ -2135,11 +2132,9 @@ 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,
+ 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..c39d486 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));
@@ -2314,7 +2314,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 +2324,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 d669028..2f06dc6 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.3
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH 01/10] block: Change BDS parameter of bdrv_open() to **
2014-01-26 19:02 ` [Qemu-devel] [PATCH 01/10] block: Change BDS parameter of bdrv_open() to ** Max Reitz
@ 2014-01-27 2:38 ` Benoît Canet
2014-01-27 18:51 ` Max Reitz
2014-01-27 19:31 ` Jeff Cody
2014-01-29 11:50 ` Kevin Wolf
2 siblings, 1 reply; 35+ messages in thread
From: Benoît Canet @ 2014-01-27 2:38 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
Le Sunday 26 Jan 2014 à 20:02:34 (+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 | 53 ++++++++++++++++++++++++++-------------------------
> block/qcow2.c | 14 +++++++++-----
> block/vmdk.c | 5 ++---
> block/vvfat.c | 6 ++----
> blockdev.c | 21 ++++++++------------
> hw/block/xen_disk.c | 2 +-
> include/block/block.h | 2 +-
> qemu-img.c | 6 +++---
> qemu-io.c | 2 +-
> qemu-nbd.c | 2 +-
> 10 files changed, 55 insertions(+), 58 deletions(-)
>
> diff --git a/block.c b/block.c
> index cb21a5f..c660609 100644
> --- a/block.c
> +++ b/block.c
> @@ -1039,7 +1039,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);
> @@ -1108,8 +1108,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);
> }
> @@ -1118,11 +1116,10 @@ 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,
> + 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",
> @@ -1189,8 +1186,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");
> @@ -1198,13 +1193,8 @@ 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;
> - }
> + *pbs = NULL;
bdrv_open set *file = NULL before calling bdrv_open_image so this is redundant.
What would be the correct behavior if the caller was giving *pbs != NULL ?
Is loosing the reference to a previously existing bds right ?
> + ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp);
> } else {
> ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
> errp);
> @@ -1222,14 +1212,17 @@ 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 = NULL;
> const char *drvname;
> Error *local_err = NULL;
>
> @@ -1238,12 +1231,17 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> options = qdict_new();
> }
>
> + if (*pbs) {
> + bs = *pbs;
> + } else {
> + bs = bdrv_new("");
> + }
> bs->options = options;
> options = qdict_clone_shallow(options);
>
> /* For snapshot=on, create a temporary qcow2 overlay */
> if (flags & BDRV_O_SNAPSHOT) {
> - BlockDriverState *bs1;
> + BlockDriverState *bs1 = NULL;
> int64_t total_size;
> BlockDriver *bdrv_qcow2;
> QEMUOptionParameter *create_options;
> @@ -1253,12 +1251,10 @@ 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,
> + 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;
> @@ -1386,6 +1382,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:
The goto chain is different on Kevin qemu.git block; there is an additional fail
label ?
On which tree and branch are these patches based ?
> @@ -1399,13 +1396,20 @@ fail:
> QDECREF(bs->options);
> QDECREF(options);
> bs->options = NULL;
> + if (!*pbs) {
> + bdrv_unref(bs);
> + }
> if (error_is_set(&local_err)) {
> error_propagate(errp, local_err);
> }
> return ret;
>
> close_and_fail:
> - bdrv_close(bs);
> + if (*pbs) {
> + bdrv_close(bs);
> + } else {
> + bdrv_unref(bs);
> + }
> QDECREF(options);
> if (error_is_set(&local_err)) {
> error_propagate(errp, local_err);
> @@ -5276,7 +5280,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
> size = get_option_parameter(param, BLOCK_OPT_SIZE);
> if (size && size->value.n == -1) {
> if (backing_file && backing_file->value.s) {
> - BlockDriverState *bs;
> + BlockDriverState *bs = NULL;
> uint64_t size;
> char buf[32];
> int back_flags;
> @@ -5285,9 +5289,7 @@ 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,
> + 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",
> @@ -5295,7 +5297,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/qcow2.c b/block/qcow2.c
> index 2da62b8..9a25fbd 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1541,7 +1541,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
> @@ -1550,7 +1551,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);
> @@ -1597,10 +1598,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)) {
> @@ -1610,7 +1612,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 99ca60f..0fbf230 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..42163f8 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,11 @@ 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? */
The state has been g_malloc0 so ok.
> - ret = bdrv_open(state->new_bs, new_image_file, options,
> + 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 +1554,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);
It already open here so ok.
> + ret = bdrv_open(&bs, filename, NULL, bdrv_flags, drv, &local_err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> return;
> @@ -1906,7 +1905,7 @@ void qmp_drive_backup(const char *device, const char *target,
> Error **errp)
> {
> BlockDriverState *bs;
> - BlockDriverState *target_bs;
> + BlockDriverState *target_bs = NULL;
> BlockDriverState *source = NULL;
> BlockDriver *drv = NULL;
> Error *local_err = NULL;
> @@ -1991,10 +1990,8 @@ 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);
> + ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err);
> if (ret < 0) {
> - bdrv_unref(target_bs);
> error_propagate(errp, local_err);
> return;
> }
> @@ -2027,7 +2024,7 @@ void qmp_drive_mirror(const char *device, const char *target,
> Error **errp)
> {
> BlockDriverState *bs;
> - BlockDriverState *source, *target_bs;
> + BlockDriverState *source, *target_bs = NULL;
> BlockDriver *drv = NULL;
> Error *local_err = NULL;
> int flags;
> @@ -2135,11 +2132,9 @@ 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,
> + 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..c39d486 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);
I wonder if the if(bs) test in the fail label is dead code.
The bdrv_unref in the test branch is not.
I don't see how bs could be NULL.
> if (ret < 0) {
> error_report("Could not open '%s': %s", filename,
> error_get_pretty(local_err));
> @@ -2314,7 +2314,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 +2324,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 d669028..2f06dc6 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.3
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH 01/10] block: Change BDS parameter of bdrv_open() to **
2014-01-27 2:38 ` Benoît Canet
@ 2014-01-27 18:51 ` Max Reitz
0 siblings, 0 replies; 35+ messages in thread
From: Max Reitz @ 2014-01-27 18:51 UTC (permalink / raw)
To: Benoît Canet; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On 27.01.2014 03:38, Benoît Canet wrote:
> Le Sunday 26 Jan 2014 à 20:02:34 (+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 | 53 ++++++++++++++++++++++++++-------------------------
>> block/qcow2.c | 14 +++++++++-----
>> block/vmdk.c | 5 ++---
>> block/vvfat.c | 6 ++----
>> blockdev.c | 21 ++++++++------------
>> hw/block/xen_disk.c | 2 +-
>> include/block/block.h | 2 +-
>> qemu-img.c | 6 +++---
>> qemu-io.c | 2 +-
>> qemu-nbd.c | 2 +-
>> 10 files changed, 55 insertions(+), 58 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index cb21a5f..c660609 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1039,7 +1039,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);
>> @@ -1108,8 +1108,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);
>> }
>> @@ -1118,11 +1116,10 @@ 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,
>> + 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",
>> @@ -1189,8 +1186,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");
>> @@ -1198,13 +1193,8 @@ 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;
>> - }
>> + *pbs = NULL;
> bdrv_open set *file = NULL before calling bdrv_open_image so this is redundant.
> What would be the correct behavior if the caller was giving *pbs != NULL ?
> Is loosing the reference to a previously existing bds right ?
I thought about that, too. But it would have to be a pretty broken
caller to rely on this function to preserve *pbs if it fails and to
overwrite it otherwise. But you're right, to conform with the behavior
of bdrv_open/bdrv_file_open, it would probably make more sense not to
overwrite *pbs.
>> + ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp);
>> } else {
>> ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
>> errp);
>> @@ -1222,14 +1212,17 @@ 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 = NULL;
>> const char *drvname;
>> Error *local_err = NULL;
>>
>> @@ -1238,12 +1231,17 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>> options = qdict_new();
>> }
>>
>> + if (*pbs) {
>> + bs = *pbs;
>> + } else {
>> + bs = bdrv_new("");
>> + }
>> bs->options = options;
>> options = qdict_clone_shallow(options);
>>
>> /* For snapshot=on, create a temporary qcow2 overlay */
>> if (flags & BDRV_O_SNAPSHOT) {
>> - BlockDriverState *bs1;
>> + BlockDriverState *bs1 = NULL;
>> int64_t total_size;
>> BlockDriver *bdrv_qcow2;
>> QEMUOptionParameter *create_options;
>> @@ -1253,12 +1251,10 @@ 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,
>> + 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;
>> @@ -1386,6 +1382,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:
> The goto chain is different on Kevin qemu.git block; there is an additional fail
> label ?
> On which tree and branch are these patches based ?
They are based on Kevin's (current) block branch (at least, that's what
my git log says ;-)).
>
>> @@ -1399,13 +1396,20 @@ fail:
>> QDECREF(bs->options);
>> QDECREF(options);
>> bs->options = NULL;
>> + if (!*pbs) {
>> + bdrv_unref(bs);
>> + }
>> if (error_is_set(&local_err)) {
>> error_propagate(errp, local_err);
>> }
>> return ret;
>>
>> close_and_fail:
>> - bdrv_close(bs);
>> + if (*pbs) {
>> + bdrv_close(bs);
>> + } else {
>> + bdrv_unref(bs);
>> + }
>> QDECREF(options);
>> if (error_is_set(&local_err)) {
>> error_propagate(errp, local_err);
>> @@ -5276,7 +5280,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
>> size = get_option_parameter(param, BLOCK_OPT_SIZE);
>> if (size && size->value.n == -1) {
>> if (backing_file && backing_file->value.s) {
>> - BlockDriverState *bs;
>> + BlockDriverState *bs = NULL;
>> uint64_t size;
>> char buf[32];
>> int back_flags;
>> @@ -5285,9 +5289,7 @@ 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,
>> + 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",
>> @@ -5295,7 +5297,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/qcow2.c b/block/qcow2.c
>> index 2da62b8..9a25fbd 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1541,7 +1541,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
>> @@ -1550,7 +1551,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);
>> @@ -1597,10 +1598,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)) {
>> @@ -1610,7 +1612,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 99ca60f..0fbf230 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..42163f8 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,11 @@ 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? */
> The state has been g_malloc0 so ok.
>> - ret = bdrv_open(state->new_bs, new_image_file, options,
>> + 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 +1554,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);
> It already open here so ok.
>> + ret = bdrv_open(&bs, filename, NULL, bdrv_flags, drv, &local_err);
>> if (ret < 0) {
>> error_propagate(errp, local_err);
>> return;
>> @@ -1906,7 +1905,7 @@ void qmp_drive_backup(const char *device, const char *target,
>> Error **errp)
>> {
>> BlockDriverState *bs;
>> - BlockDriverState *target_bs;
>> + BlockDriverState *target_bs = NULL;
>> BlockDriverState *source = NULL;
>> BlockDriver *drv = NULL;
>> Error *local_err = NULL;
>> @@ -1991,10 +1990,8 @@ 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);
>> + ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err);
>> if (ret < 0) {
>> - bdrv_unref(target_bs);
>> error_propagate(errp, local_err);
>> return;
>> }
>> @@ -2027,7 +2024,7 @@ void qmp_drive_mirror(const char *device, const char *target,
>> Error **errp)
>> {
>> BlockDriverState *bs;
>> - BlockDriverState *source, *target_bs;
>> + BlockDriverState *source, *target_bs = NULL;
>> BlockDriver *drv = NULL;
>> Error *local_err = NULL;
>> int flags;
>> @@ -2135,11 +2132,9 @@ 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,
>> + 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..c39d486 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);
> I wonder if the if(bs) test in the fail label is dead code.
> The bdrv_unref in the test branch is not.
> I don't see how bs could be NULL.
Oh, yes, you're right. I'll remove it.
>> if (ret < 0) {
>> error_report("Could not open '%s': %s", filename,
>> error_get_pretty(local_err));
>> @@ -2314,7 +2314,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 +2324,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 d669028..2f06dc6 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.3
>>
>>
Thanks for reviewing!
Max
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 01/10] block: Change BDS parameter of bdrv_open() to **
2014-01-26 19:02 ` [Qemu-devel] [PATCH 01/10] block: Change BDS parameter of bdrv_open() to ** Max Reitz
2014-01-27 2:38 ` Benoît Canet
@ 2014-01-27 19:31 ` Jeff Cody
2014-01-27 19:35 ` Max Reitz
2014-01-29 11:50 ` Kevin Wolf
2 siblings, 1 reply; 35+ messages in thread
From: Jeff Cody @ 2014-01-27 19:31 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On Sun, Jan 26, 2014 at 08:02:34PM +0100, Max Reitz wrote:
> 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 | 53 ++++++++++++++++++++++++++-------------------------
> block/qcow2.c | 14 +++++++++-----
> block/vmdk.c | 5 ++---
> block/vvfat.c | 6 ++----
> blockdev.c | 21 ++++++++------------
> hw/block/xen_disk.c | 2 +-
> include/block/block.h | 2 +-
> qemu-img.c | 6 +++---
> qemu-io.c | 2 +-
> qemu-nbd.c | 2 +-
> 10 files changed, 55 insertions(+), 58 deletions(-)
>
> diff --git a/block.c b/block.c
> index cb21a5f..c660609 100644
> --- a/block.c
> +++ b/block.c
> @@ -1039,7 +1039,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);
> @@ -1108,8 +1108,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);
> }
> @@ -1118,11 +1116,10 @@ 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,
> + 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",
> @@ -1189,8 +1186,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");
> @@ -1198,13 +1193,8 @@ 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;
> - }
> + *pbs = NULL;
> + ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp);
> } else {
> ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
> errp);
> @@ -1222,14 +1212,17 @@ 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 = NULL;
> const char *drvname;
> Error *local_err = NULL;
>
Since it is valid to pass *pbs = NULL for new allocation, perhaps we
should add an assert here to help thwart easy mistakes, and make sure
that **pbs != NULL:
assert(pbs != NULL);
> @@ -1238,12 +1231,17 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> options = qdict_new();
> }
>
> + if (*pbs) {
> + bs = *pbs;
> + } else {
> + bs = bdrv_new("");
> + }
> bs->options = options;
> options = qdict_clone_shallow(options);
>
> /* For snapshot=on, create a temporary qcow2 overlay */
> if (flags & BDRV_O_SNAPSHOT) {
> - BlockDriverState *bs1;
> + BlockDriverState *bs1 = NULL;
> int64_t total_size;
> BlockDriver *bdrv_qcow2;
> QEMUOptionParameter *create_options;
> @@ -1253,12 +1251,10 @@ 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,
> + 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;
> @@ -1386,6 +1382,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:
> @@ -1399,13 +1396,20 @@ fail:
> QDECREF(bs->options);
> QDECREF(options);
> bs->options = NULL;
> + if (!*pbs) {
> + bdrv_unref(bs);
> + }
> if (error_is_set(&local_err)) {
> error_propagate(errp, local_err);
> }
> return ret;
>
> close_and_fail:
> - bdrv_close(bs);
> + if (*pbs) {
> + bdrv_close(bs);
> + } else {
> + bdrv_unref(bs);
> + }
If *pbs is != NULL, and we reach an error case, we have to clean up
the new BDS we created. This makes sense, but it may not be
immediately obvious that *pbs = NULL means that *bs was allocated in
this function. It may be worth a comment in the fail labels.
> QDECREF(options);
> if (error_is_set(&local_err)) {
> error_propagate(errp, local_err);
> @@ -5276,7 +5280,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
> size = get_option_parameter(param, BLOCK_OPT_SIZE);
> if (size && size->value.n == -1) {
> if (backing_file && backing_file->value.s) {
> - BlockDriverState *bs;
> + BlockDriverState *bs = NULL;
> uint64_t size;
> char buf[32];
> int back_flags;
> @@ -5285,9 +5289,7 @@ 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,
> + 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",
> @@ -5295,7 +5297,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/qcow2.c b/block/qcow2.c
> index 2da62b8..9a25fbd 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1541,7 +1541,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
> @@ -1550,7 +1551,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);
> @@ -1597,10 +1598,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)) {
> @@ -1610,7 +1612,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 99ca60f..0fbf230 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..42163f8 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,11 @@ 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,
> + 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 +1554,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;
> @@ -1906,7 +1905,7 @@ void qmp_drive_backup(const char *device, const char *target,
> Error **errp)
> {
> BlockDriverState *bs;
> - BlockDriverState *target_bs;
> + BlockDriverState *target_bs = NULL;
> BlockDriverState *source = NULL;
> BlockDriver *drv = NULL;
> Error *local_err = NULL;
> @@ -1991,10 +1990,8 @@ 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);
> + ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err);
> if (ret < 0) {
> - bdrv_unref(target_bs);
> error_propagate(errp, local_err);
> return;
> }
> @@ -2027,7 +2024,7 @@ void qmp_drive_mirror(const char *device, const char *target,
> Error **errp)
> {
> BlockDriverState *bs;
> - BlockDriverState *source, *target_bs;
> + BlockDriverState *source, *target_bs = NULL;
> BlockDriver *drv = NULL;
> Error *local_err = NULL;
> int flags;
> @@ -2135,11 +2132,9 @@ 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,
> + 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..c39d486 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));
> @@ -2314,7 +2314,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 +2324,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 d669028..2f06dc6 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.3
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH 01/10] block: Change BDS parameter of bdrv_open() to **
2014-01-27 19:31 ` Jeff Cody
@ 2014-01-27 19:35 ` Max Reitz
0 siblings, 0 replies; 35+ messages in thread
From: Max Reitz @ 2014-01-27 19:35 UTC (permalink / raw)
To: Jeff Cody; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On 27.01.2014 20:31, Jeff Cody wrote:
> On Sun, Jan 26, 2014 at 08:02:34PM +0100, Max Reitz wrote:
>> 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 | 53 ++++++++++++++++++++++++++-------------------------
>> block/qcow2.c | 14 +++++++++-----
>> block/vmdk.c | 5 ++---
>> block/vvfat.c | 6 ++----
>> blockdev.c | 21 ++++++++------------
>> hw/block/xen_disk.c | 2 +-
>> include/block/block.h | 2 +-
>> qemu-img.c | 6 +++---
>> qemu-io.c | 2 +-
>> qemu-nbd.c | 2 +-
>> 10 files changed, 55 insertions(+), 58 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index cb21a5f..c660609 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1039,7 +1039,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);
>> @@ -1108,8 +1108,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);
>> }
>> @@ -1118,11 +1116,10 @@ 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,
>> + 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",
>> @@ -1189,8 +1186,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");
>> @@ -1198,13 +1193,8 @@ 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;
>> - }
>> + *pbs = NULL;
>> + ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp);
>> } else {
>> ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
>> errp);
>> @@ -1222,14 +1212,17 @@ 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 = NULL;
>> const char *drvname;
>> Error *local_err = NULL;
>>
> Since it is valid to pass *pbs = NULL for new allocation, perhaps we
> should add an assert here to help thwart easy mistakes, and make sure
> that **pbs != NULL:
>
> assert(pbs != NULL);
Sure, why not.
>> @@ -1238,12 +1231,17 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>> options = qdict_new();
>> }
>>
>> + if (*pbs) {
>> + bs = *pbs;
>> + } else {
>> + bs = bdrv_new("");
>> + }
>> bs->options = options;
>> options = qdict_clone_shallow(options);
>>
>> /* For snapshot=on, create a temporary qcow2 overlay */
>> if (flags & BDRV_O_SNAPSHOT) {
>> - BlockDriverState *bs1;
>> + BlockDriverState *bs1 = NULL;
>> int64_t total_size;
>> BlockDriver *bdrv_qcow2;
>> QEMUOptionParameter *create_options;
>> @@ -1253,12 +1251,10 @@ 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,
>> + 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;
>> @@ -1386,6 +1382,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:
>> @@ -1399,13 +1396,20 @@ fail:
>> QDECREF(bs->options);
>> QDECREF(options);
>> bs->options = NULL;
>> + if (!*pbs) {
>> + bdrv_unref(bs);
>> + }
>> if (error_is_set(&local_err)) {
>> error_propagate(errp, local_err);
>> }
>> return ret;
>>
>> close_and_fail:
>> - bdrv_close(bs);
>> + if (*pbs) {
>> + bdrv_close(bs);
>> + } else {
>> + bdrv_unref(bs);
>> + }
> If *pbs is != NULL, and we reach an error case, we have to clean up
> the new BDS we created. This makes sense, but it may not be
> immediately obvious that *pbs = NULL means that *bs was allocated in
> this function. It may be worth a comment in the fail labels.
Yes, I'll add one.
Max
>> QDECREF(options);
>> if (error_is_set(&local_err)) {
>> error_propagate(errp, local_err);
>> @@ -5276,7 +5280,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
>> size = get_option_parameter(param, BLOCK_OPT_SIZE);
>> if (size && size->value.n == -1) {
>> if (backing_file && backing_file->value.s) {
>> - BlockDriverState *bs;
>> + BlockDriverState *bs = NULL;
>> uint64_t size;
>> char buf[32];
>> int back_flags;
>> @@ -5285,9 +5289,7 @@ 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,
>> + 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",
>> @@ -5295,7 +5297,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/qcow2.c b/block/qcow2.c
>> index 2da62b8..9a25fbd 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1541,7 +1541,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
>> @@ -1550,7 +1551,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);
>> @@ -1597,10 +1598,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)) {
>> @@ -1610,7 +1612,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 99ca60f..0fbf230 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..42163f8 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,11 @@ 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,
>> + 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 +1554,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;
>> @@ -1906,7 +1905,7 @@ void qmp_drive_backup(const char *device, const char *target,
>> Error **errp)
>> {
>> BlockDriverState *bs;
>> - BlockDriverState *target_bs;
>> + BlockDriverState *target_bs = NULL;
>> BlockDriverState *source = NULL;
>> BlockDriver *drv = NULL;
>> Error *local_err = NULL;
>> @@ -1991,10 +1990,8 @@ 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);
>> + ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err);
>> if (ret < 0) {
>> - bdrv_unref(target_bs);
>> error_propagate(errp, local_err);
>> return;
>> }
>> @@ -2027,7 +2024,7 @@ void qmp_drive_mirror(const char *device, const char *target,
>> Error **errp)
>> {
>> BlockDriverState *bs;
>> - BlockDriverState *source, *target_bs;
>> + BlockDriverState *source, *target_bs = NULL;
>> BlockDriver *drv = NULL;
>> Error *local_err = NULL;
>> int flags;
>> @@ -2135,11 +2132,9 @@ 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,
>> + 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..c39d486 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));
>> @@ -2314,7 +2314,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 +2324,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 d669028..2f06dc6 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.3
>>
>>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 01/10] block: Change BDS parameter of bdrv_open() to **
2014-01-26 19:02 ` [Qemu-devel] [PATCH 01/10] block: Change BDS parameter of bdrv_open() to ** Max Reitz
2014-01-27 2:38 ` Benoît Canet
2014-01-27 19:31 ` Jeff Cody
@ 2014-01-29 11:50 ` Kevin Wolf
2014-01-31 20:07 ` Max Reitz
2 siblings, 1 reply; 35+ messages in thread
From: Kevin Wolf @ 2014-01-29 11:50 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, Stefan Hajnoczi
Am 26.01.2014 um 20:02 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 | 53 ++++++++++++++++++++++++++-------------------------
> block/qcow2.c | 14 +++++++++-----
> block/vmdk.c | 5 ++---
> block/vvfat.c | 6 ++----
> blockdev.c | 21 ++++++++------------
> hw/block/xen_disk.c | 2 +-
> include/block/block.h | 2 +-
> qemu-img.c | 6 +++---
> qemu-io.c | 2 +-
> qemu-nbd.c | 2 +-
> 10 files changed, 55 insertions(+), 58 deletions(-)
>
> diff --git a/block.c b/block.c
> index cb21a5f..c660609 100644
> --- a/block.c
> +++ b/block.c
> @@ -1039,7 +1039,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);
In fact, we could use bdrv_new() only in the bdrv_open_common() case now
and leave it to bdrv_open() in the other one. Not sure if it makes
sense, though, I'll have to read the other patches first.
> @@ -1108,8 +1108,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);
> }
> @@ -1118,11 +1116,10 @@ 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,
> + 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",
This one made me think and I find it a bit worrying. I need to review
what the old value of bs->backing_hd is in order to find out if this was
a NULL argument that is already cleaned up or whether it needs to be
cleaned up here (this one is a NULL argument, so this code is fine).
In itself the behaviour is pretty consistent, you only have to free what
you explicity allocated previously. But I'm afraid that ownership of the
reference, especially in the failure case could confuse people.
Perhaps we should add an assert(bs->backing_hd == NULL) before such
bdrv_open() calls? It's not perfect, but maybe better than nothing.
> @@ -1189,8 +1186,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");
> @@ -1198,13 +1193,8 @@ 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;
> - }
> + *pbs = NULL;
> + ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp);
> } else {
> ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
> errp);
> @@ -1222,14 +1212,17 @@ 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 = NULL;
Why do you initialise bs here? It's not supposed to be used before the
first assignment, so let the compiler warn against it.
> const char *drvname;
> Error *local_err = NULL;
>
> @@ -1238,12 +1231,17 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> options = qdict_new();
> }
>
> + if (*pbs) {
> + bs = *pbs;
> + } else {
> + bs = bdrv_new("");
> + }
I think I'd prefer if you moved this above qdict_new(), so that all
option related code stays in one place.
> bs->options = options;
> options = qdict_clone_shallow(options);
>
> /* For snapshot=on, create a temporary qcow2 overlay */
> if (flags & BDRV_O_SNAPSHOT) {
> - BlockDriverState *bs1;
> + BlockDriverState *bs1 = NULL;
> int64_t total_size;
> BlockDriver *bdrv_qcow2;
> QEMUOptionParameter *create_options;
> @@ -1253,12 +1251,10 @@ 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,
> + 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;
> @@ -1386,6 +1382,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:
> @@ -1399,13 +1396,20 @@ fail:
> QDECREF(bs->options);
> QDECREF(options);
> bs->options = NULL;
> + if (!*pbs) {
> + bdrv_unref(bs);
> + }
> if (error_is_set(&local_err)) {
> error_propagate(errp, local_err);
> }
> return ret;
>
> close_and_fail:
> - bdrv_close(bs);
> + if (*pbs) {
> + bdrv_close(bs);
> + } else {
> + bdrv_unref(bs);
> + }
> QDECREF(options);
> if (error_is_set(&local_err)) {
> error_propagate(errp, local_err);
> @@ -5276,7 +5280,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
> size = get_option_parameter(param, BLOCK_OPT_SIZE);
> if (size && size->value.n == -1) {
> if (backing_file && backing_file->value.s) {
> - BlockDriverState *bs;
> + BlockDriverState *bs = NULL;
> uint64_t size;
> char buf[32];
> int back_flags;
> @@ -5285,9 +5289,7 @@ 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,
> + 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",
> @@ -5295,7 +5297,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/qcow2.c b/block/qcow2.c
> index 2da62b8..9a25fbd 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1541,7 +1541,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
> @@ -1550,7 +1551,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);
> @@ -1597,10 +1598,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)) {
> @@ -1610,7 +1612,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 99ca60f..0fbf230 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..42163f8 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,11 @@ 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,
> + 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 +1554,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;
> @@ -1906,7 +1905,7 @@ void qmp_drive_backup(const char *device, const char *target,
> Error **errp)
> {
> BlockDriverState *bs;
> - BlockDriverState *target_bs;
> + BlockDriverState *target_bs = NULL;
> BlockDriverState *source = NULL;
> BlockDriver *drv = NULL;
> Error *local_err = NULL;
> @@ -1991,10 +1990,8 @@ 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);
> + ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err);
> if (ret < 0) {
> - bdrv_unref(target_bs);
> error_propagate(errp, local_err);
> return;
> }
It might improve the readability if you leave the declaration without an
initialiser, but have an explicit target_bs = NULL immediately before
the bdrv_open.
> @@ -2027,7 +2024,7 @@ void qmp_drive_mirror(const char *device, const char *target,
> Error **errp)
> {
> BlockDriverState *bs;
> - BlockDriverState *source, *target_bs;
> + BlockDriverState *source, *target_bs = NULL;
> BlockDriver *drv = NULL;
> Error *local_err = NULL;
> int flags;
> @@ -2135,11 +2132,9 @@ 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,
> + 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;
> }
Same here.
> 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..c39d486 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));
> @@ -2314,7 +2314,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 +2324,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 d669028..2f06dc6 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],
So in summary, it looks technically correct, but I'm not sure about
maintainability. If the problems are only for this intermediate state,
I'm okay with leaving them as they are, but otherwise we should try and
figure out something.
Kevin
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH 01/10] block: Change BDS parameter of bdrv_open() to **
2014-01-29 11:50 ` Kevin Wolf
@ 2014-01-31 20:07 ` Max Reitz
2014-02-03 9:49 ` Kevin Wolf
0 siblings, 1 reply; 35+ messages in thread
From: Max Reitz @ 2014-01-31 20:07 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, Stefan Hajnoczi
On 29.01.2014 12:50, Kevin Wolf wrote:
> Am 26.01.2014 um 20:02 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 | 53 ++++++++++++++++++++++++++-------------------------
>> block/qcow2.c | 14 +++++++++-----
>> block/vmdk.c | 5 ++---
>> block/vvfat.c | 6 ++----
>> blockdev.c | 21 ++++++++------------
>> hw/block/xen_disk.c | 2 +-
>> include/block/block.h | 2 +-
>> qemu-img.c | 6 +++---
>> qemu-io.c | 2 +-
>> qemu-nbd.c | 2 +-
>> 10 files changed, 55 insertions(+), 58 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index cb21a5f..c660609 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1039,7 +1039,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);
> In fact, we could use bdrv_new() only in the bdrv_open_common() case now
> and leave it to bdrv_open() in the other one. Not sure if it makes
> sense, though, I'll have to read the other patches first.
There were some other cases such as blockdev_init() in which I didn't
want to completely change the existing code.
>
>> @@ -1108,8 +1108,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);
>> }
>> @@ -1118,11 +1116,10 @@ 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,
>> + 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",
> This one made me think and I find it a bit worrying. I need to review
> what the old value of bs->backing_hd is in order to find out if this was
> a NULL argument that is already cleaned up or whether it needs to be
> cleaned up here (this one is a NULL argument, so this code is fine).
>
> In itself the behaviour is pretty consistent, you only have to free what
> you explicity allocated previously. But I'm afraid that ownership of the
> reference, especially in the failure case could confuse people.
>
> Perhaps we should add an assert(bs->backing_hd == NULL) before such
> bdrv_open() calls? It's not perfect, but maybe better than nothing.
This seems like a fair compromise to me.
>> @@ -1189,8 +1186,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");
>> @@ -1198,13 +1193,8 @@ 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;
>> - }
>> + *pbs = NULL;
>> + ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp);
>> } else {
>> ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
>> errp);
>> @@ -1222,14 +1212,17 @@ 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 = NULL;
> Why do you initialise bs here? It's not supposed to be used before the
> first assignment, so let the compiler warn against it.
Good question, perhaps just a reflex induced by the "file = NULL" in
front of it. ;-)
>> const char *drvname;
>> Error *local_err = NULL;
>>
>> @@ -1238,12 +1231,17 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>> options = qdict_new();
>> }
>>
>> + if (*pbs) {
>> + bs = *pbs;
>> + } else {
>> + bs = bdrv_new("");
>> + }
> I think I'd prefer if you moved this above qdict_new(), so that all
> option related code stays in one place.
Okay.
>> bs->options = options;
>> options = qdict_clone_shallow(options);
>>
>> /* For snapshot=on, create a temporary qcow2 overlay */
>> if (flags & BDRV_O_SNAPSHOT) {
>> - BlockDriverState *bs1;
>> + BlockDriverState *bs1 = NULL;
>> int64_t total_size;
>> BlockDriver *bdrv_qcow2;
>> QEMUOptionParameter *create_options;
>> @@ -1253,12 +1251,10 @@ 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,
>> + 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;
>> @@ -1386,6 +1382,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:
>> @@ -1399,13 +1396,20 @@ fail:
>> QDECREF(bs->options);
>> QDECREF(options);
>> bs->options = NULL;
>> + if (!*pbs) {
>> + bdrv_unref(bs);
>> + }
>> if (error_is_set(&local_err)) {
>> error_propagate(errp, local_err);
>> }
>> return ret;
>>
>> close_and_fail:
>> - bdrv_close(bs);
>> + if (*pbs) {
>> + bdrv_close(bs);
>> + } else {
>> + bdrv_unref(bs);
>> + }
>> QDECREF(options);
>> if (error_is_set(&local_err)) {
>> error_propagate(errp, local_err);
>> @@ -5276,7 +5280,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
>> size = get_option_parameter(param, BLOCK_OPT_SIZE);
>> if (size && size->value.n == -1) {
>> if (backing_file && backing_file->value.s) {
>> - BlockDriverState *bs;
>> + BlockDriverState *bs = NULL;
>> uint64_t size;
>> char buf[32];
>> int back_flags;
>> @@ -5285,9 +5289,7 @@ 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,
>> + 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",
>> @@ -5295,7 +5297,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/qcow2.c b/block/qcow2.c
>> index 2da62b8..9a25fbd 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1541,7 +1541,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
>> @@ -1550,7 +1551,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);
>> @@ -1597,10 +1598,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)) {
>> @@ -1610,7 +1612,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 99ca60f..0fbf230 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..42163f8 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,11 @@ 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,
>> + 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 +1554,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;
>> @@ -1906,7 +1905,7 @@ void qmp_drive_backup(const char *device, const char *target,
>> Error **errp)
>> {
>> BlockDriverState *bs;
>> - BlockDriverState *target_bs;
>> + BlockDriverState *target_bs = NULL;
>> BlockDriverState *source = NULL;
>> BlockDriver *drv = NULL;
>> Error *local_err = NULL;
>> @@ -1991,10 +1990,8 @@ 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);
>> + ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err);
>> if (ret < 0) {
>> - bdrv_unref(target_bs);
>> error_propagate(errp, local_err);
>> return;
>> }
> It might improve the readability if you leave the declaration without an
> initialiser, but have an explicit target_bs = NULL immediately before
> the bdrv_open.
Yes, especially with using assert() where such an initialization is not
needed.
>> @@ -2027,7 +2024,7 @@ void qmp_drive_mirror(const char *device, const char *target,
>> Error **errp)
>> {
>> BlockDriverState *bs;
>> - BlockDriverState *source, *target_bs;
>> + BlockDriverState *source, *target_bs = NULL;
>> BlockDriver *drv = NULL;
>> Error *local_err = NULL;
>> int flags;
>> @@ -2135,11 +2132,9 @@ 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,
>> + 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;
>> }
> Same here.
>
>> 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..c39d486 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));
>> @@ -2314,7 +2314,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 +2324,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 d669028..2f06dc6 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],
> So in summary, it looks technically correct, but I'm not sure about
> maintainability. If the problems are only for this intermediate state,
> I'm okay with leaving them as they are, but otherwise we should try and
> figure out something.
You're right in that bdrv_open should always create a new BDS and new
BDS should always be created through bdrv_open; but right now, there are
some places in the code (such as blockdev_init()) which I was just
afraid of touching as they seemed to heavily rely on being able to
separate those two steps.
Max
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH 01/10] block: Change BDS parameter of bdrv_open() to **
2014-01-31 20:07 ` Max Reitz
@ 2014-02-03 9:49 ` Kevin Wolf
0 siblings, 0 replies; 35+ messages in thread
From: Kevin Wolf @ 2014-02-03 9:49 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, Stefan Hajnoczi
Am 31.01.2014 um 21:07 hat Max Reitz geschrieben:
> >So in summary, it looks technically correct, but I'm not sure about
> >maintainability. If the problems are only for this intermediate state,
> >I'm okay with leaving them as they are, but otherwise we should try and
> >figure out something.
>
> You're right in that bdrv_open should always create a new BDS and
> new BDS should always be created through bdrv_open; but right now,
> there are some places in the code (such as blockdev_init()) which I
> was just afraid of touching as they seemed to heavily rely on being
> able to separate those two steps.
Sure, there's a good reason why we work in incremental steps. This is a
huge conversion of infrastructure and trying to change it all at once
never worked out. We shouldn't worry too much about ugly intermediate
states as long as we're sure that they are really just intermediate.
Kevin
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH 02/10] block: Add reference parameter to bdrv_open()
2014-01-26 19:02 [Qemu-devel] [PATCH 00/10] block: Integrate bdrv_file_open() into bdrv_open() Max Reitz
2014-01-26 19:02 ` [Qemu-devel] [PATCH 01/10] block: Change BDS parameter of bdrv_open() to ** Max Reitz
@ 2014-01-26 19:02 ` Max Reitz
2014-01-26 19:02 ` [Qemu-devel] [PATCH 03/10] block: Make bdrv_file_open() static Max Reitz
` (7 subsequent siblings)
9 siblings, 0 replies; 35+ messages in thread
From: Max Reitz @ 2014-01-26 19:02 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, 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 c660609..f9923bb 100644
--- a/block.c
+++ b/block.c
@@ -1039,7 +1039,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);
@@ -1117,7 +1117,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
BDRV_O_COPY_ON_READ);
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;
@@ -1194,7 +1194,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
}
*pbs = NULL;
- 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);
@@ -1216,8 +1216,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. */
@@ -1231,6 +1232,32 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
options = qdict_new();
}
+ if (reference) {
+ bool options_non_empty = qdict_size(options);
+ 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 {
@@ -1252,7 +1279,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
/* Get the required size from the image */
QINCREF(options);
- 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;
@@ -5289,7 +5316,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
back_flags =
flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
- 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 9a25fbd..7202a26 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1551,7 +1551,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);
@@ -1602,7 +1602,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 0fbf230..37b2bc8 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 42163f8..64cf476 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",
@@ -1303,7 +1303,7 @@ static void external_snapshot_prepare(BlkTransactionState *common,
/* TODO Inherit bs->options or only take explicit options with an
* extended QMP command? */
- 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) {
@@ -1554,7 +1554,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;
@@ -1990,7 +1990,7 @@ void qmp_drive_backup(const char *device, const char *target,
return;
}
- 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;
@@ -2132,8 +2132,8 @@ void qmp_drive_mirror(const char *device, const char *target,
/* Mirroring takes care of copy-on-write using the source's backing
* file.
*/
- 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 c39d486..2357dee 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));
@@ -2314,7 +2314,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",
@@ -2324,8 +2324,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 2f06dc6..10b38ba 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.3
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [PATCH 03/10] block: Make bdrv_file_open() static
2014-01-26 19:02 [Qemu-devel] [PATCH 00/10] block: Integrate bdrv_file_open() into bdrv_open() Max Reitz
2014-01-26 19:02 ` [Qemu-devel] [PATCH 01/10] block: Change BDS parameter of bdrv_open() to ** Max Reitz
2014-01-26 19:02 ` [Qemu-devel] [PATCH 02/10] block: Add reference parameter to bdrv_open() Max Reitz
@ 2014-01-26 19:02 ` Max Reitz
2014-01-27 2:51 ` Benoît Canet
2014-01-29 13:26 ` Kevin Wolf
2014-01-26 19:02 ` [Qemu-devel] [PATCH 04/10] block: Reuse NULL options check from bdrv_open() Max Reitz
` (6 subsequent siblings)
9 siblings, 2 replies; 35+ messages in thread
From: Max Reitz @ 2014-01-26 19:02 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, 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 | 5 ++---
qemu-io.c | 4 +++-
10 files changed, 44 insertions(+), 28 deletions(-)
diff --git a/block.c b/block.c
index f9923bb..0fb7892 100644
--- a/block.c
+++ b/block.c
@@ -947,9 +947,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;
@@ -1196,8 +1196,9 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
*pbs = NULL;
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:
@@ -1227,6 +1228,12 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
const char *drvname;
Error *local_err = NULL;
+ if (flags & BDRV_O_PROTOCOL) {
+ assert(!drv);
+ return bdrv_file_open(pbs, filename, reference, options,
+ flags & ~BDRV_O_PROTOCOL, errp);
+ }
+
/* NULL means an empty set of options */
if (options == NULL) {
options = qdict_new();
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 7202a26..40b957b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1479,7 +1479,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;
@@ -1491,7 +1491,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 694e6e2..dee2e61 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 9ee0a61..13513b4 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 37b2bc8..d2a69d5 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..396f9ed 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -102,6 +102,8 @@ 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 /* open the file using a protocol instead of
+ a block driver */
#define BDRV_O_CACHE_MASK (BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)
@@ -183,9 +185,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 10b38ba..ad606d1 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.3
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH 03/10] block: Make bdrv_file_open() static
2014-01-26 19:02 ` [Qemu-devel] [PATCH 03/10] block: Make bdrv_file_open() static Max Reitz
@ 2014-01-27 2:51 ` Benoît Canet
2014-01-29 13:26 ` Kevin Wolf
1 sibling, 0 replies; 35+ messages in thread
From: Benoît Canet @ 2014-01-27 2:51 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
Le Sunday 26 Jan 2014 à 20:02:36 (+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 | 5 ++---
> qemu-io.c | 4 +++-
> 10 files changed, 44 insertions(+), 28 deletions(-)
>
> diff --git a/block.c b/block.c
> index f9923bb..0fb7892 100644
> --- a/block.c
> +++ b/block.c
> @@ -947,9 +947,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;
> @@ -1196,8 +1196,9 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
> *pbs = NULL;
> 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:
> @@ -1227,6 +1228,12 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
> const char *drvname;
> Error *local_err = NULL;
>
> + if (flags & BDRV_O_PROTOCOL) {
> + assert(!drv);
> + return bdrv_file_open(pbs, filename, reference, options,
> + flags & ~BDRV_O_PROTOCOL, errp);
> + }
> +
> /* NULL means an empty set of options */
> if (options == NULL) {
> options = qdict_new();
> 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 7202a26..40b957b 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1479,7 +1479,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;
> @@ -1491,7 +1491,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 694e6e2..dee2e61 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 9ee0a61..13513b4 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 37b2bc8..d2a69d5 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..396f9ed 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -102,6 +102,8 @@ 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 /* open the file using a protocol instead of
> + a block driver */
>
> #define BDRV_O_CACHE_MASK (BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)
>
> @@ -183,9 +185,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 10b38ba..ad606d1 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.3
>
>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH 03/10] block: Make bdrv_file_open() static
2014-01-26 19:02 ` [Qemu-devel] [PATCH 03/10] block: Make bdrv_file_open() static Max Reitz
2014-01-27 2:51 ` Benoît Canet
@ 2014-01-29 13:26 ` Kevin Wolf
2014-01-31 20:20 ` Max Reitz
1 sibling, 1 reply; 35+ messages in thread
From: Kevin Wolf @ 2014-01-29 13:26 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, Stefan Hajnoczi
Am 26.01.2014 um 20:02 hat Max Reitz geschrieben:
> 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 | 5 ++---
> qemu-io.c | 4 +++-
> 10 files changed, 44 insertions(+), 28 deletions(-)
>
> diff --git a/block.c b/block.c
> index f9923bb..0fb7892 100644
> --- a/block.c
> +++ b/block.c
> @@ -947,9 +947,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;
> @@ -1196,8 +1196,9 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
> *pbs = NULL;
> 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:
> @@ -1227,6 +1228,12 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
> const char *drvname;
> Error *local_err = NULL;
>
> + if (flags & BDRV_O_PROTOCOL) {
> + assert(!drv);
> + return bdrv_file_open(pbs, filename, reference, options,
> + flags & ~BDRV_O_PROTOCOL, errp);
> + }
> +
> /* NULL means an empty set of options */
> if (options == NULL) {
> options = qdict_new();
> 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 7202a26..40b957b 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1479,7 +1479,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;
> @@ -1491,7 +1491,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 694e6e2..dee2e61 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 9ee0a61..13513b4 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 37b2bc8..d2a69d5 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..396f9ed 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -102,6 +102,8 @@ 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 /* open the file using a protocol instead of
> + a block driver */
Protocol drivers are a subset of block drivers, so this description
doesn't make sense.
I guess we need to list the differences between bdrv_open() and
bdrv_file_open() in order to define what this flag really changes. I
think this includes:
- Disables format probing
- BDRV_O_SNAPSHOT is ignored
- No backing files are opened
- Probably a few more
Kevin
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH 03/10] block: Make bdrv_file_open() static
2014-01-29 13:26 ` Kevin Wolf
@ 2014-01-31 20:20 ` Max Reitz
2014-02-03 10:05 ` Kevin Wolf
0 siblings, 1 reply; 35+ messages in thread
From: Max Reitz @ 2014-01-31 20:20 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, Stefan Hajnoczi
On 29.01.2014 14:26, Kevin Wolf wrote:
> Am 26.01.2014 um 20:02 hat Max Reitz geschrieben:
>> 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 | 5 ++---
>> qemu-io.c | 4 +++-
>> 10 files changed, 44 insertions(+), 28 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index f9923bb..0fb7892 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -947,9 +947,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;
>> @@ -1196,8 +1196,9 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>> *pbs = NULL;
>> 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:
>> @@ -1227,6 +1228,12 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>> const char *drvname;
>> Error *local_err = NULL;
>>
>> + if (flags & BDRV_O_PROTOCOL) {
>> + assert(!drv);
>> + return bdrv_file_open(pbs, filename, reference, options,
>> + flags & ~BDRV_O_PROTOCOL, errp);
>> + }
>> +
>> /* NULL means an empty set of options */
>> if (options == NULL) {
>> options = qdict_new();
>> 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 7202a26..40b957b 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1479,7 +1479,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;
>> @@ -1491,7 +1491,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 694e6e2..dee2e61 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 9ee0a61..13513b4 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 37b2bc8..d2a69d5 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..396f9ed 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -102,6 +102,8 @@ 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 /* open the file using a protocol instead of
>> + a block driver */
> Protocol drivers are a subset of block drivers, so this description
> doesn't make sense.
Hm, technically they probably are but it always seemed to me that
bdrv_open() would never directly use a protocol, instead using raw as
the format if no format was found.
More importantly, it is actually possible to use a non-protocol block
driver with BDRV_O_PROTOCOL; it just needs to be explicitly specified.
> I guess we need to list the differences between bdrv_open() and
> bdrv_file_open() in order to define what this flag really changes. I
> think this includes:
>
> - Disables format probing
> - BDRV_O_SNAPSHOT is ignored
> - No backing files are opened
> - Probably a few more
So, to me, the main difference is that bdrv_open() always uses some
non-protocol block driver, whereas bdrv_file_open() only probes for
protocol block drivers (if a non-protocol driver should be used, it has
to be explicitly specified).
The current comment for bdrv_file_open() doesn't help much, either:
“Opens a file using a protocol”. Therefore, the easiest way would just
be to state “behaves like the old bdrv_file_open()”, but that would not
be very helpful. Perhaps we could formulate it like “Opens a single file
(no backing chain, etc.) using only a protocol driver deduced from the
filename, if not explicitly specified otherwise.”
Max
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH 03/10] block: Make bdrv_file_open() static
2014-01-31 20:20 ` Max Reitz
@ 2014-02-03 10:05 ` Kevin Wolf
0 siblings, 0 replies; 35+ messages in thread
From: Kevin Wolf @ 2014-02-03 10:05 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, Stefan Hajnoczi
Am 31.01.2014 um 21:20 hat Max Reitz geschrieben:
> On 29.01.2014 14:26, Kevin Wolf wrote:
> >Am 26.01.2014 um 20:02 hat Max Reitz geschrieben:
> >>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 | 5 ++---
> >> qemu-io.c | 4 +++-
> >> 10 files changed, 44 insertions(+), 28 deletions(-)
> >>diff --git a/include/block/block.h b/include/block/block.h
> >>index a421041..396f9ed 100644
> >>--- a/include/block/block.h
> >>+++ b/include/block/block.h
> >>@@ -102,6 +102,8 @@ 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 /* open the file using a protocol instead of
> >>+ a block driver */
> >Protocol drivers are a subset of block drivers, so this description
> >doesn't make sense.
>
> Hm, technically they probably are but it always seemed to me that
> bdrv_open() would never directly use a protocol, instead using raw
> as the format if no format was found.
Except if you explicitly specify something like format=file.
> More importantly, it is actually possible to use a non-protocol
> block driver with BDRV_O_PROTOCOL; it just needs to be explicitly
> specified.
Yes, I think with explicit specification of the driver you get mostly
the same results with bdrv_open() and bdrv_file_open().
> >I guess we need to list the differences between bdrv_open() and
> >bdrv_file_open() in order to define what this flag really changes. I
> >think this includes:
> >
> >- Disables format probing
> >- BDRV_O_SNAPSHOT is ignored
> >- No backing files are opened
> >- Probably a few more
>
> So, to me, the main difference is that bdrv_open() always uses some
> non-protocol block driver, whereas bdrv_file_open() only probes for
> protocol block drivers (if a non-protocol driver should be used, it
> has to be explicitly specified).
>
> The current comment for bdrv_file_open() doesn't help much, either:
> “Opens a file using a protocol”. Therefore, the easiest way would
> just be to state “behaves like the old bdrv_file_open()”, but that
> would not be very helpful. Perhaps we could formulate it like “Opens
> a single file (no backing chain, etc.) using only a protocol driver
> deduced from the filename, if not explicitly specified otherwise.”
Perhaps we should really specify it as the difference in probing:
- bdrv_open() adds a probed format layer on top of bs
- bdrv_file_open() probes protocols for bs
All other differences can probably go away without breaking anything:
BDRV_O_SNAPSHOT can be hopefully be moved to drive_init() (the tricky one
here is qmp_change_blockdev), and supporting backing files in
bdrv_file_open() by moving their handling to common code shouldn't hurt.
Any other differences that need to be eliminated? Once we know
what the differences should be, I guess we can greatly simplify the
implementation.
Kevin
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH 04/10] block: Reuse NULL options check from bdrv_open()
2014-01-26 19:02 [Qemu-devel] [PATCH 00/10] block: Integrate bdrv_file_open() into bdrv_open() Max Reitz
` (2 preceding siblings ...)
2014-01-26 19:02 ` [Qemu-devel] [PATCH 03/10] block: Make bdrv_file_open() static Max Reitz
@ 2014-01-26 19:02 ` Max Reitz
2014-01-27 2:52 ` Benoît Canet
2014-01-26 19:02 ` [Qemu-devel] [PATCH 05/10] block: Reuse reference handling " Max Reitz
` (5 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Max Reitz @ 2014-01-26 19:02 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Max Reitz
Remove the check whether options is NULL form bdrv_file_open() and rely
on the one in bdrv_open() instead.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/block.c b/block.c
index 0fb7892..c7219cb 100644
--- a/block.c
+++ b/block.c
@@ -958,11 +958,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
Error *local_err = NULL;
int ret;
- /* NULL means an empty set of options */
- if (options == NULL) {
- options = qdict_new();
- }
-
if (reference) {
if (filename || qdict_size(options)) {
error_setg(errp, "Cannot reference an existing block device with "
@@ -1228,17 +1223,17 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
const char *drvname;
Error *local_err = NULL;
+ /* NULL means an empty set of options */
+ if (options == NULL) {
+ options = qdict_new();
+ }
+
if (flags & BDRV_O_PROTOCOL) {
assert(!drv);
return bdrv_file_open(pbs, filename, reference, options,
flags & ~BDRV_O_PROTOCOL, errp);
}
- /* NULL means an empty set of options */
- if (options == NULL) {
- options = qdict_new();
- }
-
if (reference) {
bool options_non_empty = qdict_size(options);
QDECREF(options);
--
1.8.5.3
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH 04/10] block: Reuse NULL options check from bdrv_open()
2014-01-26 19:02 ` [Qemu-devel] [PATCH 04/10] block: Reuse NULL options check from bdrv_open() Max Reitz
@ 2014-01-27 2:52 ` Benoît Canet
0 siblings, 0 replies; 35+ messages in thread
From: Benoît Canet @ 2014-01-27 2:52 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
Le Sunday 26 Jan 2014 à 20:02:37 (+0100), Max Reitz a écrit :
> Remove the check whether options is NULL form bdrv_file_open() and rely
s/form/for/g ?
> on the one in bdrv_open() instead.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/block.c b/block.c
> index 0fb7892..c7219cb 100644
> --- a/block.c
> +++ b/block.c
> @@ -958,11 +958,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> Error *local_err = NULL;
> int ret;
>
> - /* NULL means an empty set of options */
> - if (options == NULL) {
> - options = qdict_new();
> - }
> -
> if (reference) {
> if (filename || qdict_size(options)) {
> error_setg(errp, "Cannot reference an existing block device with "
> @@ -1228,17 +1223,17 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
> const char *drvname;
> Error *local_err = NULL;
>
> + /* NULL means an empty set of options */
> + if (options == NULL) {
> + options = qdict_new();
> + }
> +
> if (flags & BDRV_O_PROTOCOL) {
> assert(!drv);
> return bdrv_file_open(pbs, filename, reference, options,
> flags & ~BDRV_O_PROTOCOL, errp);
> }
>
> - /* NULL means an empty set of options */
> - if (options == NULL) {
> - options = qdict_new();
> - }
> -
> if (reference) {
> bool options_non_empty = qdict_size(options);
> QDECREF(options);
> --
> 1.8.5.3
>
>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH 05/10] block: Reuse reference handling from bdrv_open()
2014-01-26 19:02 [Qemu-devel] [PATCH 00/10] block: Integrate bdrv_file_open() into bdrv_open() Max Reitz
` (3 preceding siblings ...)
2014-01-26 19:02 ` [Qemu-devel] [PATCH 04/10] block: Reuse NULL options check from bdrv_open() Max Reitz
@ 2014-01-26 19:02 ` Max Reitz
2014-01-27 2:56 ` Benoît Canet
2014-01-26 19:02 ` [Qemu-devel] [PATCH 06/10] block: Remove bdrv_new() from bdrv_file_open() Max Reitz
` (4 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Max Reitz @ 2014-01-26 19:02 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, 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>
---
block.c | 33 +++++++--------------------------
1 file changed, 7 insertions(+), 26 deletions(-)
diff --git a/block.c b/block.c
index c7219cb..6c29115 100644
--- a/block.c
+++ b/block.c
@@ -948,8 +948,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;
@@ -958,24 +957,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
Error *local_err = NULL;
int ret;
- 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);
@@ -1228,12 +1209,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
options = qdict_new();
}
- 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 = qdict_size(options);
QDECREF(options);
@@ -1260,6 +1235,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.3
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH 05/10] block: Reuse reference handling from bdrv_open()
2014-01-26 19:02 ` [Qemu-devel] [PATCH 05/10] block: Reuse reference handling " Max Reitz
@ 2014-01-27 2:56 ` Benoît Canet
0 siblings, 0 replies; 35+ messages in thread
From: Benoît Canet @ 2014-01-27 2:56 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
Le Sunday 26 Jan 2014 à 20:02:38 (+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>
> ---
> block.c | 33 +++++++--------------------------
> 1 file changed, 7 insertions(+), 26 deletions(-)
>
> diff --git a/block.c b/block.c
> index c7219cb..6c29115 100644
> --- a/block.c
> +++ b/block.c
> @@ -948,8 +948,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;
> @@ -958,24 +957,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> Error *local_err = NULL;
> int ret;
>
> - 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);
> @@ -1228,12 +1209,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
> options = qdict_new();
> }
>
> - 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 = qdict_size(options);
> QDECREF(options);
> @@ -1260,6 +1235,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.3
>
>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH 06/10] block: Remove bdrv_new() from bdrv_file_open()
2014-01-26 19:02 [Qemu-devel] [PATCH 00/10] block: Integrate bdrv_file_open() into bdrv_open() Max Reitz
` (4 preceding siblings ...)
2014-01-26 19:02 ` [Qemu-devel] [PATCH 05/10] block: Reuse reference handling " Max Reitz
@ 2014-01-26 19:02 ` Max Reitz
2014-01-27 3:04 ` Benoît Canet
2014-01-29 13:35 ` Kevin Wolf
2014-01-26 19:02 ` [Qemu-devel] [PATCH 07/10] block: Reuse fail path from bdrv_open() Max Reitz
` (3 subsequent siblings)
9 siblings, 2 replies; 35+ messages in thread
From: Max Reitz @ 2014-01-26 19:02 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, 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 | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/block.c b/block.c
index 6c29115..72eddd5 100644
--- a/block.c
+++ b/block.c
@@ -947,17 +947,15 @@ 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;
Error *local_err = NULL;
int ret;
- bs = bdrv_new("");
bs->options = options;
options = qdict_clone_shallow(options);
@@ -1036,7 +1034,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
QDECREF(options);
bs->growable = 1;
- *pbs = bs;
return 0;
fail:
@@ -1044,7 +1041,6 @@ fail:
if (!bs->drv) {
QDECREF(bs->options);
}
- bdrv_unref(bs);
return ret;
}
@@ -1235,17 +1231,28 @@ 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) {
+ if (*pbs) {
+ bdrv_close(bs);
+ } else {
+ bdrv_unref(bs);
+ }
+ } else {
+ *pbs = bs;
+ }
+ return ret;
+ }
+
bs->options = options;
options = qdict_clone_shallow(options);
--
1.8.5.3
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH 06/10] block: Remove bdrv_new() from bdrv_file_open()
2014-01-26 19:02 ` [Qemu-devel] [PATCH 06/10] block: Remove bdrv_new() from bdrv_file_open() Max Reitz
@ 2014-01-27 3:04 ` Benoît Canet
2014-01-29 13:35 ` Kevin Wolf
1 sibling, 0 replies; 35+ messages in thread
From: Benoît Canet @ 2014-01-27 3:04 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
Le Sunday 26 Jan 2014 à 20:02:39 (+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 | 29 ++++++++++++++++++-----------
> 1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/block.c b/block.c
> index 6c29115..72eddd5 100644
> --- a/block.c
> +++ b/block.c
> @@ -947,17 +947,15 @@ 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;
> Error *local_err = NULL;
> int ret;
>
> - bs = bdrv_new("");
> bs->options = options;
> options = qdict_clone_shallow(options);
>
> @@ -1036,7 +1034,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> QDECREF(options);
>
> bs->growable = 1;
> - *pbs = bs;
> return 0;
>
> fail:
> @@ -1044,7 +1041,6 @@ fail:
> if (!bs->drv) {
> QDECREF(bs->options);
> }
> - bdrv_unref(bs);
> return ret;
> }
>
> @@ -1235,17 +1231,28 @@ 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) {
> + if (*pbs) {
> + bdrv_close(bs);
> + } else {
> + bdrv_unref(bs);
> + }
Can the goto chain be factorized with this snippet of code: something like this
is already in it ?
Something like
if (!ret) {
return ret;
}
goto exit_close_or_unref;
> + } else {
> + *pbs = bs;
> + }
> + return ret;
> + }
> +
> bs->options = options;
> options = qdict_clone_shallow(options);
>
> --
> 1.8.5.3
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH 06/10] block: Remove bdrv_new() from bdrv_file_open()
2014-01-26 19:02 ` [Qemu-devel] [PATCH 06/10] block: Remove bdrv_new() from bdrv_file_open() Max Reitz
2014-01-27 3:04 ` Benoît Canet
@ 2014-01-29 13:35 ` Kevin Wolf
2014-01-31 20:21 ` Max Reitz
1 sibling, 1 reply; 35+ messages in thread
From: Kevin Wolf @ 2014-01-29 13:35 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, Stefan Hajnoczi
Am 26.01.2014 um 20:02 hat Max Reitz geschrieben:
> 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 | 29 ++++++++++++++++++-----------
> 1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/block.c b/block.c
> index 6c29115..72eddd5 100644
> --- a/block.c
> +++ b/block.c
> @@ -947,17 +947,15 @@ 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;
> Error *local_err = NULL;
> int ret;
>
> - bs = bdrv_new("");
> bs->options = options;
> options = qdict_clone_shallow(options);
>
> @@ -1036,7 +1034,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> QDECREF(options);
>
> bs->growable = 1;
> - *pbs = bs;
> return 0;
>
> fail:
> @@ -1044,7 +1041,6 @@ fail:
> if (!bs->drv) {
> QDECREF(bs->options);
> }
> - bdrv_unref(bs);
> return ret;
> }
>
> @@ -1235,17 +1231,28 @@ 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) {
> + if (*pbs) {
> + bdrv_close(bs);
Why would bs be open when bdrv_file_open() just failed?
> + } else {
> + bdrv_unref(bs);
> + }
> + } else {
> + *pbs = bs;
> + }
> + return ret;
> + }
> +
> bs->options = options;
> options = qdict_clone_shallow(options);
Kevin
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH 06/10] block: Remove bdrv_new() from bdrv_file_open()
2014-01-29 13:35 ` Kevin Wolf
@ 2014-01-31 20:21 ` Max Reitz
0 siblings, 0 replies; 35+ messages in thread
From: Max Reitz @ 2014-01-31 20:21 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, Stefan Hajnoczi
On 29.01.2014 14:35, Kevin Wolf wrote:
> Am 26.01.2014 um 20:02 hat Max Reitz geschrieben:
>> 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 | 29 ++++++++++++++++++-----------
>> 1 file changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 6c29115..72eddd5 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -947,17 +947,15 @@ 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;
>> Error *local_err = NULL;
>> int ret;
>>
>> - bs = bdrv_new("");
>> bs->options = options;
>> options = qdict_clone_shallow(options);
>>
>> @@ -1036,7 +1034,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>> QDECREF(options);
>>
>> bs->growable = 1;
>> - *pbs = bs;
>> return 0;
>>
>> fail:
>> @@ -1044,7 +1041,6 @@ fail:
>> if (!bs->drv) {
>> QDECREF(bs->options);
>> }
>> - bdrv_unref(bs);
>> return ret;
>> }
>>
>> @@ -1235,17 +1231,28 @@ 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) {
>> + if (*pbs) {
>> + bdrv_close(bs);
> Why would bs be open when bdrv_file_open() just failed?
Good question. *g*
I'll remove this. ;-)
Max
>> + } else {
>> + bdrv_unref(bs);
>> + }
>> + } else {
>> + *pbs = bs;
>> + }
>> + return ret;
>> + }
>> +
>> bs->options = options;
>> options = qdict_clone_shallow(options);
> Kevin
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH 07/10] block: Reuse fail path from bdrv_open()
2014-01-26 19:02 [Qemu-devel] [PATCH 00/10] block: Integrate bdrv_file_open() into bdrv_open() Max Reitz
` (5 preceding siblings ...)
2014-01-26 19:02 ` [Qemu-devel] [PATCH 06/10] block: Remove bdrv_new() from bdrv_file_open() Max Reitz
@ 2014-01-26 19:02 ` Max Reitz
2014-01-27 3:10 ` Benoît Canet
2014-01-29 13:40 ` Kevin Wolf
2014-01-26 19:02 ` [Qemu-devel] [PATCH 08/10] block: Reuse bs->options setting " Max Reitz
` (2 subsequent siblings)
9 siblings, 2 replies; 35+ messages in thread
From: Max Reitz @ 2014-01-26 19:02 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, 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.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/block.c b/block.c
index 72eddd5..0f2cd3f 100644
--- a/block.c
+++ b/block.c
@@ -1038,9 +1038,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
fail:
QDECREF(options);
- if (!bs->drv) {
- QDECREF(bs->options);
- }
return ret;
}
@@ -1240,17 +1237,17 @@ 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,
- errp);
+ &local_err);
+ options = NULL;
if (ret) {
- if (*pbs) {
- bdrv_close(bs);
+ if (bs->drv) {
+ goto close_and_fail;
} else {
- bdrv_unref(bs);
+ goto fail;
}
- } else {
- *pbs = bs;
}
- return ret;
+ *pbs = bs;
+ return 0;
}
bs->options = options;
--
1.8.5.3
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH 07/10] block: Reuse fail path from bdrv_open()
2014-01-26 19:02 ` [Qemu-devel] [PATCH 07/10] block: Reuse fail path from bdrv_open() Max Reitz
@ 2014-01-27 3:10 ` Benoît Canet
2014-01-27 18:58 ` Max Reitz
2014-01-29 13:40 ` Kevin Wolf
1 sibling, 1 reply; 35+ messages in thread
From: Benoît Canet @ 2014-01-27 3:10 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
Le Sunday 26 Jan 2014 à 20:02:40 (+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.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block.c | 17 +++++++----------
> 1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/block.c b/block.c
> index 72eddd5..0f2cd3f 100644
> --- a/block.c
> +++ b/block.c
> @@ -1038,9 +1038,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>
> fail:
> QDECREF(options);
> - if (!bs->drv) {
> - QDECREF(bs->options);
> - }
> return ret;
> }
>
> @@ -1240,17 +1237,17 @@ 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,
> - errp);
> + &local_err);
> + options = NULL;
> if (ret) {
> - if (*pbs) {
> - bdrv_close(bs);
> + if (bs->drv) {
> + goto close_and_fail;
> } else {
> - bdrv_unref(bs);
> + goto fail;
> }
> - } else {
> - *pbs = bs;
> }
Forget what I said about the goto chain in the previous mail.
But something like:
if(!ret) {
*pbs = bs;
return 0;
}
if (bs->drv) {
goto close_and_fail;
}
goto fail;
would keep the code in line.
Best regards
Benoît
> + *pbs = bs;
> + return 0;
> }
>
> bs->options = options;
> --
> 1.8.5.3
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH 07/10] block: Reuse fail path from bdrv_open()
2014-01-27 3:10 ` Benoît Canet
@ 2014-01-27 18:58 ` Max Reitz
0 siblings, 0 replies; 35+ messages in thread
From: Max Reitz @ 2014-01-27 18:58 UTC (permalink / raw)
To: Benoît Canet; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On 27.01.2014 04:10, Benoît Canet wrote:
> Le Sunday 26 Jan 2014 à 20:02:40 (+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.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> block.c | 17 +++++++----------
>> 1 file changed, 7 insertions(+), 10 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 72eddd5..0f2cd3f 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1038,9 +1038,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>>
>> fail:
>> QDECREF(options);
>> - if (!bs->drv) {
>> - QDECREF(bs->options);
>> - }
>> return ret;
>> }
>>
>> @@ -1240,17 +1237,17 @@ 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,
>> - errp);
>> + &local_err);
>> + options = NULL;
>> if (ret) {
>> - if (*pbs) {
>> - bdrv_close(bs);
>> + if (bs->drv) {
>> + goto close_and_fail;
>> } else {
>> - bdrv_unref(bs);
>> + goto fail;
>> }
>> - } else {
>> - *pbs = bs;
>> }
> Forget what I said about the goto chain in the previous mail.
>
> But something like:
>
> if(!ret) {
> *pbs = bs;
> return 0;
> }
> if (bs->drv) {
> goto close_and_fail;
> }
> goto fail;
>
> would keep the code in line.
>
> Best regards
>
> Benoît
Yes, this seems better to me as well. :-)
Or, after patch 9:
if (!ret) {
goto done;
} else if (bs->drv) {
goto close_and_fail;
} else {
goto fail;
}
(which I like a bit more, with the three gotos on the same indentation
level)
Max
>> + *pbs = bs;
>> + return 0;
>> }
>>
>> bs->options = options;
>> --
>> 1.8.5.3
>>
>>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 07/10] block: Reuse fail path from bdrv_open()
2014-01-26 19:02 ` [Qemu-devel] [PATCH 07/10] block: Reuse fail path from bdrv_open() Max Reitz
2014-01-27 3:10 ` Benoît Canet
@ 2014-01-29 13:40 ` Kevin Wolf
1 sibling, 0 replies; 35+ messages in thread
From: Kevin Wolf @ 2014-01-29 13:40 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, Stefan Hajnoczi
Am 26.01.2014 um 20:02 hat Max Reitz geschrieben:
> 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.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
I find this change confusing because it is bdrv_file_open() that
takes ownership of bs->options and assigns it. So I think it should be
bdrv_file_open() that frees it again in failure cases.
Kevin
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH 08/10] block: Reuse bs->options setting from bdrv_open()
2014-01-26 19:02 [Qemu-devel] [PATCH 00/10] block: Integrate bdrv_file_open() into bdrv_open() Max Reitz
` (6 preceding siblings ...)
2014-01-26 19:02 ` [Qemu-devel] [PATCH 07/10] block: Reuse fail path from bdrv_open() Max Reitz
@ 2014-01-26 19:02 ` Max Reitz
2014-01-27 3:13 ` Benoît Canet
2014-01-29 13:45 ` Kevin Wolf
2014-01-26 19:02 ` [Qemu-devel] [PATCH 09/10] block: Reuse success path " Max Reitz
2014-01-26 19:02 ` [Qemu-devel] [PATCH 10/10] block: Remove bdrv_open_image()'s force_raw option Max Reitz
9 siblings, 2 replies; 35+ messages in thread
From: Max Reitz @ 2014-01-26 19:02 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Max Reitz
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 | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/block.c b/block.c
index 0f2cd3f..f847c4b 100644
--- a/block.c
+++ b/block.c
@@ -956,9 +956,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
Error *local_err = NULL;
int ret;
- 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");
@@ -1234,6 +1231,9 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
bs = bdrv_new("");
}
+ 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,
@@ -1250,9 +1250,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
return 0;
}
- bs->options = options;
- options = qdict_clone_shallow(options);
-
/* For snapshot=on, create a temporary qcow2 overlay */
if (flags & BDRV_O_SNAPSHOT) {
BlockDriverState *bs1 = NULL;
--
1.8.5.3
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH 08/10] block: Reuse bs->options setting from bdrv_open()
2014-01-26 19:02 ` [Qemu-devel] [PATCH 08/10] block: Reuse bs->options setting " Max Reitz
@ 2014-01-27 3:13 ` Benoît Canet
2014-01-29 13:45 ` Kevin Wolf
1 sibling, 0 replies; 35+ messages in thread
From: Benoît Canet @ 2014-01-27 3:13 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
Le Sunday 26 Jan 2014 à 20:02:41 (+0100), Max Reitz a écrit :
> 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 | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/block.c b/block.c
> index 0f2cd3f..f847c4b 100644
> --- a/block.c
> +++ b/block.c
> @@ -956,9 +956,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> Error *local_err = NULL;
> int ret;
>
> - 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");
> @@ -1234,6 +1231,9 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
> bs = bdrv_new("");
> }
>
> + 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,
> @@ -1250,9 +1250,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
> return 0;
> }
>
> - bs->options = options;
> - options = qdict_clone_shallow(options);
> -
> /* For snapshot=on, create a temporary qcow2 overlay */
> if (flags & BDRV_O_SNAPSHOT) {
> BlockDriverState *bs1 = NULL;
> --
> 1.8.5.3
>
>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH 08/10] block: Reuse bs->options setting from bdrv_open()
2014-01-26 19:02 ` [Qemu-devel] [PATCH 08/10] block: Reuse bs->options setting " Max Reitz
2014-01-27 3:13 ` Benoît Canet
@ 2014-01-29 13:45 ` Kevin Wolf
2014-01-31 20:25 ` Max Reitz
1 sibling, 1 reply; 35+ messages in thread
From: Kevin Wolf @ 2014-01-29 13:45 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, Stefan Hajnoczi
Am 26.01.2014 um 20:02 hat Max Reitz geschrieben:
> 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>
Perhaps squash this into patch 7? Because now the reference is created
by bdrv_open() instead of bdrv_file_open().
Also my suggestion in a reply to a patch earlier in this series (keeping
all option-related code at one place), probably means that it all needs
to be done together (or immediately after) handling the NULL options
case.
Kevin
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 08/10] block: Reuse bs->options setting from bdrv_open()
2014-01-29 13:45 ` Kevin Wolf
@ 2014-01-31 20:25 ` Max Reitz
0 siblings, 0 replies; 35+ messages in thread
From: Max Reitz @ 2014-01-31 20:25 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, Stefan Hajnoczi
On 29.01.2014 14:45, Kevin Wolf wrote:
> Am 26.01.2014 um 20:02 hat Max Reitz geschrieben:
>> 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>
> Perhaps squash this into patch 7? Because now the reference is created
> by bdrv_open() instead of bdrv_file_open().
Yes, why not.
> Also my suggestion in a reply to a patch earlier in this series (keeping
> all option-related code at one place), probably means that it all needs
> to be done together (or immediately after) handling the NULL options
> case.
I'll have a look.
Thank you for reviewing.
Max
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH 09/10] block: Reuse success path from bdrv_open()
2014-01-26 19:02 [Qemu-devel] [PATCH 00/10] block: Integrate bdrv_file_open() into bdrv_open() Max Reitz
` (7 preceding siblings ...)
2014-01-26 19:02 ` [Qemu-devel] [PATCH 08/10] block: Reuse bs->options setting " Max Reitz
@ 2014-01-26 19:02 ` Max Reitz
2014-01-27 17:44 ` Jeff Cody
2014-01-26 19:02 ` [Qemu-devel] [PATCH 10/10] block: Remove bdrv_open_image()'s force_raw option Max Reitz
9 siblings, 1 reply; 35+ messages in thread
From: Max Reitz @ 2014-01-26 19:02 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, 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 | 33 ++++++++++++---------------------
1 file changed, 12 insertions(+), 21 deletions(-)
diff --git a/block.c b/block.c
index f847c4b..b1bae23 100644
--- a/block.c
+++ b/block.c
@@ -943,9 +943,7 @@ 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.
+ * empty set of options.
*/
static int bdrv_file_open(BlockDriverState *bs, const char *filename,
QDict *options, int flags, Error **errp)
@@ -1010,8 +1008,8 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
}
if (!drv->bdrv_file_open) {
+ QINCREF(options);
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);
}
@@ -1020,21 +1018,10 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
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;
}
@@ -1238,7 +1225,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
assert(!drv);
ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
&local_err);
- options = NULL;
if (ret) {
if (bs->drv) {
goto close_and_fail;
@@ -1246,8 +1232,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
goto fail;
}
}
- *pbs = bs;
- return 0;
+ goto done;
}
/* For snapshot=on, create a temporary qcow2 overlay */
@@ -1377,12 +1362,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
}
}
+done:
/* Check if any unknown options were used */
if (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.3
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH 09/10] block: Reuse success path from bdrv_open()
2014-01-26 19:02 ` [Qemu-devel] [PATCH 09/10] block: Reuse success path " Max Reitz
@ 2014-01-27 17:44 ` Jeff Cody
2014-01-27 19:06 ` Max Reitz
0 siblings, 1 reply; 35+ messages in thread
From: Jeff Cody @ 2014-01-27 17:44 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On Sun, Jan 26, 2014 at 08:02:42PM +0100, Max Reitz wrote:
> 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 | 33 ++++++++++++---------------------
> 1 file changed, 12 insertions(+), 21 deletions(-)
>
> diff --git a/block.c b/block.c
> index f847c4b..b1bae23 100644
> --- a/block.c
> +++ b/block.c
> @@ -943,9 +943,7 @@ 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.
> + * empty set of options.
> */
> static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> QDict *options, int flags, Error **errp)
> @@ -1010,8 +1008,8 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> }
>
> if (!drv->bdrv_file_open) {
> + QINCREF(options);
> 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);
> }
> @@ -1020,21 +1018,10 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> 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;
> }
>
> @@ -1238,7 +1225,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
> assert(!drv);
> ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
> &local_err);
> - options = NULL;
> if (ret) {
> if (bs->drv) {
> goto close_and_fail;
> @@ -1246,8 +1232,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
> goto fail;
> }
> }
> - *pbs = bs;
> - return 0;
> + goto done;
> }
>
> /* For snapshot=on, create a temporary qcow2 overlay */
> @@ -1377,12 +1362,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
> }
> }
>
> +done:
> /* Check if any unknown options were used */
> if (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);
Tests 071 and 072 segfault, and gdb shows that it occurs here. More
investigation shows that entry is NULL, although qdict_size() is
returning 1.
> + } 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.3
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH 09/10] block: Reuse success path from bdrv_open()
2014-01-27 17:44 ` Jeff Cody
@ 2014-01-27 19:06 ` Max Reitz
0 siblings, 0 replies; 35+ messages in thread
From: Max Reitz @ 2014-01-27 19:06 UTC (permalink / raw)
To: Jeff Cody; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On 27.01.2014 18:44, Jeff Cody wrote:
> On Sun, Jan 26, 2014 at 08:02:42PM +0100, Max Reitz wrote:
>> 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 | 33 ++++++++++++---------------------
>> 1 file changed, 12 insertions(+), 21 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index f847c4b..b1bae23 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -943,9 +943,7 @@ 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.
>> + * empty set of options.
>> */
>> static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>> QDict *options, int flags, Error **errp)
>> @@ -1010,8 +1008,8 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>> }
>>
>> if (!drv->bdrv_file_open) {
>> + QINCREF(options);
>> 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);
>> }
>> @@ -1020,21 +1018,10 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>> 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;
>> }
>>
>> @@ -1238,7 +1225,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>> assert(!drv);
>> ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
>> &local_err);
>> - options = NULL;
>> if (ret) {
>> if (bs->drv) {
>> goto close_and_fail;
>> @@ -1246,8 +1232,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>> goto fail;
>> }
>> }
>> - *pbs = bs;
>> - return 0;
>> + goto done;
>> }
>>
>> /* For snapshot=on, create a temporary qcow2 overlay */
>> @@ -1377,12 +1362,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>> }
>> }
>>
>> +done:
>> /* Check if any unknown options were used */
>> if (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);
> Tests 071 and 072 segfault, and gdb shows that it occurs here. More
> investigation shows that entry is NULL, although qdict_size() is
> returning 1.
Hm, and I thought I'd run the tests, but obviously I did not for this
final version… The problem is probably the QINCREF(). Hm, I really want
to reuse this test; the only way I see then is to make the options
parameter of bdrv_file_open() an indirect pointer so it can be set to
NULL after bdrv_open() (in bdrv_file_open()). That would not be very
pretty, but it would work and there wouldn't be two tests whether all
options could be handled.
Thanks for catching this,
Max
>> + } 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.3
>>
>>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH 10/10] block: Remove bdrv_open_image()'s force_raw option
2014-01-26 19:02 [Qemu-devel] [PATCH 00/10] block: Integrate bdrv_file_open() into bdrv_open() Max Reitz
` (8 preceding siblings ...)
2014-01-26 19:02 ` [Qemu-devel] [PATCH 09/10] block: Reuse success path " Max Reitz
@ 2014-01-26 19:02 ` Max Reitz
9 siblings, 0 replies; 35+ messages in thread
From: Max Reitz @ 2014-01-26 19:02 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, 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 | 30 +++++-------------------------
block/blkdebug.c | 2 +-
block/blkverify.c | 4 ++--
include/block/block.h | 2 +-
4 files changed, 9 insertions(+), 29 deletions(-)
diff --git a/block.c b/block.c
index b1bae23..7f02ff1 100644
--- a/block.c
+++ b/block.c
@@ -1096,10 +1096,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.
*
@@ -1112,7 +1108,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;
@@ -1135,24 +1131,8 @@ 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;
- }
-
- *pbs = NULL;
- 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);
- }
+ *pbs = NULL;
+ ret = bdrv_open(pbs, filename, reference, image_options, flags, NULL, errp);
done:
qdict_del(options, bdref_key);
@@ -1308,8 +1288,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
}
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 56c4cd0..5696ef6 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -411,7 +411,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
/* Open the backing file */
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 fail;
diff --git a/block/blkverify.c b/block/blkverify.c
index cfcbcf4..fe94b59 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -136,7 +136,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
/* Open the raw file */
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;
@@ -144,7 +144,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
/* Open the test file */
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 396f9ed..9bf8ef1 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -187,7 +187,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.3
^ permalink raw reply related [flat|nested] 35+ messages in thread