On 12.09.2014 21:26, Markus Armbruster wrote:
blockdev_init() mixes up BlockDriverState and DriveInfo initialization
Finish the BlockDriverState job before starting to mess with
DriveInfo. Easier on the eyes.
Signed-off-by: Markus Armbruster <arm...@redhat.com>
---
blockdev.c | 43 +++++++++++++++++++++++--------------------
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index b361fbb..5ec4635 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -301,6 +301,7 @@ static DriveInfo *blockdev_init(const char *file, QDict
*bs_opts,
int ro = 0;
int bdrv_flags = 0;
int on_read_error, on_write_error;
+ BlockDriverState *bs;
DriveInfo *dinfo;
ThrottleConfig cfg;
int snapshot = 0;
@@ -456,26 +457,27 @@ static DriveInfo *blockdev_init(const char *file, QDict
*bs_opts,
}
/* init */
+ bs = bdrv_new(qemu_opts_id(opts), errp);
+ if (!bs) {
+ goto early_err;
+ }
+ bs->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
+ bs->read_only = ro;
+ bs->detect_zeroes = detect_zeroes;
+
+ bdrv_set_on_error(bs, on_read_error, on_write_error);
+
+ /* disk I/O throttling */
+ if (throttle_enabled(&cfg)) {
+ bdrv_io_limits_enable(bs);
+ bdrv_set_io_limits(bs, &cfg);
+ }
+
dinfo = g_malloc0(sizeof(*dinfo));
Could've changed this to g_new0 in the process, but you're the expert
for that, so I'll leave it up to you. ;-)
dinfo->id = g_strdup(qemu_opts_id(opts));
- dinfo->bdrv = bdrv_new(dinfo->id, &error);
- if (error) {
- error_propagate(errp, error);
- goto bdrv_new_err;
- }
- dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
- dinfo->bdrv->read_only = ro;
- dinfo->bdrv->detect_zeroes = detect_zeroes;
+ dinfo->bdrv = bs;
QTAILQ_INSERT_TAIL(&drives, dinfo, next);
- bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error);
-
- /* disk I/O throttling */
- if (throttle_enabled(&cfg)) {
- bdrv_io_limits_enable(dinfo->bdrv);
- bdrv_set_io_limits(dinfo->bdrv, &cfg);
- }
-
if (!file || !*file) {
if (has_driver_specific_opts) {
file = NULL;
@@ -502,7 +504,8 @@ 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, NULL, bs_opts, bdrv_flags, drv,
&error);
+ ret = bdrv_open(&bs, file, NULL, bs_opts, bdrv_flags, drv, &error);
+ assert(bs == dinfo->bdrv);
Well, this is guaranteed by bdrv_open(), but of course better having too
many assertions than too few.
With or without g_new0:
Reviewed-by: Max Reitz <mre...@redhat.com>
