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

[Markus Armbruster <armbru@redhat.com>]

Benoît Canet <benoit.canet@irqsave.net> writes:

> 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 <armbru@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.

You're right.

In the BlockBackend series, this becomes just

    err:
        blk_unref(blk);
    early_err:

so the disorder is just temporary.  I'll change it anyway if I have to
respin for some other reason.

>>  early_err:
>> -- 
>> 1.9.3
>
> Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>

Thanks!