Re: [Qemu-devel] [PATCH v2 02/23] block: New BlockBackend

[Markus Armbruster]

Benoît Canet <address@hidden> 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;
[...]