Re: [Qemu-devel] [PATCH v3 01/10] raw-posix: Fix raw_getlength() to always return -errno on error

Re: [Qemu-devel] [PATCH v3 01/10] raw-posix: Fix raw_getlength() to always return -errno on error

[Benoît Canet]

The Friday 30 May 2014 à 20:13:42 (+0200), Markus Armbruster wrote :
> We got a merry mix of -1 and -errno here.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/raw-posix.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 6586a0c..9221de5 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -1097,12 +1097,12 @@ static int64_t raw_getlength(BlockDriverState *bs)
>      struct stat st;
>  
>      if (fstat(fd, &st))
> -        return -1;
> +        return -errno;
>      if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
>          struct disklabel dl;
>  
>          if (ioctl(fd, DIOCGDINFO, &dl))
> -            return -1;
> +            return -errno;
>          return (uint64_t)dl.d_secsize *
>              dl.d_partitions[DISKPART(st.st_rdev)].p_size;
>      } else
> @@ -1116,7 +1116,7 @@ static int64_t raw_getlength(BlockDriverState *bs)
>      struct stat st;
>  
>      if (fstat(fd, &st))
> -        return -1;
> +        return -errno;
>      if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
>          struct dkwedge_info dkw;
>  
> @@ -1126,7 +1126,7 @@ static int64_t raw_getlength(BlockDriverState *bs)
>              struct disklabel dl;
>  
>              if (ioctl(fd, DIOCGDINFO, &dl))
> -                return -1;
> +                return -errno;
>              return (uint64_t)dl.d_secsize *
>                  dl.d_partitions[DISKPART(st.st_rdev)].p_size;
>          }
> @@ -1139,6 +1139,7 @@ static int64_t raw_getlength(BlockDriverState *bs)
>      BDRVRawState *s = bs->opaque;
>      struct dk_minfo minfo;
>      int ret;
> +    int64_t size;
>  
>      ret = fd_open(bs);
>      if (ret < 0) {
> @@ -1157,7 +1158,11 @@ static int64_t raw_getlength(BlockDriverState *bs)
>       * There are reports that lseek on some devices fails, but
>       * irc discussion said that contingency on contingency was overkill.
>       */
> -    return lseek(s->fd, 0, SEEK_END);
> +    size = lseek(s->fd, 0, SEEK_END);
> +    if (size < 0) {
> +        return -errno;
> +    }
> +    return size;
>  }
>  #elif defined(CONFIG_BSD)
>  static int64_t raw_getlength(BlockDriverState *bs)
> @@ -1195,6 +1200,9 @@ again:
>          size = LONG_LONG_MAX;
>  #else
>          size = lseek(fd, 0LL, SEEK_END);
> +        if (size < 0) {
> +            return -errno;
> +        }
>  #endif
>  #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
>          switch(s->type) {
> @@ -1211,6 +1219,9 @@ again:
>  #endif
>      } else {
>          size = lseek(fd, 0, SEEK_END);
> +        if (size < 0) {
> +            return -errno;
> +        }
>      }
>      return size;
>  }
> @@ -1219,13 +1230,18 @@ static int64_t raw_getlength(BlockDriverState *bs)
>  {
>      BDRVRawState *s = bs->opaque;
>      int ret;
> +    int64_t size;
>  
>      ret = fd_open(bs);
>      if (ret < 0) {
>          return ret;
>      }
>  
> -    return lseek(s->fd, 0, SEEK_END);
> +    size = lseek(s->fd, 0, SEEK_END);
> +    if (size < 0) {
> +        return -errno;
> +    }
> +    return size;
>  }
>  #endif
>  
> -- 
> 1.9.3
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH v3 02/10] block: New bdrv_nb_sectors()

[Benoît Canet]

