Benoît Canet <benoit.ca...@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 <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.
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.ca...@nodalink.com>
Thanks!
