Re: [Qemu-devel] [PATCH v3 10/10] block: Avoid bdrv_get_geometry() where errors should be detected

[Markus Armbruster]

Benoît Canet <address@hidden> writes:

> The Friday 30 May 2014 à 20:13:51 (+0200), Markus Armbruster wrote :
>> bdrv_get_geometry() hides errors.  Use bdrv_nb_sectors() or
>> bdrv_getlength() instead where that's obviously inappropriate.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> Reviewed-by: Eric Blake <address@hidden>
>> Reviewed-by: Max Reitz <address@hidden>
>> ---
>>  block.c      | 11 ++++++++---
>>  block/qapi.c | 14 ++++++++++----
>>  qemu-img.c   | 59 
>> +++++++++++++++++++++++++++++++++++++++++++++--------------
>>  3 files changed, 63 insertions(+), 21 deletions(-)
>> 
>> diff --git a/block.c b/block.c
>> index 41837b4..94f8b87 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -5573,7 +5573,7 @@ void bdrv_img_create(const char *filename, const char 
>> *fmt,
>>      if (size && size->value.n == -1) {
>>          if (backing_file && backing_file->value.s) {
>>              BlockDriverState *bs;
>> -            uint64_t size;
>> +            int64_t size;
>>              char buf[32];
>>              int back_flags;
>>  
>> @@ -5592,8 +5592,13 @@ void bdrv_img_create(const char *filename, const char 
>> *fmt,
>>                  local_err = NULL;
>>                  goto out;
>>              }
>> -            bdrv_get_geometry(bs, &size);
>> -            size *= 512;
>> +            size = bdrv_getlength(bs);
>> +            if (size < 0) {
>> +                error_setg_errno(errp, -size, "Could not get size of '%s'",
>> +                                 backing_file->value.s);
>> +                bdrv_unref(bs);
>> +                goto out;
>> +            }
>>  
>>              snprintf(buf, sizeof(buf), "%" PRId64, size);
>>              set_option_parameter(param, BLOCK_OPT_SIZE, buf);
>> diff --git a/block/qapi.c b/block/qapi.c
>> index 75f44f1..ae6488a 100644
>> --- a/block/qapi.c
>> +++ b/block/qapi.c
>> @@ -165,19 +165,25 @@ void bdrv_query_image_info(BlockDriverState *bs,
>>                             ImageInfo **p_info,
>>                             Error **errp)
>>  {
>> -    uint64_t total_sectors;
>> +    int64_t size;
>>      const char *backing_filename;
>>      char backing_filename2[1024];
>>      BlockDriverInfo bdi;
>>      int ret;
>>      Error *err = NULL;
>> -    ImageInfo *info = g_new0(ImageInfo, 1);
>> +    ImageInfo *info;
>>  
>> -    bdrv_get_geometry(bs, &total_sectors);
>> +    size = bdrv_getlength(bs);
>> +    if (size < 0) {
>> +        error_setg_errno(errp, -size, "Can't get size of device '%s'",
>> +                         bdrv_get_device_name(bs));
>> +        return;
>> +    }
>>  
>> +    info = g_new0(ImageInfo, 1);
>>      info->filename        = g_strdup(bs->filename);
>>      info->format          = g_strdup(bdrv_get_format_name(bs));
>> -    info->virtual_size    = total_sectors * 512;
>> +    info->virtual_size    = size;
>>      info->actual_size     = bdrv_get_allocated_file_size(bs);
>>      info->has_actual_size = info->actual_size >= 0;
>>      if (bdrv_is_encrypted(bs)) {
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 2cb56c5..fca58bc 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -959,7 +959,6 @@ static int img_compare(int argc, char **argv)
>>      int64_t sector_num = 0;
>>      int64_t nb_sectors;
>>      int c, pnum;
>> -    uint64_t bs_sectors;
>>      uint64_t progress_base;
>>  
>>      for (;;) {
>> @@ -1021,10 +1020,18 @@ static int img_compare(int argc, char **argv)
>>  
>>      buf1 = qemu_blockalign(bs1, IO_BUF_SIZE);
>>      buf2 = qemu_blockalign(bs2, IO_BUF_SIZE);
>> -    bdrv_get_geometry(bs1, &bs_sectors);
>> -    total_sectors1 = bs_sectors;
>> -    bdrv_get_geometry(bs2, &bs_sectors);
>> -    total_sectors2 = bs_sectors;
>> +    total_sectors1 = bdrv_nb_sectors(bs1);
>> +    if (total_sectors1 < 0) {
>> +        error_report("Can't get size of %s: %s",
>> +                     filename1, strerror(-total_sectors1));
>> +        goto out;
>> +    }
>> +    total_sectors2 = bdrv_nb_sectors(bs2);
>> +    if (total_sectors2 < 0) {
>> +        error_report("Can't get size of %s: %s",
>> +                     filename2, strerror(-total_sectors2));
>> +        goto out;
>> +    }
> Would setting ret be useful beforie goto out ? 

Yes, to a value > 1.  But which one?  We use 2, 3 and 4, but their
documented only as ">1 - Error occured".

>>      total_sectors = MIN(total_sectors1, total_sectors2);
>>      progress_base = MAX(total_sectors1, total_sectors2);
>>  
[...]

Thanks!