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

["Benoît Canet" <benoit.canet@irqsave.net>]

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 <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c      | 11 ++++++++---
>  block/qapi.c | 14 +++++++++----
>  qemu-img.c   | 64 +++++++++++++++++++++++++++++++++++++++++++++++-------------
>  3 files changed, 68 insertions(+), 21 deletions(-)
> 
> diff --git a/block.c b/block.c
> index cae9085..b92f05f 100644
> --- a/block.c
> +++ b/block.c
> @@ -5574,7 +5574,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;
>  
> @@ -5593,8 +5593,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 97e1641..90f406d 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 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.

Best regards

Benoît

> +
> +            bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
> +            error_report("Could not get size of '%s': %s",
> +                         backing_name, strerror(-old_backing_num_sectors));
> +            ret = -1;
> +            goto out;
> +        }
>          if (bs_new_backing) {
> -            bdrv_get_geometry(bs_new_backing, &new_backing_num_sectors);
> +            new_backing_num_sectors = bdrv_nb_sectors(bs_new_backing);
> +            if (new_backing_num_sectors < 0) {
> +                error_report("Could not get size of '%s': %s",
> +                             out_baseimg, strerror(-new_backing_num_sectors));
> +                ret = -1;
> +                goto out;
> +            }
>          }
>  
>          if (num_sectors != 0) {
> -- 
> 1.9.3
>