Benoît Canet <benoit.ca...@nodalink.com> writes:
>> --- a/block.c
>> +++ b/block.c
>> @@ -2119,10 +2119,11 @@ static void bdrv_delete(BlockDriverState *bs)
>>
>> bdrv_close(bs);
>>
>
>
>> + drive_info_del(drive_get_by_blockdev(bs));
>> +
>> /* remove from list, if necessary */
>> bdrv_make_anon(bs);
>>
>> - drive_info_del(drive_get_by_blockdev(bs));
>
> Do we really want this move ?
Yes, we do. If bdrv_make_anon() runs before drive_info_del(), this
conditional in drive_info_del() is always false:
if (dinfo->bdrv->device_name[0]) {
blk_unref(blk_by_name(dinfo->bdrv->device_name));
}
I apologize for the hairiness. Things will become *way* simpler in
PATCH 4.
> Or is it an artefact of your work on the preparation series ?
I added the line in "[PATCH 2/4] block: Keep DriveInfo alive until
BlockDriverState dies" in the wrong place for this patch, so I have to
move it here. When I noticed, the patch was already out. If I have to
respin, I'll avoid the churn.
>> g_free(bs);
>> }
>>
>
>> + * Return the BlockBackend with name @name if it exists, else null.
>
>> + * @name must not be null.
> The contract is that no one will call blk_by_name(NULL);
>
>> + */
>> +BlockBackend *blk_by_name(const char *name)
>> +{
>> + BlockBackend *blk;
>
> Can we ?
>
> assert(name);
Sure, why not.
>> +
>> + QTAILQ_FOREACH(blk, &blk_backends, link) {
>> + if (!strcmp(name, blk->name)) {
>> + return blk;
>> + }
>> + }
>> + return NULL;
>> +}
[...]
>> @@ -227,6 +228,15 @@ void drive_info_del(DriveInfo *dinfo)
>> if (dinfo->opts) {
>> qemu_opts_del(dinfo->opts);
>> }
>> + /*
>> + * Hairy special case: if do_drive_del() has made dinfo->bdrv
>> + * anonymous, it also unref'ed the associated BlockBackend.
>> + */
>
> Then you are filling the other case here.
>
> maybe
>
> /*
> * Hairy special case: if do_drive_del() has made dinfo->bdrv
> * anonymous, it also unref'ed the associated BlockBackend.
> * Process the other case here.
> */
>
> would further explain you intents.
Okay.
>> + if (dinfo->bdrv->device_name[0]) {
>> + blk_unref(blk_by_name(dinfo->bdrv->device_name));
>> + }
>> +
>> g_free(dinfo->id);
>> QTAILQ_REMOVE(&drives, dinfo, next);
>> g_free(dinfo->serial);
>> @@ -307,6 +317,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;
>> + BlockBackend *blk;
>> BlockDriverState *bs;
>> DriveInfo *dinfo;
>> ThrottleConfig cfg;
>> @@ -463,9 +474,13 @@ static DriveInfo *blockdev_init(const char *file, QDict
>> *bs_opts,
>> }
>
>> const char *fmt1 = NULL, *fmt2 = NULL, *cache, *filename1, *filename2;
>> + BlockBackend *blk1, *blk2;
>> BlockDriverState *bs1, *bs2;
>> int64_t total_sectors1, total_sectors2;
>> uint8_t *buf1 = NULL, *buf2 = NULL;
>> @@ -1011,18 +1022,20 @@ static int img_compare(int argc, char **argv)
>> goto out3;
>> }
>>
>> + blk1 = blk_new("image 1", &error_abort);
>> bs1 = bdrv_new_open("image 1", filename1, fmt1, flags, true, quiet);
>> if (!bs1) {
>> error_report("Can't open file %s", filename1);
>> ret = 2;
>> - goto out3;
>> + goto out2;
>
> Here we allocate blk1 and bs1 an if bs1 is null (error) we jump to
> out2 which is:
>
>> out2:
>> bdrv_unref(bs1);
>> + blk_unref(blk1);
>
> It's a bit strange that we will bdrv_unref(NULL) in this case.
>
> This goto chain seems odd.
You can solve the "on error, destroy the things already created" problem
like this:
Foo *foo;
Bar *bar;
Baz *baz;
[...]
foo = new_foo();
if (!foo) {
goto out_foo;
}
bar = new_bar();
if (!bar) {
goto out_bar;
}
baz = new_baz();
if (!baz) {
goto out_baz;
}
[...]
out_baz:
del_baz(baz);
out_bar:
del_bar(bar);
out_foo:
del_foo(foo);
Works, but keeping track of where exactly to go to is somewhat tedious
and error prone.
If the del_ functions do nothing for null arguments, the following also
works:
Foo *foo = NULL;
Bar *bar = NULL;
Baz *baz = NULL;
[...]
foo = new_foo();
if (!foo) {
goto out;
}
bar = new_bar();
if (!bar) {
goto out;
}
baz = new_baz();
if (!baz) {
goto out;
}
[...]
out:
del_baz(baz);
del_bar(bar);
del_foo(foo);
Simpler. This is the reason why free(NULL) does nothing.
>> }
>>
>> + blk2 = blk_new("image 2", &error_abort);
>> bs2 = bdrv_new_open("image 2", filename2, fmt2, flags, true, quiet);
>> if (!bs2) {
>> error_report("Can't open file %s", filename2);
>> ret = 2;
>> - goto out2;
>> + goto out1;
>> }
>>
>> buf1 = qemu_blockalign(bs1, IO_BUF_SIZE);
>> @@ -1183,11 +1196,14 @@ static int img_compare(int argc, char **argv)
>> ret = 0;
>>
>> out:
>> - bdrv_unref(bs2);
>> qemu_vfree(buf1);
>> qemu_vfree(buf2);
>> +out1:
>> + bdrv_unref(bs2);
>> + blk_unref(blk2);
>> out2:
>> bdrv_unref(bs1);
>> + blk_unref(blk1);
>> out3:
>> qemu_progress_end();
>> return ret;
>> @@ -1200,6 +1216,7 @@ static int img_convert(int argc, char **argv)
>> int progress = 0, flags, src_flags;
>> const char *fmt, *out_fmt, *cache, *src_cache, *out_baseimg,
>> *out_filename;
>> BlockDriver *drv, *proto_drv;
>> + BlockBackend **blk = NULL, *out_blk = NULL;
>
> Should blk be blks or blkes ? They are multiple.
I follow the bs precedence (next line).
In general, I feel using plural for array names doesn't work so well,
because most uses tend to be subscripted, and there plural feels
awkward.
>> BlockDriverState **bs = NULL, *out_bs = NULL;
>> int64_t total_sectors, nb_sectors, sector_num, bs_offset;
>> int64_t *bs_sectors = NULL;
[...]
