Re: [Qemu-devel] [PATCH 1/4] blockdev: Disentangle BlockDriverState and DriveInfo creation

[Benoît Canet]

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 <address@hidden>
> ---
>  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 <address@hidden>
>