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

[Max Reitz]

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 <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));

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