The Friday 12 Sep 2014 à 21:26:21 (+0200), 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));
> 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);
>
> if (ret < 0) {
> error_setg(errp, "could not open disk image %s: %s",
> @@ -511,8 +514,9 @@ static DriveInfo *blockdev_init(const char *file, QDict
> *bs_opts,
> goto err;
> }
>
> - if (bdrv_key_required(dinfo->bdrv))
> + if (bdrv_key_required(bs)) {
> autostart = 0;
> + }
>
> QDECREF(bs_opts);
> qemu_opts_del(opts);
> @@ -520,9 +524,8 @@ static DriveInfo *blockdev_init(const char *file, QDict
> *bs_opts,
> return dinfo;
>
> err:
> - bdrv_unref(dinfo->bdrv);
> + bdrv_unref(bs);
I would have moved this.
> QTAILQ_REMOVE(&drives, dinfo, next);
> -bdrv_new_err:
> g_free(dinfo->id);
> g_free(dinfo);
To Here.
No functional difference but it would reflect it's goto chain role:
being in the reverse order of the allocations.
> early_err:
> --
> 1.9.3
Reviewed-by: Benoît Canet <benoit.ca...@nodalink.com>
>
