Re: [Qemu-devel] [PATCH v2 06/23] block: Make BlockBackend own its BlockDriverState

[Markus Armbruster]

Benoît Canet <address@hidden> writes:

>>          /* blkdev->bs is not create by us, we get a reference
>>           * so we can bdrv_unref() unconditionally */
>> -        bdrv_ref(blkdev->bs);
>
>> +        /* Except we don't bdrv_unref() anymore, we blk_unref().
> Is this dot extra ?                                               ^

No, I wrote two sentences intentionally.

The whole comment gets replaced in PATCH 14.

> The following line seems to make better sense without this dot and the
> initial cap.
>> +         * Conditionally, because we can't easily blk_ref() here.
>
>> @@ -712,9 +711,7 @@ static int img_check(int argc, char **argv)
>>  
>>  fail:
>>      qapi_free_ImageCheck(check);
>> -    bdrv_unref(bs);
>>      blk_unref(blk);
>> -
> Spurious blank line removal here.

Intentional.  Without it, the code looks like this:

    if (check->corruptions) {
        ret = 2;
    } else if (check->leaks) {
        ret = 3;
    } else {
        ret = 0;
    }

fail:
    qapi_free_ImageCheck(check);
    blk_unref(blk);

    return ret;
}

I don't think the blank line helps readability here.

>> @@ -483,9 +481,6 @@ int main(int argc, char **argv)
>>       */
>>      bdrv_drain_all();
>>  
>> -    if (qemuio_bs) {
>> -        bdrv_unref(qemuio_bs);
>> -    }
>>      blk_unref(qemuio_blk);
>>      g_free(readline_state);
>>      return 0;
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index fa8a7d0..0c496af 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -771,7 +771,6 @@ int main(int argc, char **argv)
>
> Well that does seems to simplify things a lot.

:)