Re: [Qemu-devel] [PATCH 02/31] block: Make bdrv_round_to_clusters() signature more useful

[John Snow]

On 04/26/2017 05:47 PM, Eric Blake wrote:
> On 04/26/2017 04:41 PM, John Snow wrote:
>>
>>
>> On 04/17/2017 09:33 PM, Eric Blake wrote:
>>> In the process of converting sector-based interfaces to bytes,
>>> I'm finding it easier to represent a byte count as a 64-bit
>>> integer at the block layer (even if we are internally capped
>>> by SIZE_MAX or even INT_MAX for individual transactions, it's
>>> still nicer to not have to worry about truncation/overflow
>>> issues on as many variables).  Update the signature of
>>> bdrv_round_to_clusters() to uniformly use uint64_t, matching
>> 
>>
>> While we're here, since you went with int64_t in the end, what steered
>> you away from uint64_t, or was that just a thinko?
> 
> Later patches were made easier with signed (the compiler complained when
> I mixed signed and unsigned pointers).
> 
>>
>> (AFAICT: off_t is usually something like int64_t, so your choice makes
>> sense to me, generally.)
> 
> Indeed, and that's something I should update my commit message to mention.
> 
>>
>> --js
>>
>>> the signature already chosen for bdrv_is_allocated, and
>>> adjust clients according to the required fallout.
> 
> If you want me to try and use uint64_t *pnum instead of int64_t *pnum
> throughout both my series 1  (the changes to bdrv_is_allocated) and this
> one, it will take more effort.  I'll do it if there's a reason, but I'd
> rather not if the signed version is good enough.
> 

No, I didn't mean to imply you should, I was just pointing out the
commit message typo. int64_t is likely the correct choice for a number
of reasons, at least being able to return -1 from functions returning a
byte offset being the chief reason.

--js



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 02/31] block: Make bdrv_round_to_clusters() signature more useful

[Eric Blake]

On 04/26/2017 04:41 PM, John Snow wrote:
> 
> 
> On 04/17/2017 09:33 PM, Eric Blake wrote:
>> In the process of converting sector-based interfaces to bytes,
>> I'm finding it easier to represent a byte count as a 64-bit
>> integer at the block layer (even if we are internally capped
>> by SIZE_MAX or even INT_MAX for individual transactions, it's
>> still nicer to not have to worry about truncation/overflow
>> issues on as many variables).  Update the signature of
>> bdrv_round_to_clusters() to uniformly use uint64_t, matching
> 
> 
> While we're here, since you went with int64_t in the end, what steered
> you away from uint64_t, or was that just a thinko?

Later patches were made easier with signed (the compiler complained when
I mixed signed and unsigned pointers).

> 
> (AFAICT: off_t is usually something like int64_t, so your choice makes
> sense to me, generally.)

Indeed, and that's something I should update my commit message to mention.

> 
> --js
> 
>> the signature already chosen for bdrv_is_allocated, and
>> adjust clients according to the required fallout.

If you want me to try and use uint64_t *pnum instead of int64_t *pnum
throughout both my series 1  (the changes to bdrv_is_allocated) and this
one, it will take more effort.  I'll do it if there's a reason, but I'd
rather not if the signed version is good enough.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 02/31] block: Make bdrv_round_to_clusters() signature more useful

[John Snow]

On 04/17/2017 09:33 PM, Eric Blake wrote:
> In the process of converting sector-based interfaces to bytes,
> I'm finding it easier to represent a byte count as a 64-bit
> integer at the block layer (even if we are internally capped
> by SIZE_MAX or even INT_MAX for individual transactions, it's
> still nicer to not have to worry about truncation/overflow
> issues on as many variables).  Update the signature of
> bdrv_round_to_clusters() to uniformly use uint64_t, matching


While we're here, since you went with int64_t in the end, what steered
you away from uint64_t, or was that just a thinko?

(AFAICT: off_t is usually something like int64_t, so your choice makes
sense to me, generally.)

--js

