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

[Markus Armbruster]

Benoît Canet <address@hidden> writes:

> The Wednesday 04 Jun 2014 à 13:51: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>
[...]
>> diff --git a/qemu-img.c b/qemu-img.c
>> index e6d0edf..7e6dde0 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -958,7 +958,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 (;;) {
>> @@ -1020,10 +1019,20 @@ 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));
>> +        ret = 4;
>> +        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));
>> +        ret = 4;
>> +        goto out;
>> +    }
>>      total_sectors = MIN(total_sectors1, total_sectors2);
>>      progress_base = MAX(total_sectors1, total_sectors2);
>>  
>> @@ -1185,7 +1194,7 @@ static int img_convert(int argc, char **argv)
>>      BlockDriver *drv, *proto_drv;
>>      BlockDriverState **bs = NULL, *out_bs = NULL;
>>      int64_t total_sectors, nb_sectors, sector_num, bs_offset;
>> -    uint64_t *bs_sectors = NULL;
>> +    int64_t *bs_sectors = NULL;
>>      uint8_t * buf = NULL;
>>      size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE;
>>      const uint8_t *buf1;
>> @@ -1326,7 +1335,7 @@ static int img_convert(int argc, char **argv)
>>      qemu_progress_print(0, 100);
>>  
>>      bs = g_new0(BlockDriverState *, bs_n);
>> -    bs_sectors = g_new(uint64_t, bs_n);
>> +    bs_sectors = g_new(int64_t, bs_n);
>>  
>>      total_sectors = 0;
>>      for (bs_i = 0; bs_i < bs_n; bs_i++) {
>> @@ -1340,7 +1349,13 @@ static int img_convert(int argc, char **argv)
>>              ret = -1;
>>              goto out;
>>          }
>> -        bdrv_get_geometry(bs[bs_i], &bs_sectors[bs_i]);
>> +        bs_sectors[bs_i] = bdrv_nb_sectors(bs[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;
>> +        }
>>          total_sectors += bs_sectors[bs_i];
>>      }
>>  
>> @@ -2421,9 +2436,9 @@ static int img_rebase(int argc, char **argv)
>>       * the image is the same as the original one at any time.
>>       */
>>      if (!unsafe) {
>> -        uint64_t num_sectors;
>> -        uint64_t old_backing_num_sectors;
>> -        uint64_t new_backing_num_sectors = 0;
>> +        int64_t num_sectors;
>> +        int64_t old_backing_num_sectors;
>> +        int64_t new_backing_num_sectors = 0;
>>          uint64_t sector;
>>          int n;
>>          uint8_t * buf_old;
>> @@ -2433,10 +2448,31 @@ static int img_rebase(int argc, char **argv)
>>          buf_old = qemu_blockalign(bs, IO_BUF_SIZE);
>>          buf_new = qemu_blockalign(bs, IO_BUF_SIZE);
>>  
>> -        bdrv_get_geometry(bs, &num_sectors);
>> -        bdrv_get_geometry(bs_old_backing, &old_backing_num_sectors);
>> +        num_sectors = bdrv_nb_sectors(bs);
>> +        if (num_sectors < 0) {
>> +            error_report("Could not get size of '%s': %s",
>> +                         filename, strerror(-num_sectors));
>> +            ret = -1;
>> +            goto out;
>> +        }
>> +        old_backing_num_sectors = bdrv_nb_sectors(bs_old_backing);
>> +        if (old_backing_num_sectors < 0) {
>> +            char backing_name[1024];
>
> Could you put this on the heap ?
>
> I recently fixed a stack overflow when taking snapshots due to multiple 
> PATH_MAX
> char array in a recursive function.
>
> We don't know how this function will be used later.

img_rebase() is not a general purpose function, it's a qemu-img command.
Stack use is well below a single page even with my patch.  I can't see
how it could possibly become recursive.