Re: [Qemu-devel] [PATCH v3 07/14] qemu-img: Use BlockBackend as far as possible

[Max Reitz <mreitz@redhat.com>]

On 2015-01-26 at 22:38, Eric Blake wrote:
> On 01/26/2015 08:00 AM, Max Reitz wrote:
>> Although qemu-img already creates BlockBackends, it does not do accesses
>> to the images through them. This patch converts all of the bdrv_* calls
>> for which this is currently possible to blk_* calls. Most of the
>> remaining calls will probably stay bdrv_* calls because they really do
>> operate on the BDS level instead of the BB level.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   qemu-img.c | 98 ++++++++++++++++++++++++++++++++++----------------------------
>>   1 file changed, 54 insertions(+), 44 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 0b23c87..8b4139e 100644
>> --- a/qemu-img.c
>> @@ -1130,22 +1130,26 @@ static int img_compare(int argc, char **argv)
>>       }
>>       bs2 = blk_bs(blk2);
>>   
>> -    buf1 = qemu_blockalign(bs1, IO_BUF_SIZE);
>> -    buf2 = qemu_blockalign(bs2, IO_BUF_SIZE);
>> -    total_sectors1 = bdrv_nb_sectors(bs1);
>> +    buf1 = blk_blockalign(blk1, IO_BUF_SIZE);
>> +    buf2 = blk_blockalign(blk2, IO_BUF_SIZE);
>> +    total_sectors1 = blk_getlength(blk1);
>>       if (total_sectors1 < 0) {
>>           error_report("Can't get size of %s: %s",
>>                        filename1, strerror(-total_sectors1));
>>           ret = 4;
>>           goto out;
>>       }
>> -    total_sectors2 = bdrv_nb_sectors(bs2);
>> +    total_sectors2 = blk_getlength(blk2);
> The naming feels awkward; your conversion is now using bytes while the
> old code was using sectors, so 'total_sectors2' feels weird...
>
>>       if (total_sectors2 < 0) {
>>           error_report("Can't get size of %s: %s",
>>                        filename2, strerror(-total_sectors2));
>>           ret = 4;
>>           goto out;
>>       }
>> +
>> +    total_sectors1 /= BDRV_SECTOR_SIZE;
>> +    total_sectors2 /= BDRV_SECTOR_SIZE;
> ...at least you end up converting to sectors after all.  But it makes me
> wonder if you should have blk_nb_sectors(), and/or temporary
> intermediate variables to avoid cross-unit confusion.
>
>> @@ -1476,13 +1480,14 @@ static int img_convert(int argc, char **argv)
>>               goto out;
>>           }
>>           bs[bs_i] = blk_bs(blk[bs_i]);
>> -        bs_sectors[bs_i] = bdrv_nb_sectors(bs[bs_i]);
>> +        bs_sectors[bs_i] = blk_getlength(blk[bs_i]);
>>           if (bs_sectors[bs_i] < 0) {
>>               error_report("Could not get size of %s: %s",
>>                            argv[optind + bs_i], strerror(-bs_sectors[bs_i]));
>>               ret = -1;
>>               goto out;
>>           }
>> +        bs_sectors[bs_i] /= BDRV_SECTOR_SIZE;
> Another instance of the confusion.
>
>>           total_sectors += bs_sectors[bs_i];
>>       }
>>   
>> @@ -1625,16 +1630,19 @@ static int img_convert(int argc, char **argv)
>>                                            out_bs->bl.discard_alignment))
>>                       );
>>   
>> -    buf = qemu_blockalign(out_bs, bufsectors * BDRV_SECTOR_SIZE);
>> +    buf = blk_blockalign(out_blk, bufsectors * BDRV_SECTOR_SIZE);
>>   
>>       if (skip_create) {
>> -        int64_t output_sectors = bdrv_nb_sectors(out_bs);
>> +        int64_t output_sectors = blk_getlength(out_blk);
>>           if (output_sectors < 0) {
>>               error_report("unable to get output image length: %s\n",
>>                            strerror(-output_sectors));
>>               ret = -1;
>>               goto out;
>> -        } else if (output_sectors < total_sectors) {
>> +        }
>> +
>> +        output_sectors /= BDRV_SECTOR_SIZE;
>> +        if (output_sectors < total_sectors) {
> And another.
>
>> @@ -2585,17 +2591,17 @@ static int img_rebase(int argc, char **argv)
>>           uint8_t * buf_new;
>>           float local_progress = 0;
>>   
>> -        buf_old = qemu_blockalign(bs, IO_BUF_SIZE);
>> -        buf_new = qemu_blockalign(bs, IO_BUF_SIZE);
>> +        buf_old = blk_blockalign(blk, IO_BUF_SIZE);
>> +        buf_new = blk_blockalign(blk, IO_BUF_SIZE);
>>   
>> -        num_sectors = bdrv_nb_sectors(bs);
>> +        num_sectors = blk_getlength(blk);
>>           if (num_sectors < 0) {
> ...
>
>> -        if (bs_new_backing) {
>> -            new_backing_num_sectors = bdrv_nb_sectors(bs_new_backing);
>> +
>> +        num_sectors /= BDRV_SECTOR_SIZE;
>> +        old_backing_num_sectors /= BDRV_SECTOR_SIZE;
> and another.
>
> I did not closely audit if there were any other conversions that should
> have been made.  Also, I suspect that a blk_nb_sectors() as a pre-req
> patch would make this one feel cleaner if you respin and rebase.

The reason why I don't want to add blk_nb_sectors() is simply that we 
want to convert all the code to using bytes at a later point anyway, so 
I felt like it'd be a step backwards to introduce blk_nb_sectors().

However, I do see your point and intermediate variables probably don't 
make this any nicer. Also, if we get to convert the code to bytes, 
finding all instances of {blk,bdrv}_nb_sectors() will be one of the 
easier tasks, so I'll just introduce blk_nb_sectors().

Max

> But if
> we don't add blk_nb_sectors(), at least this version of the patch
> appears to be clean with what it does, so you can consider this to be a
> rather weak:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>