> the signature already chosen for bdrv_is_allocated, and
> adjust clients according to the required fallout.
> 
> Signed-off-by: Eric Blake 
> ---
>  include/block/block.h | 4 ++--
>  block/io.c| 7 ---
>  block/mirror.c| 9 -
>  block/trace-events| 2 +-
>  4 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 86ad511..eed1330 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -468,9 +468,9 @@ int bdrv_get_flags(BlockDriverState *bs);
>  int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
>  ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs);
>  void bdrv_round_to_clusters(BlockDriverState *bs,
> -int64_t offset, unsigned int bytes,
> +int64_t offset, int64_t bytes,
>  int64_t *cluster_offset,
> -unsigned int *cluster_bytes);
> +int64_t *cluster_bytes);
> 
>  const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
>  void bdrv_get_backing_filename(BlockDriverState *bs,
> diff --git a/block/io.c b/block/io.c
> index d61a906..07165dc 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -422,9 +422,9 @@ static void mark_request_serialising(BdrvTrackedRequest 
> *req, uint64_t align)
>   * Round a region to cluster boundaries
>   */
>  void bdrv_round_to_clusters(BlockDriverState *bs,
> -int64_t offset, unsigned int bytes,
> +int64_t offset, int64_t bytes,
>  int64_t *cluster_offset,
> -unsigned int *cluster_bytes)
> +int64_t *cluster_bytes)
>  {
>  BlockDriverInfo bdi;
> 
> @@ -920,7 +920,7 @@ static int coroutine_fn 
> bdrv_co_do_copy_on_readv(BdrvChild *child,
>  struct iovec iov;
>  QEMUIOVector bounce_qiov;
>  int64_t cluster_offset;
> -unsigned int cluster_bytes;
> +int64_t cluster_bytes;
>  size_t skip_bytes;
>  int ret;
> 
> @@ -941,6 +941,7 @@ static int coroutine_fn 
> bdrv_co_do_copy_on_readv(BdrvChild *child,
>  trace_bdrv_co_do_copy_on_readv(bs, offset, bytes,
> cluster_offset, cluster_bytes);
> 
> +assert(cluster_bytes < SIZE_MAX);
>  iov.iov_len = cluster_bytes;
>  iov.iov_base = bounce_buffer = qemu_try_blockalign(bs, iov.iov_len);
>  if (bounce_buffer == NULL) {
> diff --git a/block/mirror.c b/block/mirror.c
> index 846e392..2510793 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -177,7 +177,7 @@ static void mirror_read_complete(void *opaque, int ret)
>  /* Clip bytes relative to offset to not exceed end-of-file */
>  static inline void mirror_clip_bytes(MirrorBlockJob *s,
>   int64_t offset,
> - unsigned int *bytes)
> + int64_t *bytes)
>  {
>  *bytes = MIN(*bytes, s->bdev_length - offset);
>  }
> @@ -190,10 +190,9 @@ static int mirror_cow_align(MirrorBlockJob *s, int64_t 
> *offset,
>  bool need_cow;
>  int ret = 0;
>  int64_t align_offset = *offset;
> -unsigned int align_bytes = *bytes;
> +int64_t align_bytes = *bytes;
>  int max_bytes = s->granularity * s->max_iov;
> 
> -assert(*bytes < INT_MAX);
>  need_cow = !test_bit(*offset / s->granularity, s->cow_bitmap);
>  need_cow |= !test_bit((*offset + *bytes - 1) / s->granularity,
>s->cow_bitmap);
> @@ -384,7 +383,7 @@ static uint64_t coroutine_fn 
> mirror_iteration(MirrorBlockJob *s)
>  while (nb_chunks > 0 && offset < s->bdev_length) {
>  int64_t ret;
>  int io_sectors;
> -unsigned int io_bytes;
> +int64_t io_bytes;
>  int64_t io_bytes_acct;
>  BlockDriverState *file;
>  enum MirrorMethod {
> @@ -410,7 +409,7 @@ static uint64_t coroutine_fn 
> mirror_iteration(MirrorBlockJob *s)
>  io_bytes = s->granularity;
>  } else if (ret >= 0 && !(ret & BDRV_BLOCK_DATA)) {
>  int64_t target_offset;
> -  

[Qemu-devel] [PATCH 02/31] block: Make bdrv_round_to_clusters() signature more useful

[Eric Blake]

In the process of converting sector-based interfaces to bytes,
I'm finding it easier to represent a byte count as a 64-bit
integer at the block layer (even if we are internally capped
by SIZE_MAX or even INT_MAX for individual transactions, it's
still nicer to not have to worry about truncation/overflow
issues on as many variables).  Update the signature of
bdrv_round_to_clusters() to uniformly use uint64_t, matching
the signature already chosen for bdrv_is_allocated, and
adjust clients according to the required fallout.

Signed-off-by: Eric Blake 
---
 include/block/block.h | 4 ++--
 block/io.c| 7 ---
 block/mirror.c| 9 -
 block/trace-events| 2 +-
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 86ad511..eed1330 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -468,9 +468,9 @@ int bdrv_get_flags(BlockDriverState *bs);
 int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
 ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs);
 void bdrv_round_to_clusters(BlockDriverState *bs,
-int64_t offset, unsigned int bytes,
+int64_t offset, int64_t bytes,
 int64_t *cluster_offset,
-unsigned int *cluster_bytes);
+int64_t *cluster_bytes);

 const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
 void bdrv_get_backing_filename(BlockDriverState *bs,
diff --git a/block/io.c b/block/io.c
index d61a906..07165dc 100644
--- a/block/io.c
+++ b/block/io.c
@@ -422,9 +422,9 @@ static void mark_request_serialising(BdrvTrackedRequest 
*req, uint64_t align)
  * Round a region to cluster boundaries
  */
 void bdrv_round_to_clusters(BlockDriverState *bs,
-int64_t offset, unsigned int bytes,
+int64_t offset, int64_t bytes,
 int64_t *cluster_offset,
-unsigned int *cluster_bytes)
+int64_t *cluster_bytes)
 {
 BlockDriverInfo bdi;

@@ -920,7 +920,7 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild 
*child,
 struct iovec iov;
 QEMUIOVector bounce_qiov;
 int64_t cluster_offset;
-unsigned int cluster_bytes;
+int64_t cluster_bytes;
 size_t skip_bytes;
 int ret;

@@ -941,6 +941,7 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild 
*child,
 trace_bdrv_co_do_copy_on_readv(bs, offset, bytes,
cluster_offset, cluster_bytes);

+assert(cluster_bytes < SIZE_MAX);
 iov.iov_len = cluster_bytes;
 iov.iov_base = bounce_buffer = qemu_try_blockalign(bs, iov.iov_len);
 if (bounce_buffer == NULL) {
diff --git a/block/mirror.c b/block/mirror.c
index 846e392..2510793 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -177,7 +177,7 @@ static void mirror_read_complete(void *opaque, int ret)
 /* Clip bytes relative to offset to not exceed end-of-file */
 static inline void mirror_clip_bytes(MirrorBlockJob *s,
  int64_t offset,
- unsigned int *bytes)
+ int64_t *bytes)
 {
 *bytes = MIN(*bytes, s->bdev_length - offset);
 }
@@ -190,10 +190,9 @@ static int mirror_cow_align(MirrorBlockJob *s, int64_t 
*offset,
 bool need_cow;
 int ret = 0;
 int64_t align_offset = *offset;
-unsigned int align_bytes = *bytes;
+int64_t align_bytes = *bytes;
 int max_bytes = s->granularity * s->max_iov;

-assert(*bytes < INT_MAX);
 need_cow = !test_bit(*offset / s->granularity, s->cow_bitmap);
 need_cow |= !test_bit((*offset + *bytes - 1) / s->granularity,
   s->cow_bitmap);
@@ -384,7 +383,7 @@ static uint64_t coroutine_fn 
mirror_iteration(MirrorBlockJob *s)
 while (nb_chunks > 0 && offset < s->bdev_length) {
 int64_t ret;
 int io_sectors;
-unsigned int io_bytes;
+int64_t io_bytes;
 int64_t io_bytes_acct;
 BlockDriverState *file;
 enum MirrorMethod {
@@ -410,7 +409,7 @@ static uint64_t coroutine_fn 
mirror_iteration(MirrorBlockJob *s)
 io_bytes = s->granularity;
 } else if (ret >= 0 && !(ret & BDRV_BLOCK_DATA)) {
 int64_t target_offset;
-unsigned int target_bytes;
+int64_t target_bytes;
 bdrv_round_to_clusters(blk_bs(s->target), offset, io_bytes,
_offset, _bytes);
 if (target_offset == offset &&
diff --git a/block/trace-events b/block/trace-events
index 04f6463..2301a50 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -15,7 +15,7 @@ bdrv_aio_writev(void *bs, int64_t sector_num, int nb_sectors, 
void *opaque) "bs
 bdrv_co_readv(void *bs, int64_t sector_num, int nb_sector) "bs %p