The Friday 30 May 2014 à 20:13:43 (+0200), Markus Armbruster wrote :
> A call to retrieve the image size converts between bytes and sectors
> several times:
> 
> * BlockDriver method bdrv_getlength() returns bytes.
> 
> * refresh_total_sectors() converts to sectors, rounding up, and stores
>   in total_sectors.
> 
> * bdrv_getlength() converts total_sectors back to bytes (now rounded
>   up to a multiple of the sector size).
> 
> * Callers wanting sectors rather bytes convert it right back.
>   Example: bdrv_get_geometry().
> 
> bdrv_nb_sectors() provides a way to omit the last two conversions.
> It's exactly bdrv_getlength() with the conversion to bytes omitted.
> It's functionally like bdrv_get_geometry() without its odd error
> handling.
> 
> Reimplement bdrv_getlength() and bdrv_get_geometry() on top of
> bdrv_nb_sectors().
> 
> The next patches will convert some users of bdrv_getlength() to
> bdrv_nb_sectors().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block.c               | 29 +++++++++++++++++++----------
>  include/block/block.h |  1 +
>  2 files changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/block.c b/block.c
> index a517d72..14dc5fe 100644
> --- a/block.c
> +++ b/block.c
> @@ -693,6 +693,7 @@ static int find_image_format(BlockDriverState *bs, const char *filename,
>  
>  /**
>   * Set the current 'total_sectors' value
> + * Return 0 on success, -errno on error.
>   */
>  static int refresh_total_sectors(BlockDriverState *bs, int64_t hint)
>  {
> @@ -3551,11 +3552,12 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState *bs)
>  }
>  
>  /**
> - * Length of a file in bytes. Return < 0 if error or unknown.
> + * Return number of sectors on success, -errno on error.
>   */
> -int64_t bdrv_getlength(BlockDriverState *bs)
> +int64_t bdrv_nb_sectors(BlockDriverState *bs)
>  {
>      BlockDriver *drv = bs->drv;
> +
>      if (!drv)
>          return -ENOMEDIUM;
>  
> @@ -3565,19 +3567,26 @@ int64_t bdrv_getlength(BlockDriverState *bs)
>              return ret;
>          }
>      }
> -    return bs->total_sectors * BDRV_SECTOR_SIZE;
> +    return bs->total_sectors;
> +}
> +
> +/**
> + * Return length in bytes on success, -errno on error.
> + * The length is always a multiple of BDRV_SECTOR_SIZE.
> + */
> +int64_t bdrv_getlength(BlockDriverState *bs)
> +{
> +    int64_t ret = bdrv_nb_sectors(bs);
> +
> +    return ret < 0 ? ret : ret * BDRV_SECTOR_SIZE;
>  }
>  
>  /* return 0 as number of sectors if no device present or error */
>  void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr)
>  {
> -    int64_t length;
> -    length = bdrv_getlength(bs);
> -    if (length < 0)
> -        length = 0;
> -    else
> -        length = length >> BDRV_SECTOR_BITS;
> -    *nb_sectors_ptr = length;
> +    int64_t nb_sectors = bdrv_nb_sectors(bs);
> +
> +    *nb_sectors_ptr = nb_sectors < 0 ? 0 : nb_sectors;
>  }
>  
>  void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError on_read_error,
> diff --git a/include/block/block.h b/include/block/block.h
> index faee3aa..4a1e875 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -279,6 +279,7 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
>      const char *backing_file);
>  int bdrv_get_backing_file_depth(BlockDriverState *bs);
>  int bdrv_truncate(BlockDriverState *bs, int64_t offset);
> +int64_t bdrv_nb_sectors(BlockDriverState *bs);
>  int64_t bdrv_getlength(BlockDriverState *bs);
>  int64_t bdrv_get_allocated_file_size(BlockDriverState *bs);
>  void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
> -- 
> 1.9.3
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH v3 03/10] block: Use bdrv_nb_sectors() in bdrv_make_zero()

[Benoît Canet]

