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

[Benoît Canet]

The Wednesday 04 Jun 2014 à 15:20:18 (+0200), Markus Armbruster wrote :
> 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.

ok