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

[Markus Armbruster]

Max Reitz <address@hidden> writes:

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

When I made block use g_new() & friends, I only converted patterns like
p = g_malloc(sizeof(T)), not patterns like p = g_malloc(sizeof(*p)).

In the former case, p = g_new(T) is a clear improvement, because now the
compiler checks T matches typeof(*p).

In the latter case, we trade some visible obviousness for type safety.
Matter of taste.

If we agree to prefer type safety in block land, I'll gladly do the
conversion work.

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

Indeed.  bdrv_open() is in another file, and its function comment
doesn't quite spell out this part of its contract.

Assertions do double-duty: they check assumptions are valid, and they
remind the reader of assumptions.  The second part can be useful even
when the first part is trivial.

> With or without g_new0:
>
> Reviewed-by: Max Reitz <address@hidden>

Thanks!