The Friday 30 May 2014 à 20:13:44 (+0200), Markus Armbruster wrote :
> Instead of bdrv_getlength().
> 
> Variable target_size is initially in bytes, then changes meaning to
> sectors.  Ugh.  Replace by target_sectors.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 14dc5fe..9517108 100644
> --- a/block.c
> +++ b/block.c
> @@ -2855,18 +2855,16 @@ int bdrv_write_zeroes(BlockDriverState *bs, int64_t sector_num,
>   */
>  int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
>  {
> -    int64_t target_size;
> -    int64_t ret, nb_sectors, sector_num = 0;
> +    int64_t target_sectors, ret, nb_sectors, sector_num = 0;
>      int n;
>  
> -    target_size = bdrv_getlength(bs);
> -    if (target_size < 0) {
> -        return target_size;
> +    target_sectors = bdrv_nb_sectors(bs);
> +    if (target_sectors < 0) {
> +        return target_sectors;
>      }
> -    target_size /= BDRV_SECTOR_SIZE;
>  
>      for (;;) {
> -        nb_sectors = target_size - sector_num;
> +        nb_sectors = target_sectors - sector_num;
>          if (nb_sectors <= 0) {
>              return 0;
>          }
> -- 
> 1.9.3
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH v3 04/10] block: Use bdrv_nb_sectors() in bdrv_aligned_preadv()

[Benoît Canet]

The Friday 30 May 2014 à 20:13:45 (+0200), Markus Armbruster wrote :
> Instead of bdrv_getlength().  Eliminate variable len.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 9517108..6639d09 100644
> --- a/block.c
> +++ b/block.c
> @@ -3081,15 +3081,14 @@ static int coroutine_fn bdrv_aligned_preadv(BlockDriverState *bs,
>          ret = drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov);
>      } else {
>          /* Read zeros after EOF of growable BDSes */
> -        int64_t len, total_sectors, max_nb_sectors;
> +        int64_t total_sectors, max_nb_sectors;
>  
> -        len = bdrv_getlength(bs);
> -        if (len < 0) {
> -            ret = len;
> +        total_sectors = bdrv_nb_sectors(bs);
> +        if (total_sectors < 0) {
> +            ret = total_sectors;
>              goto out;
>          }
>  
> -        total_sectors = DIV_ROUND_UP(len, BDRV_SECTOR_SIZE);
>          max_nb_sectors = ROUND_UP(MAX(0, total_sectors - sector_num),
>                                    align >> BDRV_SECTOR_BITS);
>          if (max_nb_sectors > 0) {
> -- 
> 1.9.3
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH v3 05/10] block: Use bdrv_nb_sectors() in bdrv_co_get_block_status()

[Benoît Canet]

The Friday 30 May 2014 à 20:13:46 (+0200), Markus Armbruster wrote :
> Instead of bdrv_getlength().
> 
> Replace variables length, length2 by total_sectors, nb_sectors2.
> Bonus: use total_sectors instead of the slightly unclean
> bs->total_sectors.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 6639d09..cfeb497 100644
> --- a/block.c
> +++ b/block.c
> @@ -3926,21 +3926,21 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>                                                       int64_t sector_num,
>                                                       int nb_sectors, int *pnum)
>  {
> -    int64_t length;
> +    int64_t total_sectors;
>      int64_t n;
>      int64_t ret, ret2;
>  
> -    length = bdrv_getlength(bs);
> -    if (length < 0) {
> -        return length;
> +    total_sectors = bdrv_nb_sectors(bs);
> +    if (total_sectors < 0) {
> +        return total_sectors;
>      }
>  
> -    if (sector_num >= (length >> BDRV_SECTOR_BITS)) {
> +    if (sector_num >= total_sectors) {
>          *pnum = 0;
>          return 0;
>      }
>  
> -    n = bs->total_sectors - sector_num;
> +    n = total_sectors - sector_num;
>      if (n < nb_sectors) {
>          nb_sectors = n;
>      }
> @@ -3975,8 +3975,8 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>              ret |= BDRV_BLOCK_ZERO;
>          } else if (bs->backing_hd) {
>              BlockDriverState *bs2 = bs->backing_hd;
> -            int64_t length2 = bdrv_getlength(bs2);
> -            if (length2 >= 0 && sector_num >= (length2 >> BDRV_SECTOR_BITS)) {
> +            int64_t nb_sectors2 = bdrv_nb_sectors(bs2);
> +            if (nb_sectors2 >= 0 && sector_num >= nb_sectors2) {
>                  ret |= BDRV_BLOCK_ZERO;
>              }
>          }
> -- 
> 1.9.3
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH v3 06/10] block: Use bdrv_nb_sectors() in img_convert()

[Benoît Canet]

The Friday 30 May 2014 à 20:13:47 (+0200), Markus Armbruster wrote :
> Instead of bdrv_getlength().  Replace variable output_length by
> output_sectors.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  qemu-img.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 1ad899e..8d996ba 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1478,13 +1478,13 @@ static int img_convert(int argc, char **argv)
>      buf = qemu_blockalign(out_bs, bufsectors * BDRV_SECTOR_SIZE);
>  
>      if (skip_create) {
> -        int64_t output_length = bdrv_getlength(out_bs);
> -        if (output_length < 0) {
> +        int64_t output_sectors = bdrv_nb_sectors(out_bs);
> +        if (output_sectors < 0) {
>              error_report("unable to get output image length: %s\n",
> -                         strerror(-output_length));
> +                         strerror(-output_sectors));
>              ret = -1;
>              goto out;
> -        } else if (output_length < total_sectors << BDRV_SECTOR_BITS) {
> +        } else if (output_sectors < total_sectors) {
>              error_report("output file is smaller than input file");
>              ret = -1;
>              goto out;
> -- 
> 1.9.3
> 
> 

Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH v3 07/10] block: Use bdrv_nb_sectors() where sectors, not bytes are wanted

[Benoît Canet]

The Friday 30 May 2014 à 20:13:48 (+0200), Markus Armbruster wrote :
> Instead of bdrv_getlength().
> 
> Aside: a few of these callers don't handle errors.  I didn't
> investigate whether they should.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block-migration.c | 9 ++++-----
>  block.c           | 3 +--
>  block/qcow2.c     | 2 +-
>  block/vmdk.c      | 5 ++---
>  4 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/block-migration.c b/block-migration.c
> index 1656270..1c7b7be 100644
> --- a/block-migration.c
> +++ b/block-migration.c
> @@ -186,7 +186,7 @@ static int bmds_aio_inflight(BlkMigDevState *bmds, int64_t sector)
>  {
>      int64_t chunk = sector / (int64_t)BDRV_SECTORS_PER_DIRTY_CHUNK;
>  
> -    if ((sector << BDRV_SECTOR_BITS) < bdrv_getlength(bmds->bs)) {
> +    if (sector < bdrv_nb_sectors(bmds->bs)) {
>          return !!(bmds->aio_bitmap[chunk / (sizeof(unsigned long) * 8)] &
>              (1UL << (chunk % (sizeof(unsigned long) * 8))));
>      } else {
> @@ -223,8 +223,7 @@ static void alloc_aio_bitmap(BlkMigDevState *bmds)
>      BlockDriverState *bs = bmds->bs;
>      int64_t bitmap_size;
>  
> -    bitmap_size = (bdrv_getlength(bs) >> BDRV_SECTOR_BITS) +
> -            BDRV_SECTORS_PER_DIRTY_CHUNK * 8 - 1;
> +    bitmap_size = bdrv_nb_sectors(bs) + BDRV_SECTORS_PER_DIRTY_CHUNK * 8 - 1;
>      bitmap_size /= BDRV_SECTORS_PER_DIRTY_CHUNK * 8;
>  
>      bmds->aio_bitmap = g_malloc0(bitmap_size);
> @@ -350,7 +349,7 @@ static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
>      int64_t sectors;
>  
>      if (!bdrv_is_read_only(bs)) {
> -        sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
> +        sectors = bdrv_nb_sectors(bs);
>          if (sectors <= 0) {
>              return;
>          }
> @@ -800,7 +799,7 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
>  
>              if (bs != bs_prev) {
>                  bs_prev = bs;
> -                total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
> +                total_sectors = bdrv_nb_sectors(bs);
>                  if (total_sectors <= 0) {
>                      error_report("Error getting length of block device %s",
>                                   device_name);
> diff --git a/block.c b/block.c
> index cfeb497..8ebfb79 100644
> --- a/block.c
> +++ b/block.c
> @@ -5258,13 +5258,12 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
>  
>      granularity >>= BDRV_SECTOR_BITS;
>      assert(granularity);
> -    bitmap_size = bdrv_getlength(bs);
> +    bitmap_size = bdrv_nb_sectors(bs);

The name bitmap_size seems to imply the unit is byte.
>      if (bitmap_size < 0) {
>          error_setg_errno(errp, -bitmap_size, "could not get length of device");
>          errno = -bitmap_size;
>          return NULL;
>      }
> -    bitmap_size >>= BDRV_SECTOR_BITS;
>      bitmap = g_malloc0(sizeof(BdrvDirtyBitmap));
>      bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
>      QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
> diff --git a/block/qcow2.c b/block/qcow2.c
> index a4b97e8..98f624c 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1539,7 +1539,7 @@ static int preallocate(BlockDriverState *bs)
>      int ret;
>      QCowL2Meta *meta;
>  
> -    nb_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
> +    nb_sectors = bdrv_nb_sectors(bs);
>      offset = 0;
>  
>      while (nb_sectors) {
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 480ea37..5c5e73b 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -669,8 +669,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
>      if (le32_to_cpu(header.flags) & VMDK4_FLAG_RGD) {
>          l1_backup_offset = le64_to_cpu(header.rgd_offset) << 9;
>      }
> -    if (bdrv_getlength(file) <
> -            le64_to_cpu(header.grain_offset) * BDRV_SECTOR_SIZE) {
> +    if (bdrv_nb_sectors(file) < le64_to_cpu(header.grain_offset)) {
>          error_setg(errp, "File truncated, expecting at least %" PRId64 " bytes",
>                     (int64_t)(le64_to_cpu(header.grain_offset)
>                               * BDRV_SECTOR_SIZE));
> @@ -2004,7 +2003,7 @@ static int vmdk_check(BlockDriverState *bs, BdrvCheckResult *result,
>      BDRVVmdkState *s = bs->opaque;
>      VmdkExtent *extent = NULL;
>      int64_t sector_num = 0;
> -    int64_t total_sectors = bdrv_getlength(bs) / BDRV_SECTOR_SIZE;
> +    int64_t total_sectors = bdrv_nb_sectors(bs);
>      int ret;
>      uint64_t cluster_offset;
>  
> -- 
> 1.9.3
> 
> 

Re: [Qemu-devel] [PATCH v3 08/10] block: Drop superfluous aligning of bdrv_getlength()'s value

[Benoît Canet]

The Friday 30 May 2014 à 20:13:49 (+0200), Markus Armbruster wrote :
> It returns a multiple of the sector size.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c       | 1 -
>  block/qcow2.c | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 8ebfb79..41837b4 100644
> --- a/block.c
> +++ b/block.c
> @@ -1260,7 +1260,6 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
>          error_setg_errno(errp, -total_size, "Could not get image size");
>          goto out;
>      }
> -    total_size &= BDRV_SECTOR_MASK;
>  
>      /* Create the temporary image */
>      ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 98f624c..4af09bd 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1927,7 +1927,6 @@ static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
>          /* align end of file to a sector boundary to ease reading with
>             sector based I/Os */
>          cluster_offset = bdrv_getlength(bs->file);
> -        cluster_offset = (cluster_offset + 511) & ~511;
>          bdrv_truncate(bs->file, cluster_offset);
>          return 0;
>      }
> -- 
> 1.9.3
> 
> 

Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH v3 09/10] qemu-img: Make img_convert() get image size just once per image

[Benoît Canet]

The Friday 30 May 2014 à 20:13:50 (+0200), Markus Armbruster wrote :
> Chiefly so I don't have to do the error checking in quadruplicate in
> the next commit.  Moreover, replacing the frequently updated
> bs_sectors by an array assigned just once makes the code easier to
> understand.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-img.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 8d996ba..2cb56c5 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1186,7 +1186,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;
> +    uint64_t *bs_sectors = NULL;
>      uint8_t * buf = NULL;
>      size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE;
>      const uint8_t *buf1;
> @@ -1326,7 +1326,8 @@ static int img_convert(int argc, char **argv)
>  
>      qemu_progress_print(0, 100);
>  
> -    bs = g_malloc0(bs_n * sizeof(BlockDriverState *));
> +    bs = g_new0(BlockDriverState *, bs_n);
> +    bs_sectors = g_new(uint64_t, bs_n);
>  
>      total_sectors = 0;
>      for (bs_i = 0; bs_i < bs_n; bs_i++) {
> @@ -1340,8 +1341,8 @@ static int img_convert(int argc, char **argv)
>              ret = -1;
>              goto out;
>          }
> -        bdrv_get_geometry(bs[bs_i], &bs_sectors);
> -        total_sectors += bs_sectors;
> +        bdrv_get_geometry(bs[bs_i], &bs_sectors[bs_i]);
> +        total_sectors += bs_sectors[bs_i];
>      }
>  
>      if (sn_opts) {
> @@ -1465,7 +1466,6 @@ static int img_convert(int argc, char **argv)
>  
>      bs_i = 0;
>      bs_offset = 0;
> -    bdrv_get_geometry(bs[0], &bs_sectors);
>  
>      /* increase bufsectors from the default 4096 (2M) if opt_transfer_length
>       * or discard_alignment of the out_bs is greater. Limit to 32768 (16MB)
> @@ -1532,19 +1532,19 @@ static int img_convert(int argc, char **argv)
>              buf2 = buf;
>              while (remainder > 0) {
>                  int nlow;
> -                while (bs_num == bs_sectors) {
> +                while (bs_num == bs_sectors[bs_i]) {
> +                    bs_offset += bs_sectors[bs_i];
>                      bs_i++;
>                      assert (bs_i < bs_n);
> -                    bs_offset += bs_sectors;
> -                    bdrv_get_geometry(bs[bs_i], &bs_sectors);
>                      bs_num = 0;
>                      /* printf("changing part: sector_num=%" PRId64 ", "
>                         "bs_i=%d, bs_offset=%" PRId64 ", bs_sectors=%" PRId64
> -                       "\n", sector_num, bs_i, bs_offset, bs_sectors); */
> +                       "\n", sector_num, bs_i, bs_offset, bs_sectors[bs_i]); */
>                  }
> -                assert (bs_num < bs_sectors);
> +                assert (bs_num < bs_sectors[bs_i]);
>  
> -                nlow = (remainder > bs_sectors - bs_num) ? bs_sectors - bs_num : remainder;
> +                nlow = remainder > bs_sectors[bs_i] - bs_num
> +                    ? bs_sectors[bs_i] - bs_num : remainder;
>  
>                  ret = bdrv_read(bs[bs_i], bs_num, buf2, nlow);
>                  if (ret < 0) {
> @@ -1605,14 +1605,13 @@ restart:
>                  break;
>              }
>  
> -            while (sector_num - bs_offset >= bs_sectors) {
> +            while (sector_num - bs_offset >= bs_sectors[bs_i]) {
> +                bs_offset += bs_sectors[bs_i];
>                  bs_i ++;
>                  assert (bs_i < bs_n);
> -                bs_offset += bs_sectors;
> -                bdrv_get_geometry(bs[bs_i], &bs_sectors);
>                  /* printf("changing part: sector_num=%" PRId64 ", bs_i=%d, "
>                    "bs_offset=%" PRId64 ", bs_sectors=%" PRId64 "\n",
> -                   sector_num, bs_i, bs_offset, bs_sectors); */
> +                   sector_num, bs_i, bs_offset, bs_sectors[bs_i]); */
>              }
>  
>              if ((out_baseimg || has_zero_init) &&
> @@ -1665,7 +1664,7 @@ restart:
>                  }
>              }
>  
> -            n = MIN(n, bs_sectors - (sector_num - bs_offset));
> +            n = MIN(n, bs_sectors[bs_i] - (sector_num - bs_offset));
>  
>              sectors_read += n;
>              if (count_allocated_sectors) {
> @@ -1723,6 +1722,7 @@ out:
>          }
>          g_free(bs);
>      }
> +    g_free(bs_sectors);
>  fail_getopt:
>      g_free(options);
>  
> -- 
> 1.9.3
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

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

[Benoît Canet]

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 <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   | 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 ? 

>      total_sectors = MIN(total_sectors1, total_sectors2);
>      progress_base = MAX(total_sectors1, total_sectors2);
>  
OA> @@ -1186,7 +1193,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;
> @@ -1327,7 +1334,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++) {
> @@ -1341,7 +1348,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];
>      }
>  
> @@ -2422,9 +2435,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;
> @@ -2434,10 +2447,28 @@ 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));
> +            goto out;
> +        }
> +        old_backing_num_sectors = bdrv_nb_sectors(bs_old_backing);
> +        if (old_backing_num_sectors < 0) {
> +            char backing_name[1024];
> +
> +            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));
> +            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));
> +                goto out;
> +            }
>          }
>  
>          if (num_sectors != 0) {
> -- 
> 1.9.3
> 
> 

Re: [Qemu-devel] [PATCH v3 07/10] block: Use bdrv_nb_sectors() where sectors, not bytes are wanted

[Markus Armbruster]

Benoît Canet <benoit.canet@irqsave.net> writes:

> The Friday 30 May 2014 à 20:13:48 (+0200), Markus Armbruster wrote :
>> Instead of bdrv_getlength().
>> 
>> Aside: a few of these callers don't handle errors.  I didn't
>> investigate whether they should.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
[...]
>> diff --git a/block.c b/block.c
>> index cfeb497..8ebfb79 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -5258,13 +5258,12 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
>>  
>>      granularity >>= BDRV_SECTOR_BITS;
>>      assert(granularity);
>> -    bitmap_size = bdrv_getlength(bs);
>> +    bitmap_size = bdrv_nb_sectors(bs);
>
> The name bitmap_size seems to imply the unit is byte.

I didn't examine how the bitmap is used.  My patch doesn't change the
value put into bitmap_size.

>>      if (bitmap_size < 0) {
>>          error_setg_errno(errp, -bitmap_size, "could not get length of device");
>>          errno = -bitmap_size;
>>          return NULL;
>>      }
>> -    bitmap_size >>= BDRV_SECTOR_BITS;
>>      bitmap = g_malloc0(sizeof(BdrvDirtyBitmap));
>>      bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
>>      QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
[...]

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

[Markus Armbruster]

Benoît Canet <benoit.canet@irqsave.net> 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 <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   | 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!

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

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Benoît Canet <benoit.canet@irqsave.net> 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.
[...]
>>> 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".

All right, I'm picking the value we use for read failures: 4.

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