Re: [Qemu-block] [PATCH] block: document semanatics of bdrv_co_preadv|pwritev

[Fam Zheng]

On Fri, 08/04 16:49, Daniel P. Berrange wrote:
> This is odd.  In the bdrv_aligned_readv() it looks very much like
> we'll reference qiov->niov, if bytes != 0, so if qiov was NULL we
> would crash.

It doesn't make sense if read doesn't have an iov, where should the data be
placed? :)

> 
> In bdrv_aligned_writev(), qiov->niov is also refernced if bytes != 0,
> *unless*  flags contains BDRV_REQ_ZERO_WRITE, in which case we'll
> invoke bdrv_co_do_pwrite_zeroes() instead.

This is intended. Zero-write doesn't need qiov, hence the BDRV_REQ_ZERO_WRITE
branch. Otherwise, we can assert qiov != NULL.

> 
> So unless I'm missing something, bdrv_co_preadv|writev cannot be
> called with a NULL  qiov, and bdrv_aligned_writev|readv might
> need their assertions tightened up.

bdrv_co_pwritev _is_ called with a NULL qiov from blk_aio_pwrite_zeroes. Your
other reasonings are right.

So for write we cannot remove the bytes parameter.

Fam

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 29/56] block: Make BlockDirtyInfo byte count unsigned in QAPI/QMP

[John Snow]

On 08/07/2017 10:45 AM, Markus Armbruster wrote:
> Byte counts should use QAPI type 'size' (uint64_t).  BlockDirtyInfo
> member @count is 'int' (int64_t).  bdrv_query_dirty_bitmaps() computes
> @count from bdrv_get_dirty_count() in uint64_t, then implicitly
> converts to int64_t.  Before the commit before previous, the
> conversion was in bdrv_get_dirty_count() instead.
> 
> Change member @count to 'size'.
> 
> query-block now reports @count values above 2^63-1 correctly instead
> of their (negative) two's complement.
> 
> Signed-off-by: Markus Armbruster 

Assuming there's no "gotcha" here introduced by changing the QAPI, then
ACK; but you're the expert there, so I trust you!

Reviewed-by: John Snow 

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 28/56] block: Widen dirty bitmap granularity to uint64_t for safety

[John Snow]

On 08/07/2017 10:45 AM, Markus Armbruster wrote:
> Block dirty bitmaps represent granularity in bytes as uint32_t.  It
> must be a power of two and a multiple of BDRV_SECTOR_SIZE.
> 
> The trouble with uint32_t is computations like this one in
> mirror_do_read():
> 
> uint64_t max_bytes;
> 
> max_bytes = s->granularity * s->max_iov;
> 
> The operands of * are uint32_t and int, so the product is computed in
> uint32_t (assuming 32 bit int), then zero-extended to uint64_t.
> 
> Since granularity is generally combined with 64 bit file offsets, it's
> best to make it 64 bits, too.  Less opportunity to screw up.
> 
> Signed-off-by: Markus Armbruster 

[bweeooop]

> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c

[bwp]

> @@ -506,16 +506,11 @@ uint32_t 
> bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
>  return granularity;
>  }
>  
> -uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap)
> +uint64_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap)
>  {
>  return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
>  }
>  
> -uint32_t bdrv_dirty_bitmap_meta_granularity(BdrvDirtyBitmap *bitmap)
> -{
> -return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->meta);
> -}

Why? Unused? Not cool enough to mention?

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 27/56] block/dirty-bitmap: Clean up signed vs. unsigned dirty counts

[John Snow]

On 08/07/2017 10:45 AM, Markus Armbruster wrote:
> hbitmap_count() returns uint64_t.
> 
> Clean up test-hbitmap.c to check its value with g_assert_cmpuint()
> instead of g_assert_cmpint().
> 
> bdrv_get_dirty_count() and bdrv_get_meta_dirty_count() return its
> value converted to int64_t.  Clean them up to return it unadulterated.
> 
> This moves the implicit conversion to some callers, so clean them up,
> too.
> 
> mirror_run() assigns the value of bdrv_get_meta_dirty_count() to a
> local int64_t variable.  Change it to uint64_t.  Signedness still gets
> mixed up in the computation of s->common.len, but messing with that is
> more than I can handle right now.
> 
> get_remaining_dirty() tallies bdrv_get_dirty_count() values in
> int64_t.  Its caller block_save_pending() converts it back to
> uint64_t.  Change get_remaining_dirty() to uint64_t.
> 
> Signed-off-by: Markus Armbruster 

So these functions can't fail, so there's no reason to keep the int64_t
type around, OK.

Reviewed-by: John Snow 

Re: [Qemu-block] [PATCH for-2.10] block/nfs: fix mutex assertion in nfs_file_close()

[John Snow]

On 08/07/2017 06:29 PM, Jeff Cody wrote:
> Commit c096358e747e88fc7364e40e3c354ee0bb683960 introduced assertion
> checks for when qemu_mutex() functions are called without the
> corresponding qemu_mutex_init() having initialized the mutex.
> 
> This uncovered a latent bug in qemu's nfs driver - in
> nfs_client_close(), the NFSClient structure is overwritten with zeros,
> prior to the mutex being destroyed.
> 
> Go ahead and destroy the mutex in nfs_client_close(), and change where
> we call qemu_mutex_init() so that it is correctly balanced.
> 
> There are also a couple of memory leaks obscured by the memset, so this
> fixes those as well.
> 
> Finally, we should be able to get rid of the memset(), as it isn't
> necessary.
> 
> Signed-off-by: Jeff Cody 


Looks sane to me.

Reviewed-by: John Snow 

Re: [Qemu-block] [PATCH for-2.10 0/5] block: bdrv_reopen() fixes

[John Snow]

On 08/03/2017 11:02 AM, Kevin Wolf wrote:
> This is the first part of some fixes to bdrv_reopen(), which seems
> reasonable enough to merge for 2.10.
> 
> There is much more wrong with bdrv_reopen() currently, especially with
> respect to op blocker permissions (basically the required permissions
> can change based on the options used in bdrv_reopen(), and both things
> affect the whole tree and aren't integrated with each other at all), but
> I have little hope to get this fixed before 2.10, so let's start with
> what is ready now.
> 
> Kevin Wolf (5):
>   block: Fix order in bdrv_replace_child()
>   block: Allow reopen rw without BDRV_O_ALLOW_RDWR
>   block: Set BDRV_O_ALLOW_RDWR during rw reopen
>   qemu-io: Allow reopen read-write
>   qemu-iotests: Test reopen between read-only and read-write
> 
>  include/block/block.h  |  3 +-
>  block.c| 20 +-
>  qemu-io-cmds.c | 19 +++--
>  tests/qemu-iotests/187 | 69 
> ++
>  tests/qemu-iotests/187.out | 18 
>  tests/qemu-iotests/group   |  1 +
>  6 files changed, 120 insertions(+), 10 deletions(-)
>  create mode 100755 tests/qemu-iotests/187
>  create mode 100644 tests/qemu-iotests/187.out
> 

Not that you need it, but I reviewed the series because I wanted to know
what this change was, so while I'm here:

Reviewed-by: John Snow 

Re: [Qemu-block] [PATCH 4/4] qcow2: Check failure of bdrv_getlength()

[Jeff Cody]

On Mon, Aug 07, 2017 at 03:30:07PM -0500, Eric Blake wrote:
> qcow2_co_pwritev_compressed() should not call bdrv_truncate()
> if determining the size failed.
> 
> Reported-by: Markus Armbruster 
> Signed-off-by: Eric Blake 



Reviewed-by: Jeff Cody 


> ---
>  block/qcow2.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 99407403ea..40ba26c111 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3282,12 +3282,15 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, 
> uint64_t offset,
>  z_stream strm;
>  int ret, out_len;
>  uint8_t *buf, *out_buf;
> -uint64_t cluster_offset;
> +int64_t cluster_offset;
> 
>  if (bytes == 0) {
>  /* align end of file to a sector boundary to ease reading with
> sector based I/Os */
>  cluster_offset = bdrv_getlength(bs->file->bs);
> +if (cluster_offset < 0) {
> +return cluster_offset;
> +}
>  return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, 
> NULL);
>  }
> 
> -- 
> 2.13.4
> 
> 

Re: [Qemu-block] [PATCH 2/4] qcow: Check failure of bdrv_getlength() and bdrv_truncate()

[Jeff Cody]

On Mon, Aug 07, 2017 at 03:30:05PM -0500, Eric Blake wrote:
> This also requires changing the return type of get_cluster_offset()
> and adjusting all callers.
> 
> Use osdep.h macros instead of open-coded rounding while in the
> area.
> 
> Reported-by: Markus Armbruster 
> Signed-off-by: Eric Blake 
> ---
>  block/qcow.c | 64 
> ++--
>  1 file changed, 45 insertions(+), 19 deletions(-)
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index c08cdc4a7b..937023d447 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -347,16 +347,17 @@ static int qcow_reopen_prepare(BDRVReopenState *state,
>   * 'compressed_size'. 'compressed_size' must be > 0 and <
>   * cluster_size
>   *
> - * return 0 if not allocated.
> + * return 0 if not allocated, -errno on failure.
>   */
> -static uint64_t get_cluster_offset(BlockDriverState *bs,
> -   uint64_t offset, int allocate,
> -   int compressed_size,
> -   int n_start, int n_end)
> +static int64_t get_cluster_offset(BlockDriverState *bs,
> +  uint64_t offset, int allocate,
> +  int compressed_size,
> +  int n_start, int n_end)
>  {
>  BDRVQcowState *s = bs->opaque;
>  int min_index, i, j, l1_index, l2_index;
> -uint64_t l2_offset, *l2_table, cluster_offset, tmp;
> +int64_t l2_offset, cluster_offset;
> +uint64_t *l2_table, tmp;
>  uint32_t min_count;
>  int new_l2_table;
> 
> @@ -368,8 +369,11 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>  return 0;
>  /* allocate a new l2 entry */
>  l2_offset = bdrv_getlength(bs->file->bs);
> +if (l2_offset < 0) {
> +return l2_offset;
> +}
>  /* round to cluster size */
> -l2_offset = (l2_offset + s->cluster_size - 1) & ~(s->cluster_size - 
> 1);
> +l2_offset = QEMU_ALIGN_UP(l2_offset, s->cluster_size);
>  /* update the L1 entry */
>  s->l1_table[l1_index] = l2_offset;
>  tmp = cpu_to_be64(l2_offset);
> @@ -430,8 +434,10 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>  if (decompress_cluster(bs, cluster_offset) < 0)
>  return 0;
>  cluster_offset = bdrv_getlength(bs->file->bs);
> -cluster_offset = (cluster_offset + s->cluster_size - 1) &
> -~(s->cluster_size - 1);
> +if (cluster_offset < 0) {
> +return cluster_offset;
> +}
> +cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size);
>  /* write the cluster content */
>  if (bdrv_pwrite(bs->file, cluster_offset, s->cluster_cache,
>  s->cluster_size) !=
> @@ -439,12 +445,19 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>  return -1;
>  } else {
>  cluster_offset = bdrv_getlength(bs->file->bs);
> +if (cluster_offset < 0) {
> +return cluster_offset;
> +}
>  if (allocate == 1) {
> +int ret;
> +
>  /* round to cluster size */
> -cluster_offset = (cluster_offset + s->cluster_size - 1) &
> -~(s->cluster_size - 1);
> -bdrv_truncate(bs->file, cluster_offset + s->cluster_size,
> -  PREALLOC_MODE_OFF, NULL);
> +cluster_offset = QEMU_ALIGN_UP(cluster_offset, 
> s->cluster_size);
> +ret = bdrv_truncate(bs->file, cluster_offset + 
> s->cluster_size,
> +PREALLOC_MODE_OFF, NULL);

My r-b stands, but do you think it is worth checking for
(cluster_offset + s->cluster_size) > INT64_MAX?

> +if (ret < 0) {
> +return ret;
> +}
>  /* if encrypted, we must initialize the cluster
> content which won't be written */
>  if (bs->encrypted &&
> @@ -491,11 +504,14 @@ static int64_t coroutine_fn 
> qcow_co_get_block_status(BlockDriverState *bs,
>  {
>  BDRVQcowState *s = bs->opaque;
>  int index_in_cluster, n;
> -uint64_t cluster_offset;
> +int64_t cluster_offset;
> 
>  qemu_co_mutex_lock(>lock);
>  cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
>  qemu_co_mutex_unlock(>lock);
> +if (cluster_offset < 0) {
> +return cluster_offset;
> +}
>  index_in_cluster = sector_num & (s->cluster_sectors - 1);
>  n = s->cluster_sectors - index_in_cluster;
>  if (n > nb_sectors)
> @@ -567,7 +583,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState 
> *bs, int64_t sector_num,
>  BDRVQcowState *s = bs->opaque;
>  int index_in_cluster;
>  int ret = 

Re: [Qemu-block] [PATCH 3/4] qcow2: Drop debugging dump_refcounts()

[Jeff Cody]

On Mon, Aug 07, 2017 at 03:30:06PM -0500, Eric Blake wrote:
> It's been #if 0'd since its introduction in 2006, commit 585f8587.
> We can revive dead code if we need it, but in the meantime, it has
> bit-rotted (for example, not checking for failure in bdrv_getlength()).
> 
> Signed-off-by: Eric Blake 


Reviewed-by: Jeff Cody 


> ---
>  block/qcow2.c | 21 -
>  1 file changed, 21 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index d7c600b5a2..99407403ea 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3798,27 +3798,6 @@ static ImageInfoSpecific 
> *qcow2_get_specific_info(BlockDriverState *bs)
>  return spec_info;
>  }
> 
> -#if 0
> -static void dump_refcounts(BlockDriverState *bs)
> -{
> -BDRVQcow2State *s = bs->opaque;
> -int64_t nb_clusters, k, k1, size;
> -int refcount;
> -
> -size = bdrv_getlength(bs->file->bs);
> -nb_clusters = size_to_clusters(s, size);
> -for(k = 0; k < nb_clusters;) {
> -k1 = k;
> -refcount = get_refcount(bs, k);
> -k++;
> -while (k < nb_clusters && get_refcount(bs, k) == refcount)
> -k++;
> -printf("%" PRId64 ": refcount=%d nb=%" PRId64 "\n", k, refcount,
> -   k - k1);
> -}
> -}
> -#endif
> -
>  static int qcow2_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
>int64_t pos)
>  {
> -- 
> 2.13.4
> 
> 

Re: [Qemu-block] [PATCH 2/4] qcow: Check failure of bdrv_getlength() and bdrv_truncate()

[Jeff Cody]

On Mon, Aug 07, 2017 at 03:30:05PM -0500, Eric Blake wrote:
> This also requires changing the return type of get_cluster_offset()
> and adjusting all callers.
> 
> Use osdep.h macros instead of open-coded rounding while in the
> area.
> 
> Reported-by: Markus Armbruster 
> Signed-off-by: Eric Blake 



Reviewed-by: Jeff Cody 



> ---
>  block/qcow.c | 64 
> ++--
>  1 file changed, 45 insertions(+), 19 deletions(-)
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index c08cdc4a7b..937023d447 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -347,16 +347,17 @@ static int qcow_reopen_prepare(BDRVReopenState *state,
>   * 'compressed_size'. 'compressed_size' must be > 0 and <
>   * cluster_size
>   *
> - * return 0 if not allocated.
> + * return 0 if not allocated, -errno on failure.
>   */
> -static uint64_t get_cluster_offset(BlockDriverState *bs,
> -   uint64_t offset, int allocate,
> -   int compressed_size,
> -   int n_start, int n_end)
> +static int64_t get_cluster_offset(BlockDriverState *bs,
> +  uint64_t offset, int allocate,
> +  int compressed_size,
> +  int n_start, int n_end)
>  {
>  BDRVQcowState *s = bs->opaque;
>  int min_index, i, j, l1_index, l2_index;
> -uint64_t l2_offset, *l2_table, cluster_offset, tmp;
> +int64_t l2_offset, cluster_offset;
> +uint64_t *l2_table, tmp;
>  uint32_t min_count;
>  int new_l2_table;
> 
> @@ -368,8 +369,11 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>  return 0;
>  /* allocate a new l2 entry */
>  l2_offset = bdrv_getlength(bs->file->bs);
> +if (l2_offset < 0) {
> +return l2_offset;
> +}
>  /* round to cluster size */
> -l2_offset = (l2_offset + s->cluster_size - 1) & ~(s->cluster_size - 
> 1);
> +l2_offset = QEMU_ALIGN_UP(l2_offset, s->cluster_size);
>  /* update the L1 entry */
>  s->l1_table[l1_index] = l2_offset;
>  tmp = cpu_to_be64(l2_offset);
> @@ -430,8 +434,10 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>  if (decompress_cluster(bs, cluster_offset) < 0)
>  return 0;
>  cluster_offset = bdrv_getlength(bs->file->bs);
> -cluster_offset = (cluster_offset + s->cluster_size - 1) &
> -~(s->cluster_size - 1);
> +if (cluster_offset < 0) {
> +return cluster_offset;
> +}
> +cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size);
>  /* write the cluster content */
>  if (bdrv_pwrite(bs->file, cluster_offset, s->cluster_cache,
>  s->cluster_size) !=
> @@ -439,12 +445,19 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>  return -1;
>  } else {
>  cluster_offset = bdrv_getlength(bs->file->bs);
> +if (cluster_offset < 0) {
> +return cluster_offset;
> +}
>  if (allocate == 1) {
> +int ret;
> +
>  /* round to cluster size */
> -cluster_offset = (cluster_offset + s->cluster_size - 1) &
> -~(s->cluster_size - 1);
> -bdrv_truncate(bs->file, cluster_offset + s->cluster_size,
> -  PREALLOC_MODE_OFF, NULL);
> +cluster_offset = QEMU_ALIGN_UP(cluster_offset, 
> s->cluster_size);
> +ret = bdrv_truncate(bs->file, cluster_offset + 
> s->cluster_size,
> +PREALLOC_MODE_OFF, NULL);
> +if (ret < 0) {
> +return ret;
> +}
>  /* if encrypted, we must initialize the cluster
> content which won't be written */
>  if (bs->encrypted &&
> @@ -491,11 +504,14 @@ static int64_t coroutine_fn 
> qcow_co_get_block_status(BlockDriverState *bs,
>  {
>  BDRVQcowState *s = bs->opaque;
>  int index_in_cluster, n;
> -uint64_t cluster_offset;
> +int64_t cluster_offset;
> 
>  qemu_co_mutex_lock(>lock);
>  cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
>  qemu_co_mutex_unlock(>lock);
> +if (cluster_offset < 0) {
> +return cluster_offset;
> +}
>  index_in_cluster = sector_num & (s->cluster_sectors - 1);
>  n = s->cluster_sectors - index_in_cluster;
>  if (n > nb_sectors)
> @@ -567,7 +583,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState 
> *bs, int64_t sector_num,
>  BDRVQcowState *s = bs->opaque;
>  int index_in_cluster;
>  int ret = 0, n;
> -uint64_t cluster_offset;
> +int64_t 

Re: [Qemu-block] [PATCH 1/4] vpc: Check failure of bdrv_getlength()

[Jeff Cody]

On Mon, Aug 07, 2017 at 03:30:04PM -0500, Eric Blake wrote:
> vpc_open() was checking for bdrv_getlength() failure in one, but
> not the other, location.
> 
> Reported-by: Markus Armbruster 
> Signed-off-by: Eric Blake 
> ---
>  block/vpc.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/block/vpc.c b/block/vpc.c
> index 574879ba7c..468d10ec1c 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -219,6 +219,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
> int flags,
>  uint64_t pagetable_size;
>  int disk_type = VHD_DYNAMIC;
>  int ret;
> +int64_t bs_size;
> 
>  bs->file = bdrv_open_child(NULL, options, "file", bs, _file,
> false, errp);
> @@ -411,7 +412,13 @@ static int vpc_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  }
>  }
> 
> -if (s->free_data_block_offset > bdrv_getlength(bs->file->bs)) {
> +bs_size = bdrv_getlength(bs->file->bs);
> +if (bs_size < 0) {
> +error_setg_errno(errp, -bs_size, "unable to learn image size");
> +ret = bs_size;
> +goto fail;
> +}
> +if (s->free_data_block_offset > bs_size) {
>  error_setg(errp, "block-vpc: free_data_block_offset points after 
> "
>   "the end of file. The image has been 
> truncated.");
>  ret = -EINVAL;
> -- 
> 2.13.4
> 
> 

Reviewed-by: Jeff Cody 

Re: [Qemu-block] [PATCH for-2.10 0/4] More blk_getlength() fixes

[John Snow]

On 08/07/2017 04:30 PM, Eric Blake wrote:
> Thanks again to Markus for catching these spots.
> 
> Eric Blake (4):
>   vpc: Check failure of bdrv_getlength()
>   qcow: Check failure of bdrv_getlength() and bdrv_truncate()
>   qcow2: Drop debugging dump_refcounts()
>   qcow2: Check failure of bdrv_getlength()
> 
>  block/qcow.c  | 64 
> +--
>  block/qcow2.c | 26 
>  block/vpc.c   |  9 -
>  3 files changed, 57 insertions(+), 42 deletions(-)
> 

Reviewed-by: John Snow 

[Qemu-block] [PATCH for-2.10] block/nfs: fix mutex assertion in nfs_file_close()

[Jeff Cody]

Commit c096358e747e88fc7364e40e3c354ee0bb683960 introduced assertion
checks for when qemu_mutex() functions are called without the
corresponding qemu_mutex_init() having initialized the mutex.

This uncovered a latent bug in qemu's nfs driver - in
nfs_client_close(), the NFSClient structure is overwritten with zeros,
prior to the mutex being destroyed.

Go ahead and destroy the mutex in nfs_client_close(), and change where
we call qemu_mutex_init() so that it is correctly balanced.

There are also a couple of memory leaks obscured by the memset, so this
fixes those as well.

Finally, we should be able to get rid of the memset(), as it isn't
necessary.

Signed-off-by: Jeff Cody 
---
 block/nfs.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/block/nfs.c b/block/nfs.c
index d8db419..bec16b7 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -433,19 +433,23 @@ static void nfs_client_close(NFSClient *client)
 if (client->context) {
 if (client->fh) {
 nfs_close(client->context, client->fh);
+client->fh = NULL;
 }
 aio_set_fd_handler(client->aio_context, nfs_get_fd(client->context),
false, NULL, NULL, NULL, NULL);
 nfs_destroy_context(client->context);
+client->context = NULL;
 }
-memset(client, 0, sizeof(NFSClient));
-}
-
-static void nfs_file_close(BlockDriverState *bs)
-{
-NFSClient *client = bs->opaque;
-nfs_client_close(client);
+g_free(client->path);
 qemu_mutex_destroy(>mutex);
+qapi_free_NFSServer(client->server);
+client->server = NULL;
+}
+
+static void nfs_file_close(BlockDriverState *bs)
+{
+NFSClient *client = bs->opaque;
+nfs_client_close(client);
 }
 
 static NFSServer *nfs_config(QDict *options, Error **errp)
@@ -498,6 +502,7 @@ static int64_t nfs_client_open(NFSClient *client, QDict 
*options,
 struct stat st;
 char *file = NULL, *strp = NULL;
 
+qemu_mutex_init(>mutex);
 opts = qemu_opts_create(_opts, NULL, 0, _abort);
 qemu_opts_absorb_qdict(opts, options, _err);
 if (local_err) {
@@ -660,7 +665,7 @@ static int nfs_file_open(BlockDriverState *bs, QDict 
*options, int flags,
 if (ret < 0) {
 return ret;
 }
-qemu_mutex_init(>mutex);
+
 bs->total_sectors = ret;
 ret = 0;
 return ret;
-- 
2.9.4

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10] throttle: Make LeakyBucket.avg and LeakyBucket.max integer types

[Eric Blake]

On 08/07/2017 11:15 AM, Alberto Garcia wrote:
> Both the throttling limits set with the throttling.iops-* and
> throttling.bps-* options and their QMP equivalents defined in the
> BlockIOThrottle struct are integer values.
> 
> Those limits are also reported in the BlockDeviceInfo struct and they
> are integers there as well.
> 
> Therefore there's no reason to store them internally as double and do
> the conversion everytime we're setting or querying them, so this patch
> uses int64_t for those types.
> 
> LeakyBucket.level and LeakyBucket.burst_level do however remain double
> because their value changes depending on the fraction of time elapsed
> since the previous I/O operation.
> 
> There's one particular instance of the previous code where bkt->max
> could have a non-integer value: that's in throttle_fix_bucket() when
> bkt->max is initialized to bkt->avg / 10. This is now an integer
> division and the result is rounded. We don't need to worry about this
> because:
> 
>a) with the magnitudes we're dealing with (bytes per second, I/O
>   operations per second) the limits are likely to be always
>   multiples of 10.
> 
>b) even if they weren't this doesn't affect the actual limits, only
>   the algorithm that makes the throttling smoother.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  include/qemu/throttle.h | 4 ++--
>  util/throttle.c | 7 ++-
>  2 files changed, 4 insertions(+), 7 deletions(-)

Reviewed-by: Eric Blake 

-- 
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-block] [Qemu-devel] [PATCH 2/4] qcow: Check failure of bdrv_getlength() and bdrv_truncate()

[Philippe Mathieu-Daudé]

On 08/07/2017 05:30 PM, Eric Blake wrote:

This also requires changing the return type of get_cluster_offset()
and adjusting all callers.

Use osdep.h macros instead of open-coded rounding while in the
area.

Reported-by: Markus Armbruster 
Signed-off-by: Eric Blake 


Reviewed-by: Philippe Mathieu-Daudé 


---
  block/qcow.c | 64 ++--
  1 file changed, 45 insertions(+), 19 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index c08cdc4a7b..937023d447 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -347,16 +347,17 @@ static int qcow_reopen_prepare(BDRVReopenState *state,
   * 'compressed_size'. 'compressed_size' must be > 0 and <
   * cluster_size
   *
- * return 0 if not allocated.
+ * return 0 if not allocated, -errno on failure.
   */
-static uint64_t get_cluster_offset(BlockDriverState *bs,
-   uint64_t offset, int allocate,
-   int compressed_size,
-   int n_start, int n_end)
+static int64_t get_cluster_offset(BlockDriverState *bs,
+  uint64_t offset, int allocate,
+  int compressed_size,
+  int n_start, int n_end)
  {
  BDRVQcowState *s = bs->opaque;
  int min_index, i, j, l1_index, l2_index;
-uint64_t l2_offset, *l2_table, cluster_offset, tmp;
+int64_t l2_offset, cluster_offset;
+uint64_t *l2_table, tmp;
  uint32_t min_count;
  int new_l2_table;

@@ -368,8 +369,11 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
  return 0;
  /* allocate a new l2 entry */
  l2_offset = bdrv_getlength(bs->file->bs);
+if (l2_offset < 0) {
+return l2_offset;
+}
  /* round to cluster size */
-l2_offset = (l2_offset + s->cluster_size - 1) & ~(s->cluster_size - 1);
+l2_offset = QEMU_ALIGN_UP(l2_offset, s->cluster_size);
  /* update the L1 entry */
  s->l1_table[l1_index] = l2_offset;
  tmp = cpu_to_be64(l2_offset);
@@ -430,8 +434,10 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
  if (decompress_cluster(bs, cluster_offset) < 0)
  return 0;
  cluster_offset = bdrv_getlength(bs->file->bs);
-cluster_offset = (cluster_offset + s->cluster_size - 1) &
-~(s->cluster_size - 1);
+if (cluster_offset < 0) {
+return cluster_offset;
+}
+cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size);
  /* write the cluster content */
  if (bdrv_pwrite(bs->file, cluster_offset, s->cluster_cache,
  s->cluster_size) !=
@@ -439,12 +445,19 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
  return -1;
  } else {
  cluster_offset = bdrv_getlength(bs->file->bs);
+if (cluster_offset < 0) {
+return cluster_offset;
+}
  if (allocate == 1) {
+int ret;
+
  /* round to cluster size */
-cluster_offset = (cluster_offset + s->cluster_size - 1) &
-~(s->cluster_size - 1);
-bdrv_truncate(bs->file, cluster_offset + s->cluster_size,
-  PREALLOC_MODE_OFF, NULL);
+cluster_offset = QEMU_ALIGN_UP(cluster_offset, 
s->cluster_size);
+ret = bdrv_truncate(bs->file, cluster_offset + s->cluster_size,
+PREALLOC_MODE_OFF, NULL);
+if (ret < 0) {
+return ret;
+}
  /* if encrypted, we must initialize the cluster
 content which won't be written */
  if (bs->encrypted &&
@@ -491,11 +504,14 @@ static int64_t coroutine_fn 
qcow_co_get_block_status(BlockDriverState *bs,
  {
  BDRVQcowState *s = bs->opaque;
  int index_in_cluster, n;
-uint64_t cluster_offset;
+int64_t cluster_offset;

  qemu_co_mutex_lock(>lock);
  cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
  qemu_co_mutex_unlock(>lock);
+if (cluster_offset < 0) {
+return cluster_offset;
+}
  index_in_cluster = sector_num & (s->cluster_sectors - 1);
  n = s->cluster_sectors - index_in_cluster;
  if (n > nb_sectors)
@@ -567,7 +583,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, 
int64_t sector_num,
  BDRVQcowState *s = bs->opaque;
  int index_in_cluster;
  int ret = 0, n;
-uint64_t cluster_offset;
+int64_t cluster_offset;
  struct iovec hd_iov;
  QEMUIOVector hd_qiov;
  uint8_t *buf;
@@ -588,8 +604,10 @@ static coroutine_fn int qcow_co_readv(BlockDriverState 
*bs, int64_t sector_num,

  while 

Re: [Qemu-block] [Qemu-devel] [PATCH 4/4] qcow2: Check failure of bdrv_getlength()

[Philippe Mathieu-Daudé]

On 08/07/2017 05:30 PM, Eric Blake wrote:

qcow2_co_pwritev_compressed() should not call bdrv_truncate()
if determining the size failed.

Reported-by: Markus Armbruster 
Signed-off-by: Eric Blake 


Reviewed-by: Philippe Mathieu-Daudé 


---
  block/qcow2.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 99407403ea..40ba26c111 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3282,12 +3282,15 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, 
uint64_t offset,
  z_stream strm;
  int ret, out_len;
  uint8_t *buf, *out_buf;
-uint64_t cluster_offset;
+int64_t cluster_offset;

  if (bytes == 0) {
  /* align end of file to a sector boundary to ease reading with
 sector based I/Os */
  cluster_offset = bdrv_getlength(bs->file->bs);
+if (cluster_offset < 0) {
+return cluster_offset;
+}
  return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, 
NULL);
  }

Re: [Qemu-block] [Qemu-devel] [PATCH 3/4] qcow2: Drop debugging dump_refcounts()

[Philippe Mathieu-Daudé]

On 08/07/2017 05:30 PM, Eric Blake wrote:

It's been #if 0'd since its introduction in 2006, commit 585f8587.
We can revive dead code if we need it, but in the meantime, it has
bit-rotted (for example, not checking for failure in bdrv_getlength()).

Signed-off-by: Eric Blake 


Reviewed-by: Philippe Mathieu-Daudé 


--- >   block/qcow2.c | 21 -
  1 file changed, 21 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index d7c600b5a2..99407403ea 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3798,27 +3798,6 @@ static ImageInfoSpecific 
*qcow2_get_specific_info(BlockDriverState *bs)
  return spec_info;
  }

-#if 0
-static void dump_refcounts(BlockDriverState *bs)
-{
-BDRVQcow2State *s = bs->opaque;
-int64_t nb_clusters, k, k1, size;
-int refcount;
-
-size = bdrv_getlength(bs->file->bs);
-nb_clusters = size_to_clusters(s, size);
-for(k = 0; k < nb_clusters;) {
-k1 = k;
-refcount = get_refcount(bs, k);
-k++;
-while (k < nb_clusters && get_refcount(bs, k) == refcount)
-k++;
-printf("%" PRId64 ": refcount=%d nb=%" PRId64 "\n", k, refcount,
-   k - k1);
-}
-}
-#endif
-
  static int qcow2_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
int64_t pos)
  {

Re: [Qemu-block] [Qemu-devel] [PATCH 1/4] vpc: Check failure of bdrv_getlength()

[Philippe Mathieu-Daudé]

On 08/07/2017 05:30 PM, Eric Blake wrote:

vpc_open() was checking for bdrv_getlength() failure in one, but
not the other, location.

Reported-by: Markus Armbruster 
Signed-off-by: Eric Blake 


Reviewed-by: Philippe Mathieu-Daudé 


---
  block/vpc.c | 9 -
  1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/block/vpc.c b/block/vpc.c
index 574879ba7c..468d10ec1c 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -219,6 +219,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
int flags,
  uint64_t pagetable_size;
  int disk_type = VHD_DYNAMIC;
  int ret;
+int64_t bs_size;

  bs->file = bdrv_open_child(NULL, options, "file", bs, _file,
 false, errp);
@@ -411,7 +412,13 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
int flags,
  }
  }

-if (s->free_data_block_offset > bdrv_getlength(bs->file->bs)) {
+bs_size = bdrv_getlength(bs->file->bs);
+if (bs_size < 0) {
+error_setg_errno(errp, -bs_size, "unable to learn image size");
+ret = bs_size;
+goto fail;
+}
+if (s->free_data_block_offset > bs_size) {
  error_setg(errp, "block-vpc: free_data_block_offset points after "
   "the end of file. The image has been 
truncated.");
  ret = -EINVAL;

[Qemu-block] [PATCH 3/4] qcow2: Drop debugging dump_refcounts()

[Eric Blake]

It's been #if 0'd since its introduction in 2006, commit 585f8587.
We can revive dead code if we need it, but in the meantime, it has
bit-rotted (for example, not checking for failure in bdrv_getlength()).

Signed-off-by: Eric Blake 
---
 block/qcow2.c | 21 -
 1 file changed, 21 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index d7c600b5a2..99407403ea 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3798,27 +3798,6 @@ static ImageInfoSpecific 
*qcow2_get_specific_info(BlockDriverState *bs)
 return spec_info;
 }

-#if 0
-static void dump_refcounts(BlockDriverState *bs)
-{
-BDRVQcow2State *s = bs->opaque;
-int64_t nb_clusters, k, k1, size;
-int refcount;
-
-size = bdrv_getlength(bs->file->bs);
-nb_clusters = size_to_clusters(s, size);
-for(k = 0; k < nb_clusters;) {
-k1 = k;
-refcount = get_refcount(bs, k);
-k++;
-while (k < nb_clusters && get_refcount(bs, k) == refcount)
-k++;
-printf("%" PRId64 ": refcount=%d nb=%" PRId64 "\n", k, refcount,
-   k - k1);
-}
-}
-#endif
-
 static int qcow2_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
   int64_t pos)
 {
-- 
2.13.4

[Qemu-block] [PATCH for-2.10 0/4] More blk_getlength() fixes

[Eric Blake]

Thanks again to Markus for catching these spots.

Eric Blake (4):
  vpc: Check failure of bdrv_getlength()
  qcow: Check failure of bdrv_getlength() and bdrv_truncate()
  qcow2: Drop debugging dump_refcounts()
  qcow2: Check failure of bdrv_getlength()

 block/qcow.c  | 64 +--
 block/qcow2.c | 26 
 block/vpc.c   |  9 -
 3 files changed, 57 insertions(+), 42 deletions(-)

-- 
2.13.4

[Qemu-block] [PATCH 1/4] vpc: Check failure of bdrv_getlength()

[Eric Blake]

vpc_open() was checking for bdrv_getlength() failure in one, but
not the other, location.

Reported-by: Markus Armbruster 
Signed-off-by: Eric Blake 
---
 block/vpc.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/block/vpc.c b/block/vpc.c
index 574879ba7c..468d10ec1c 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -219,6 +219,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
int flags,
 uint64_t pagetable_size;
 int disk_type = VHD_DYNAMIC;
 int ret;
+int64_t bs_size;

 bs->file = bdrv_open_child(NULL, options, "file", bs, _file,
false, errp);
@@ -411,7 +412,13 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
int flags,
 }
 }

-if (s->free_data_block_offset > bdrv_getlength(bs->file->bs)) {
+bs_size = bdrv_getlength(bs->file->bs);
+if (bs_size < 0) {
+error_setg_errno(errp, -bs_size, "unable to learn image size");
+ret = bs_size;
+goto fail;
+}
+if (s->free_data_block_offset > bs_size) {
 error_setg(errp, "block-vpc: free_data_block_offset points after "
  "the end of file. The image has been truncated.");
 ret = -EINVAL;
-- 
2.13.4

[Qemu-block] [PATCH 4/4] qcow2: Check failure of bdrv_getlength()

[Eric Blake]

qcow2_co_pwritev_compressed() should not call bdrv_truncate()
if determining the size failed.

Reported-by: Markus Armbruster 
Signed-off-by: Eric Blake 
---
 block/qcow2.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 99407403ea..40ba26c111 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3282,12 +3282,15 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, 
uint64_t offset,
 z_stream strm;
 int ret, out_len;
 uint8_t *buf, *out_buf;
-uint64_t cluster_offset;
+int64_t cluster_offset;

 if (bytes == 0) {
 /* align end of file to a sector boundary to ease reading with
sector based I/Os */
 cluster_offset = bdrv_getlength(bs->file->bs);
+if (cluster_offset < 0) {
+return cluster_offset;
+}
 return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, 
NULL);
 }

-- 
2.13.4

[Qemu-block] [PATCH 2/4] qcow: Check failure of bdrv_getlength() and bdrv_truncate()

[Eric Blake]

This also requires changing the return type of get_cluster_offset()
and adjusting all callers.

Use osdep.h macros instead of open-coded rounding while in the
area.

Reported-by: Markus Armbruster 
Signed-off-by: Eric Blake 
---
 block/qcow.c | 64 ++--
 1 file changed, 45 insertions(+), 19 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index c08cdc4a7b..937023d447 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -347,16 +347,17 @@ static int qcow_reopen_prepare(BDRVReopenState *state,
  * 'compressed_size'. 'compressed_size' must be > 0 and <
  * cluster_size
  *
- * return 0 if not allocated.
+ * return 0 if not allocated, -errno on failure.
  */
-static uint64_t get_cluster_offset(BlockDriverState *bs,
-   uint64_t offset, int allocate,
-   int compressed_size,
-   int n_start, int n_end)
+static int64_t get_cluster_offset(BlockDriverState *bs,
+  uint64_t offset, int allocate,
+  int compressed_size,
+  int n_start, int n_end)
 {
 BDRVQcowState *s = bs->opaque;
 int min_index, i, j, l1_index, l2_index;
-uint64_t l2_offset, *l2_table, cluster_offset, tmp;
+int64_t l2_offset, cluster_offset;
+uint64_t *l2_table, tmp;
 uint32_t min_count;
 int new_l2_table;

@@ -368,8 +369,11 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
 return 0;
 /* allocate a new l2 entry */
 l2_offset = bdrv_getlength(bs->file->bs);
+if (l2_offset < 0) {
+return l2_offset;
+}
 /* round to cluster size */
-l2_offset = (l2_offset + s->cluster_size - 1) & ~(s->cluster_size - 1);
+l2_offset = QEMU_ALIGN_UP(l2_offset, s->cluster_size);
 /* update the L1 entry */
 s->l1_table[l1_index] = l2_offset;
 tmp = cpu_to_be64(l2_offset);
@@ -430,8 +434,10 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
 if (decompress_cluster(bs, cluster_offset) < 0)
 return 0;
 cluster_offset = bdrv_getlength(bs->file->bs);
-cluster_offset = (cluster_offset + s->cluster_size - 1) &
-~(s->cluster_size - 1);
+if (cluster_offset < 0) {
+return cluster_offset;
+}
+cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size);
 /* write the cluster content */
 if (bdrv_pwrite(bs->file, cluster_offset, s->cluster_cache,
 s->cluster_size) !=
@@ -439,12 +445,19 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
 return -1;
 } else {
 cluster_offset = bdrv_getlength(bs->file->bs);
+if (cluster_offset < 0) {
+return cluster_offset;
+}
 if (allocate == 1) {
+int ret;
+
 /* round to cluster size */
-cluster_offset = (cluster_offset + s->cluster_size - 1) &
-~(s->cluster_size - 1);
-bdrv_truncate(bs->file, cluster_offset + s->cluster_size,
-  PREALLOC_MODE_OFF, NULL);
+cluster_offset = QEMU_ALIGN_UP(cluster_offset, 
s->cluster_size);
+ret = bdrv_truncate(bs->file, cluster_offset + s->cluster_size,
+PREALLOC_MODE_OFF, NULL);
+if (ret < 0) {
+return ret;
+}
 /* if encrypted, we must initialize the cluster
content which won't be written */
 if (bs->encrypted &&
@@ -491,11 +504,14 @@ static int64_t coroutine_fn 
qcow_co_get_block_status(BlockDriverState *bs,
 {
 BDRVQcowState *s = bs->opaque;
 int index_in_cluster, n;
-uint64_t cluster_offset;
+int64_t cluster_offset;

 qemu_co_mutex_lock(>lock);
 cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
 qemu_co_mutex_unlock(>lock);
+if (cluster_offset < 0) {
+return cluster_offset;
+}
 index_in_cluster = sector_num & (s->cluster_sectors - 1);
 n = s->cluster_sectors - index_in_cluster;
 if (n > nb_sectors)
@@ -567,7 +583,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, 
int64_t sector_num,
 BDRVQcowState *s = bs->opaque;
 int index_in_cluster;
 int ret = 0, n;
-uint64_t cluster_offset;
+int64_t cluster_offset;
 struct iovec hd_iov;
 QEMUIOVector hd_qiov;
 uint8_t *buf;
@@ -588,8 +604,10 @@ static coroutine_fn int qcow_co_readv(BlockDriverState 
*bs, int64_t sector_num,

 while (nb_sectors != 0) {
 /* prepare next request */
-cluster_offset = get_cluster_offset(bs, sector_num << 9,
-

Re: [Qemu-block] Is the use of bdrv_getlength() in qcow.c kosher?

[Eric Blake]

On 08/04/2017 10:32 AM, Eric Blake wrote:
> On 08/04/2017 07:45 AM, Markus Armbruster wrote:
>> Markus Armbruster  writes:
>>
>>> bdrv_getlength() can fail.  The uses in qcow.c don't check.  Is that
>>> safe?
>>
>> There's another one in qcow2_co_pwritev_compressed().
>>
>> Yet another one in dump_refcounts().

I haven't seen a patch for these yet, so I'll submit one shortly.

>>
>> While we're talking: the one in qcow2_measure() assigns to a variable of
>> type ssize_t.  Should be int64_t.
> 
> Oh indeed.  I wonder if my recently-added qemu-iotests 190 will catch
> that on a 32-bit machine?  If not, it should...

I can't trigger a failure, because the code in question is doing:


int64_t ssize = bdrv_getlength(in_bs);
if (ssize < 0) {
error_setg_errno(_err, -ssize,
 "Unable to get image virtual_size");
goto err;
}

Yes, the variable name is odd, but the types look correct.

-- 
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-block] [RFC PATCH 16/56] migration: Make XBZRLE transferred size unsigned in QAPI/QMP

[Juan Quintela]

Markus Armbruster  wrote:
> Sizes should use QAPI type 'size' (uint64_t).  XBZRLECacheStats member
> @bytes is 'int' (int64_t).  save_xbzrle_page() computes the byte count
> increment in size_t, implicitly converts it to int, then adds that to
> @bytes.
>
> Change the XBZRLECacheStats member to 'size' and clean up
> save_xbzrle_page().
>
> query-migrate now reports transferred sizes above 2^63-1 correctly
> instead of their (negative) two's complement.
>
> HMP's "info migrate" already reported them correctly, because it
> printed the signed integer with PRIu64.
>
> Signed-off-by: Markus Armbruster 

Reviewed-by: Juan Quintela 

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10] iotests: fix 185

[Vladimir Sementsov-Ogievskiy]

07.08.2017 18:57, Vladimir Sementsov-Ogievskiy wrote:

07.08.2017 18:46, Vladimir Sementsov-Ogievskiy wrote:

07.08.2017 17:29, Eric Blake wrote:

On 08/07/2017 09:16 AM, Vladimir Sementsov-Ogievskiy wrote:

185 iotest is broken.

How to test:

i=0; while ./check -qcow2 -nocache 185; do ((i+=1)); echo N = $i; \

   done; echo N = $i

finished for me like this:

185 2s ... - output mismatch (see 185.out.bad)
--- /work/src/qemu/master/tests/qemu-iotests/185.out 2017-07-14 \
 15:14:29.520343805 +0300
+++ 185.out.bad 2017-08-07 16:51:02.231922900 +0300
@@ -37,7 +37,7 @@
  {"return": {}}
  {"return": {}}
  {"timestamp": {"seconds":  TIMESTAMP, "microseconds": TIMESTAMP}, \
  "event": "SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds": TIMESTAMP}, \
 "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", \
 "len": 4194304, "offset": 4194304, "speed": 65536, "type": \
 "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds": TIMESTAMP}, \
 "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", \
 "len": 0, "offset": 0, "speed": 65536, "type": "mirror"}}

This diff says both 'len' and 'offset' differ...


  === Start backup job and exit qemu ===

Failures: 185
Failed 1 of 1 tests
N = 8

It doesn't seems logical to expect the constant offset on cancel,
so let filter it out.

s/let/let's/


Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  tests/qemu-iotests/185 | 2 +-
  tests/qemu-iotests/185.out | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds": TIMESTAMP}, 
"event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 
4194304, "offset": 4194304, "speed": 65536, "type": "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds": TIMESTAMP}, 
"event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 
4194304, "offset": OFFSET, "speed": 65536, "type": "mirror"}}

...while this diff only touched 'offset'.  Did you copy-and-paste
incorrectly in the commit message?  If so, then with the commit message
fixed, I'm okay with:
Reviewed-by: Eric Blake 

Hmm, I can't reproduce with "len = 0", and it looks impossible. But 
I've scrolled up in one of my terminals and found this reproduce, 
really, '"len": 0', so, that is possible.


reproduced on N = 426




And, after looking through the code it looks like it really possible 
- s->common.len is set not in the very beginning, but after for ex. 
call to block_job_pause_point().


So, the reproduction that I've copied into commit message was very 
good match.


But, I'm not sure, that len should be fixed in the same way. May be 
it would be better to set earlier and don't update on each iteration 
(why is it updated??)





So, IMHO, this patch is good (with s/"len": 0/"len": 4194304/ and 
s/let/let's/ in commit message)





--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH 06/17] block/nbd-client: fix nbd_read_reply_entry

[Eric Blake]

On 08/07/2017 11:09 AM, Vladimir Sementsov-Ogievskiy wrote:

> On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Set reply.handle to 0 on error path to prevent normal path of
>> nbd_co_receive_reply.

>>
>> The client still tried to send a flush request to the server, when it
>> should REALLY quit talking to the server at all and just disconnect,
>> because the moment the client recognizes that the server has sent
>> garbage is the moment that the client can no longer assume that the
>> server will react correctly to any further commands.
>>
>> So I don't think your patch is quite correct, even if you have
>> identified a shortfall in our client code.
>>
> Why do you think so? It just does what it states in commit message.

The commit message doesn't state much on the way of WHY.  It it the
subsequent discussion that says that the reason WHY we need to fix
something is to make the client robustly hang up when it encounters a
broken server - but once you couch it in those terms, this patch is NOT
making the client hang up gracefully (it is making the client skip ONE
invalid reply, but then immediately lets the client send another request
and gets stuck waiting for a reply to that second request).  A full fix
for the issue would make sure the client hangs up as soon as it detects
a bogus server.

> 
> Patch 17 should fix the case (but I doubt that it can be taken separately).

It's okay if it takes more than one patch to fix an issue, if the
earlier patches document that more work is still needed.  Or maybe we
can squash the best parts of 17 and this one.  I'm still playing with
the best way to minimally address the issue for 2.10.

-- 
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-block] [RFC PATCH 15/56] migration: Make XBZRLE cache size unsigned in QAPI/QMP

[Juan Quintela]

Markus Armbruster  wrote:
> Sizes should use QAPI type 'size' (uint64_t).  migrate-set-cache-size
> parameter @value is 'int' (int64_t).  qmp_migrate_set_cache_size()
> ensures it fits into size_t.  page_cache.c implicitly converts the
> signed size to unsigned types (it can't quite decide whether to use
> uint64_t or size_t for cache offsets, but that's not something I can
> tackle now).
>
> XBZRLECacheStats member @cache-size and query-migrate-cache-size's
> result are also 'int'.
>
> Change the migrate-set-cache-size parameter and the XBZRLECacheStats
> members to 'size', fix up hmp_migrate_set_cache_size(), and tweak a
> few variable types to reduce implicit conversions.
>
> migrate-set-cache-size now accepts size values between 2^63 and
> 2^64-1.  It accepts negative values as before, because that's how the
> QObject input visitor works for backward compatibility.
>
> So does HMP's migrate_set_cache_size.
>
> query-migrate and query-migrate-cache-size now report cache sizes
> above 2^63-1 correctly instead of their (negative) two's complement.
>
> So does HMP's "info migrate_cache_size".  HMP's "info migrate" already
> reported the cache size correctly, because it printed the signed
> integer with PRIu32.
>

Reviewed-by: Juan Quintela 


> diff --git a/qapi-schema.json b/qapi-schema.json
> index c8cceb9..ecabff6 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -646,7 +646,7 @@
>  # Since: 1.2
>  ##
>  { 'struct': 'XBZRLECacheStats',
> -  'data': {'cache-size': 'int', 'bytes': 'int', 'pages': 'int',
> +  'data': {'cache-size': 'size', 'bytes': 'int', 'pages': 'int',
> 'cache-miss': 'int', 'cache-miss-rate': 'number',
> 'overflow': 'int' } }
>  
> @@ -2875,7 +2875,7 @@
>  # <- { "return": {} }
>  #
>  ##
> -{ 'command': 'migrate-set-cache-size', 'data': {'value': 'int'} }
> +{ 'command': 'migrate-set-cache-size', 'data': {'value': 'size'} }
>  
>  ##
>  # @query-migrate-cache-size:
> @@ -2892,7 +2892,7 @@
>  # <- { "return": 67108864 }
>  #
>  ##
> -{ 'command': 'query-migrate-cache-size', 'returns': 'int' }
> +{ 'command': 'query-migrate-cache-size', 'returns': 'size' }
>  
>  ##
>  # @ObjectPropertyInfo:

I am ussming this bits are backward compatible (I don't understand QMP
to assure this)

[Qemu-block] [PATCH for-2.10] throttle: Make LeakyBucket.avg and LeakyBucket.max integer types

[Alberto Garcia]

Both the throttling limits set with the throttling.iops-* and
throttling.bps-* options and their QMP equivalents defined in the
BlockIOThrottle struct are integer values.

Those limits are also reported in the BlockDeviceInfo struct and they
are integers there as well.

Therefore there's no reason to store them internally as double and do
the conversion everytime we're setting or querying them, so this patch
uses int64_t for those types.

LeakyBucket.level and LeakyBucket.burst_level do however remain double
because their value changes depending on the fraction of time elapsed
since the previous I/O operation.

There's one particular instance of the previous code where bkt->max
could have a non-integer value: that's in throttle_fix_bucket() when
bkt->max is initialized to bkt->avg / 10. This is now an integer
division and the result is rounded. We don't need to worry about this
because:

   a) with the magnitudes we're dealing with (bytes per second, I/O
  operations per second) the limits are likely to be always
  multiples of 10.

   b) even if they weren't this doesn't affect the actual limits, only
  the algorithm that makes the throttling smoother.

Signed-off-by: Alberto Garcia 
---
 include/qemu/throttle.h | 4 ++--
 util/throttle.c | 7 ++-
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
index d056008c18..ec37ac0fcb 100644
--- a/include/qemu/throttle.h
+++ b/include/qemu/throttle.h
@@ -77,8 +77,8 @@ typedef enum {
  */
 
 typedef struct LeakyBucket {
-double  avg;  /* average goal in units per second */
-double  max;  /* leaky bucket max burst in units */
+int64_t avg;  /* average goal in units per second */
+int64_t max;  /* leaky bucket max burst in units */
 double  level;/* bucket level in units */
 double  burst_level;  /* bucket level in units (for computing bursts) 
*/
 unsigned burst_length;/* max length of the burst period, in seconds */
diff --git a/util/throttle.c b/util/throttle.c
index b2a52b8b34..7856931f67 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -111,7 +111,7 @@ int64_t throttle_compute_wait(LeakyBucket *bkt)
 if (bkt->burst_length > 1) {
 /* We use 1/10 of the max value to smooth the throttling.
  * See throttle_fix_bucket() for more details. */
-extra = bkt->burst_level - bkt->max / 10;
+extra = bkt->burst_level - (double) bkt->max / 10;
 if (extra > 0) {
 return throttle_do_compute_wait(bkt->max, extra);
 }
@@ -361,8 +361,6 @@ bool throttle_is_valid(ThrottleConfig *cfg, Error **errp)
 /* fix bucket parameters */
 static void throttle_fix_bucket(LeakyBucket *bkt)
 {
-double min;
-
 /* zero bucket level */
 bkt->level = bkt->burst_level = 0;
 
@@ -374,9 +372,8 @@ static void throttle_fix_bucket(LeakyBucket *bkt)
  * Having a max burst value of 100ms of the average will help smooth the
  * throttling
  */
-min = bkt->avg / 10;
 if (bkt->avg && !bkt->max) {
-bkt->max = min;
+bkt->max = (bkt->avg + 5) / 10;
 }
 }
 
-- 
2.11.0

Re: [Qemu-block] [PATCH 0/3] respect bdrv_getlength() error code

[Kevin Wolf]

Am 04.08.2017 um 17:10 hat Denis V. Lunev geschrieben:
> These cases were reported by Markus Armbruster 
> Patches add error checking of the bdrv_getlength() call or remove
> the call of that function.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [PATCH 06/17] block/nbd-client: fix nbd_read_reply_entry

[Vladimir Sementsov-Ogievskiy]

07.08.2017 18:33, Eric Blake wrote:

On 08/07/2017 10:13 AM, Eric Blake wrote:

On 08/07/2017 07:56 AM, Vladimir Sementsov-Ogievskiy wrote:

07.08.2017 14:52, Eric Blake wrote:

On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:

Set reply.handle to 0 on error path to prevent normal path of
nbd_co_receive_reply.

...and the client is recognizing that the server sent garbage, but then
proceeds to handle the packet anyway.  The ideal reaction is that the
client should disconnect from the server, rather than handle the packet.
  But because it didn't do that, the client is now unable to receive any
future messages from the server.  Compare the traces of:

followed by a clean disconnect; vs. the buggy server:

29148@1502118384.385133:nbd_opt_go_success Export is good to go
29148@1502118384.385321:nbd_send_request Sending request to server: {
.from = 0, .len = 1, .handle = 94152262559840, .flags = 0x0, .type = 0
(read) }
29148@1502118396.494643:nbd_receive_reply Got reply: { magic =
0x1446698, .error =  0, handle = 94152262559840 }
invalid magic (got 0x1446698)
read 1/1 bytes at offset 0
1 bytes, 1 ops; 0:00:12.10 (0.082581 bytes/sec and 0.0826 ops/sec)
29148@1502118396.494746:nbd_send_request Sending request to server: {
.from = 0, .len = 0, .handle = 94152262559840, .flags = 0x0, .type = 3
(flush) }

where the client is now hung.  Thankfully, the client is blocked in an
idle loop (not eating CPU), so I don't know if this counts as the
ability for a malicious server to cause a denial of service against qemu
as an NBD client (in general, being unable to read further data from the
server because client internal state is now botched is not that much
different from being unable to read further data from the server because
the client hung up on the invalid server), unless there is some way to
cause qemu to die from an assertion failure rather than just get stuck.

With just patch 6/17 applied, the client still hangs, but this time with
a different trace:

30053@1502119637.604092:nbd_opt_go_success Export is good to go
30053@1502119637.604256:nbd_send_request Sending request to server: {
.from = 0, .len = 1, .handle = 94716063746144, .flags = 0x0, .type = 0
(read) }
30053@1502119649.070156:nbd_receive_reply Got reply: { magic =
0x1446698, .error =  0, handle = 94716063746144 }
invalid magic (got 0x1446698)
read failed: Input/output error
30053@1502119649.070243:nbd_send_request Sending request to server: {
.from = 0, .len = 0, .handle = 94716063746144, .flags = 0x0, .type = 3
(flush) }

The client still tried to send a flush request to the server, when it
should REALLY quit talking to the server at all and just disconnect,
because the moment the client recognizes that the server has sent
garbage is the moment that the client can no longer assume that the
server will react correctly to any further commands.

So I don't think your patch is quite correct, even if you have
identified a shortfall in our client code.


Why do you think so? It just does what it states in commit message.

Patch 17 should fix the case (but I doubt that it can be taken separately).


--
Best regards,
Vladimir

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10] iotests: fix 185

[Vladimir Sementsov-Ogievskiy]

07.08.2017 18:46, Vladimir Sementsov-Ogievskiy wrote:

07.08.2017 17:29, Eric Blake wrote:

On 08/07/2017 09:16 AM, Vladimir Sementsov-Ogievskiy wrote:

185 iotest is broken.

How to test:

i=0; while ./check -qcow2 -nocache 185; do ((i+=1)); echo N = $i; \

   done; echo N = $i

finished for me like this:

185 2s ... - output mismatch (see 185.out.bad)
--- /work/src/qemu/master/tests/qemu-iotests/185.out 2017-07-14 \
 15:14:29.520343805 +0300
+++ 185.out.bad 2017-08-07 16:51:02.231922900 +0300
@@ -37,7 +37,7 @@
  {"return": {}}
  {"return": {}}
  {"timestamp": {"seconds":  TIMESTAMP, "microseconds": TIMESTAMP}, \
  "event": "SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds": TIMESTAMP}, \
 "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", \
 "len": 4194304, "offset": 4194304, "speed": 65536, "type": \
 "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds": TIMESTAMP}, \
 "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", \
 "len": 0, "offset": 0, "speed": 65536, "type": "mirror"}}

This diff says both 'len' and 'offset' differ...


  === Start backup job and exit qemu ===

Failures: 185
Failed 1 of 1 tests
N = 8

It doesn't seems logical to expect the constant offset on cancel,
so let filter it out.

s/let/let's/


Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  tests/qemu-iotests/185 | 2 +-
  tests/qemu-iotests/185.out | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds": TIMESTAMP}, 
"event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 
4194304, "offset": 4194304, "speed": 65536, "type": "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds": TIMESTAMP}, 
"event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 
4194304, "offset": OFFSET, "speed": 65536, "type": "mirror"}}

...while this diff only touched 'offset'.  Did you copy-and-paste
incorrectly in the commit message?  If so, then with the commit message
fixed, I'm okay with:
Reviewed-by: Eric Blake 

Hmm, I can't reproduce with "len = 0", and it looks impossible. But 
I've scrolled up in one of my terminals and found this reproduce, 
really, '"len": 0', so, that is possible.


And, after looking through the code it looks like it really possible - 
s->common.len is set not in the very beginning, but after for ex. call 
to block_job_pause_point().


So, the reproduction that I've copied into commit message was very 
good match.


But, I'm not sure, that len should be fixed in the same way. May be it 
would be better to set earlier and don't update on each iteration (why 
is it updated??)





So, IMHO, this patch is good (with s/"len": 0/"len": 4194304/ and 
s/let/let's/ in commit message)



--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH] iotests: fix 185

[Kevin Wolf]

Am 07.08.2017 um 16:16 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 185 iotest is broken.
> 
> How to test:
> > i=0; while ./check -qcow2 -nocache 185; do ((i+=1)); echo N = $i; \
>   done; echo N = $i
> 
> finished for me like this:
> 
> 185 2s ... - output mismatch (see 185.out.bad)
> --- /work/src/qemu/master/tests/qemu-iotests/185.out2017-07-14 \
> 15:14:29.520343805 +0300
> +++ 185.out.bad 2017-08-07 16:51:02.231922900 +0300
> @@ -37,7 +37,7 @@
>  {"return": {}}
>  {"return": {}}
>  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, \
>  "event": "SHUTDOWN", "data": {"guest": false}}
> -{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, \
> "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", \
> "len": 4194304, "offset": 4194304, "speed": 65536, "type": \
> "mirror"}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, \
> "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", \
> "len": 0, "offset": 0, "speed": 65536, "type": "mirror"}}
> 
>  === Start backup job and exit qemu ===
> 
> Failures: 185
> Failed 1 of 1 tests
> N = 8
> 
> It doesn't seems logical to expect the constant offset on cancel,
> so let filter it out.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

I'm quoting 185:

# Note that the reference output intentionally includes the 'offset' field in
# BLOCK_JOB_CANCELLED events for all of the following block jobs. They are
# predictable and any change in the offsets would hint at a bug in the job
# throttling code.
#
# In order to achieve these predictable offsets, all of the following tests
# use speed=65536. Each job will perform exactly one iteration before it has
# to sleep at least for a second, which is plenty of time for the 'quit' QMP
# command to be received (after receiving the command, the rest runs
# synchronously, so jobs can arbitrarily continue or complete).
#
# The buffer size for commit and streaming is 512k (waiting for 8 seconds after
# the first request), for active commit and mirror it's large enough to cover
# the full 4M, and for backup it's the qcow2 cluster size, which we know is
# 64k. As all of these are at least as large as the speed, we are sure that the
# offset doesn't advance after the first iteration before qemu exits.

So before we change the expected output, can we explain why the offsets
aren't predictable, even if throttling is used and contrary to what the
comment says? Should we sleep a little before issuing 'quit'?

(By the way, I couldn't reproduce in N = 128 attempts, so it doesn't
look like I can look into what's happening in detail, except with code
review.)

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10] iotests: fix 185

[Vladimir Sementsov-Ogievskiy]

07.08.2017 17:29, Eric Blake wrote:

On 08/07/2017 09:16 AM, Vladimir Sementsov-Ogievskiy wrote:

185 iotest is broken.

How to test:

i=0; while ./check -qcow2 -nocache 185; do ((i+=1)); echo N = $i; \

   done; echo N = $i

finished for me like this:

185 2s ... - output mismatch (see 185.out.bad)
--- /work/src/qemu/master/tests/qemu-iotests/185.out2017-07-14 \
 15:14:29.520343805 +0300
+++ 185.out.bad 2017-08-07 16:51:02.231922900 +0300
@@ -37,7 +37,7 @@
  {"return": {}}
  {"return": {}}
  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, \
  "event": "SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, \
 "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", \
 "len": 4194304, "offset": 4194304, "speed": 65536, "type": \
 "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, \
 "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", \
 "len": 0, "offset": 0, "speed": 65536, "type": "mirror"}}

This diff says both 'len' and 'offset' differ...


  === Start backup job and exit qemu ===

Failures: 185
Failed 1 of 1 tests
N = 8

It doesn't seems logical to expect the constant offset on cancel,
so let filter it out.

s/let/let's/


Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  tests/qemu-iotests/185 | 2 +-
  tests/qemu-iotests/185.out | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", 
"len": 4194304, "offset": 4194304, "speed": 65536, "type": "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", 
"len": 4194304, "offset": OFFSET, "speed": 65536, "type": "mirror"}}

...while this diff only touched 'offset'.  Did you copy-and-paste
incorrectly in the commit message?  If so, then with the commit message
fixed, I'm okay with:
Reviewed-by: Eric Blake 

Hmm, I can't reproduce with "len = 0", and it looks impossible. But I've 
scrolled up in one of my terminals and found this reproduce, really, 
'"len": 0', so, that is possible.


And, after looking through the code it looks like it really possible - 
s->common.len is set not in the very beginning, but after for ex. call 
to block_job_pause_point().


So, the reproduction that I've copied into commit message was very good 
match.


But, I'm not sure, that len should be fixed in the same way. May be it 
would be better to set earlier and don't update on each iteration (why 
is it updated??)



--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH for-2.10] vmdk: Fix error handling/reporting of vmdk_check

[Kevin Wolf]

Am 04.08.2017 um 16:09 hat Fam Zheng geschrieben:
> Errors from the callees must be captured and propagated to our caller,
> ensure this for both find_extent() and bdrv_getlength().
> 
> Reported-by: Markus Armbruster 
> Signed-off-by: Fam Zheng 

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [PATCH 06/17] block/nbd-client: fix nbd_read_reply_entry

[Eric Blake]

On 08/07/2017 10:13 AM, Eric Blake wrote:
> On 08/07/2017 07:56 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 07.08.2017 14:52, Eric Blake wrote:
>>> On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
 Set reply.handle to 0 on error path to prevent normal path of
 nbd_co_receive_reply.
> 

> ...and the client is recognizing that the server sent garbage, but then
> proceeds to handle the packet anyway.  The ideal reaction is that the
> client should disconnect from the server, rather than handle the packet.
>  But because it didn't do that, the client is now unable to receive any
> future messages from the server.  Compare the traces of:
> 

> followed by a clean disconnect; vs. the buggy server:
> 
> 29148@1502118384.385133:nbd_opt_go_success Export is good to go
> 29148@1502118384.385321:nbd_send_request Sending request to server: {
> .from = 0, .len = 1, .handle = 94152262559840, .flags = 0x0, .type = 0
> (read) }
> 29148@1502118396.494643:nbd_receive_reply Got reply: { magic =
> 0x1446698, .error =  0, handle = 94152262559840 }
> invalid magic (got 0x1446698)
> read 1/1 bytes at offset 0
> 1 bytes, 1 ops; 0:00:12.10 (0.082581 bytes/sec and 0.0826 ops/sec)
> 29148@1502118396.494746:nbd_send_request Sending request to server: {
> .from = 0, .len = 0, .handle = 94152262559840, .flags = 0x0, .type = 3
> (flush) }
> 
> where the client is now hung.  Thankfully, the client is blocked in an
> idle loop (not eating CPU), so I don't know if this counts as the
> ability for a malicious server to cause a denial of service against qemu
> as an NBD client (in general, being unable to read further data from the
> server because client internal state is now botched is not that much
> different from being unable to read further data from the server because
> the client hung up on the invalid server), unless there is some way to
> cause qemu to die from an assertion failure rather than just get stuck.

With just patch 6/17 applied, the client still hangs, but this time with
a different trace:

30053@1502119637.604092:nbd_opt_go_success Export is good to go
30053@1502119637.604256:nbd_send_request Sending request to server: {
.from = 0, .len = 1, .handle = 94716063746144, .flags = 0x0, .type = 0
(read) }
30053@1502119649.070156:nbd_receive_reply Got reply: { magic =
0x1446698, .error =  0, handle = 94716063746144 }
invalid magic (got 0x1446698)
read failed: Input/output error
30053@1502119649.070243:nbd_send_request Sending request to server: {
.from = 0, .len = 0, .handle = 94716063746144, .flags = 0x0, .type = 3
(flush) }

The client still tried to send a flush request to the server, when it
should REALLY quit talking to the server at all and just disconnect,
because the moment the client recognizes that the server has sent
garbage is the moment that the client can no longer assume that the
server will react correctly to any further commands.

So I don't think your patch is quite correct, even if you have
identified a shortfall in our client code.

-- 
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-block] [PATCH 06/17] block/nbd-client: fix nbd_read_reply_entry

[Eric Blake]

On 08/07/2017 07:56 AM, Vladimir Sementsov-Ogievskiy wrote:
> 07.08.2017 14:52, Eric Blake wrote:
>> On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Set reply.handle to 0 on error path to prevent normal path of
>>> nbd_co_receive_reply.

Side note: in general, our server must allow a handle of 0 as valid (as
the handle is something chosen by the client); but our particular client
always uses non-zero handles (and therefore using 0 as a sentinel for an
error path is okay).

>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>   block/nbd-client.c | 1 +
>>>   1 file changed, 1 insertion(+)
>> Can you document a case where not fixing this would be an observable bug
>> (even if it requires using gdb and single-stepping between client and
>> server to make what is otherwise a racy situation easy to see)?  I'm
>> trying to figure out if this is 2.10 material.
> 
> it is simple enough:
> 
> run qemu-nbd in gdb, set break on nbd_send_reply, and when it shoot s,
> next up to "stl_be_p(buf, NBD_REPLY_MAGIC);" and after it do "call
> stl_be_p(buf, 1000)"

Okay, so you have set up a malicious server intentionally sending bad
magic...

> 
> run qemu-io with some read in gdb, set break on
> br block/nbd-client.c:83
> 
> ( it is break; after failed nbd_receive_reply call)
> 
> and on
> br block/nbd-client.c:170
> 
> (it is in nbd_co_receive_reply after yield)
> 
> on first break we will be sure that  nbd_receive_reply failed,
> on second we will be sure by
> (gdb) p s->reply
> $1 = {handle = 93825000680144, error = 0}
> (gdb) p request->handle
> $2 = 93825000680144
> 
> that we are on normal receiving path.

...and the client is recognizing that the server sent garbage, but then
proceeds to handle the packet anyway.  The ideal reaction is that the
client should disconnect from the server, rather than handle the packet.
 But because it didn't do that, the client is now unable to receive any
future messages from the server.  Compare the traces of:

$  ./qemu-io -c 'r 0 1' -f raw nbd://localhost:10809/foo --trace nbd_\*

against a working server:

29103@1502118291.281180:nbd_opt_go_success Export is good to go
29103@1502118291.281397:nbd_send_request Sending request to server: {
.from = 0, .len = 1, .handle = 93860726319200, .flags = 0x0, .type = 0
(read) }
29103@1502118291.281705:nbd_receive_reply Got reply: { magic =
0x67446698, .error =  0, handle = 93860726319200 }
read 1/1 bytes at offset 0
1 bytes, 1 ops; 0.0003 sec (2.822 KiB/sec and 2890.1734 ops/sec)
29103@1502118291.281773:nbd_send_request Sending request to server: {
.from = 0, .len = 0, .handle = 93860726319200, .flags = 0x0, .type = 3
(flush) }
29103@1502118291.281902:nbd_receive_reply Got reply: { magic =
0x67446698, .error =  0, handle = 93860726319200 }
29103@1502118291.281939:nbd_send_request Sending request to server: {
.from = 0, .len = 0, .handle = 93860726319200, .flags = 0x0, .type = 3
(flush) }
29103@1502118291.282064:nbd_receive_reply Got reply: { magic =
0x67446698, .error =  0, handle = 93860726319200 }
29103@1502118291.282078:nbd_send_request Sending request to server: {
.from = 0, .len = 0, .handle = 0, .flags = 0x0, .type = 2 (discard) }

followed by a clean disconnect; vs. the buggy server:

29148@1502118384.385133:nbd_opt_go_success Export is good to go
29148@1502118384.385321:nbd_send_request Sending request to server: {
.from = 0, .len = 1, .handle = 94152262559840, .flags = 0x0, .type = 0
(read) }
29148@1502118396.494643:nbd_receive_reply Got reply: { magic =
0x1446698, .error =  0, handle = 94152262559840 }
invalid magic (got 0x1446698)
read 1/1 bytes at offset 0
1 bytes, 1 ops; 0:00:12.10 (0.082581 bytes/sec and 0.0826 ops/sec)
29148@1502118396.494746:nbd_send_request Sending request to server: {
.from = 0, .len = 0, .handle = 94152262559840, .flags = 0x0, .type = 3
(flush) }

where the client is now hung.  Thankfully, the client is blocked in an
idle loop (not eating CPU), so I don't know if this counts as the
ability for a malicious server to cause a denial of service against qemu
as an NBD client (in general, being unable to read further data from the
server because client internal state is now botched is not that much
different from being unable to read further data from the server because
the client hung up on the invalid server), unless there is some way to
cause qemu to die from an assertion failure rather than just get stuck.

It also looks like the client acts on at most one bad packet from the
server before it gets stuck - but the question is whether a malicious
server could, in that one bad packet reply, cause qemu to misbehave in
some other way.

I'm adding Prasad, to analyze whether this needs a CVE.

We don't have a good protocol fuzzer to simulate a bad client and/or bad
server as the partner over the socket - if you can find any more such
interactions where a bad partner can cause a hang or crash, let's get
those fixed in 2.10.

Meanwhile, I'll probably include this 

Re: [Qemu-block] [RFC PATCH 56/56] crypto: Make QCryptoBlockInfoLUKS offsets unsigned in QAPI/QMP

[Daniel P. Berrange]

On Mon, Aug 07, 2017 at 04:46:00PM +0200, Markus Armbruster wrote:
> Byte offsets should use QAPI type 'size' (uint64_t).
> QCryptoBlockInfoLUKS member @payload-offset and
> QCryptoBlockInfoLUKSSlot member @key-offset are 'int' (int64_t).
> qcrypto_block_luks_get_info() gets the former QCryptoBlock member
> @payload_offset, implicitly converting from uint64_t, and computes the
> latter from QCryptoBlockLUKSKeySlot member @key_offset, implicitly
> converting from long long.
> 
> Change both offsets to 'size'.
> 
> query-block and query-named-block-nodes now report @payload-offset
> values above 2^63-1 correctly instead of their (negative) two's
> complement.  Should never occur in practice.
> 
> Signed-off-by: Markus Armbruster 

Reviewed-by: Daniel P. Berrange 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

[Qemu-block] [RFC PATCH 53/56] block: Make blockdev-add byte counts unsigned in QAPI/QMP

[Markus Armbruster]

File offsets, sizes and alignments should use QAPI type 'size'
(uint64_t).  blockdev-add parameters are 'int' (int64_t):
BlockdevOptionsNull member @size; BlockdevOptionsQcow2 members
@cache-size, @l2-cache-size, @refcount-cache-size; BlockdevOptionsNfs
members @readahead-size, @page-cache-size; BlockdevOptionsCurlBase
member @readahead; BlockdevOptionsRaw members @offset, @size.

The block drivers get their values with qemu_opt_get_size(), which
returns uint64_t.  They store them in uint64_t variables, except for
the null driver, which stores in BDRVNullState member int64_t length.

Change these BlockdevOptionsFOO members to 'size'.

Change BDRVNullState member length to uint64_t.  This moves the
implicit conversion to int64_t from null_file_open() to
null_getlength().  No worse than before.

blockdev-add and -blockdev now accept values between 2^63 and 2^64-1.
They accept negative values as before, because that's how the QObject
input visitor works for backward compatibility.

Values above 2^63 are unlikely to actually work; the block subsystem
isn't prepared for them.  Again, no worse than before.

Aside: we call the same thing @readahead-size in one place, and
@readahead in another place.  Sad.

Signed-off-by: Markus Armbruster 
---
 block/null.c |  2 +-
 qapi/block-core.json | 22 +++---
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/block/null.c b/block/null.c
index 876f909..2ab0e70 100644
--- a/block/null.c
+++ b/block/null.c
@@ -20,7 +20,7 @@
 #define NULL_OPT_ZEROES  "read-zeroes"
 
 typedef struct {
-int64_t length;
+uint64_t length;
 int64_t latency_ns;
 bool read_zeroes;
 } BDRVNullState;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 64b84a5..3482f8c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2189,7 +2189,7 @@
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsNull',
-  'data': { '*size': 'int', '*latency-ns': 'uint64' } }
+  'data': { '*size': 'size', '*latency-ns': 'uint64' } }
 
 ##
 # @BlockdevOptionsVVFAT:
@@ -2421,9 +2421,9 @@
 '*pass-discard-snapshot': 'bool',
 '*pass-discard-other': 'bool',
 '*overlap-check': 'Qcow2OverlapChecks',
-'*cache-size': 'int',
-'*l2-cache-size': 'int',
-'*refcount-cache-size': 'int',
+'*cache-size': 'size',
+'*l2-cache-size': 'size',
+'*refcount-cache-size': 'size',
 '*cache-clean-interval': 'int',
 '*encrypt': 'BlockdevQcow2Encryption' } }
 
@@ -2568,9 +2568,9 @@
 { 'struct': 'BlockdevOptionsBlkdebug',
   'data': { 'image': 'BlockdevRef',
 '*config': 'str',
-'*align': 'int', '*max-transfer': 'int32',
-'*opt-write-zero': 'int32', '*max-write-zero': 'int32',
-'*opt-discard': 'int32', '*max-discard': 'int32',
+'*align': 'size', '*max-transfer': 'size',
+'*opt-write-zero': 'size', '*max-write-zero': 'size',
+'*opt-discard': 'size', '*max-discard': 'size',
 '*inject-error': ['BlkdebugInjectErrorOptions'],
 '*set-state': ['BlkdebugSetStateOptions'] } }
 
@@ -2862,8 +2862,8 @@
 '*user': 'int',
 '*group': 'int',
 '*tcp-syn-count': 'int',
-'*readahead-size': 'int',
-'*page-cache-size': 'int',
+'*readahead-size': 'size',
+'*page-cache-size': 'size',
 '*debug': 'int' } }
 
 ##
@@ -2893,7 +2893,7 @@
 ##
 { 'struct': 'BlockdevOptionsCurlBase',
   'data': { 'url': 'str',
-'*readahead': 'int',
+'*readahead': 'size',
 '*timeout': 'int',
 '*username': 'str',
 '*password-secret': 'str',
@@ -3001,7 +3001,7 @@
 ##
 { 'struct': 'BlockdevOptionsRaw',
   'base': 'BlockdevOptionsGenericFormat',
-  'data': { '*offset': 'int', '*size': 'int' } }
+  'data': { '*offset': 'size', '*size': 'size' } }
 
 ##
 # @BlockdevOptionsVxHS:
-- 
2.7.5

[Qemu-block] [RFC PATCH 51/56] block/nfs: Fix for readahead-size, page-cache-size > INT64_MAX

[Markus Armbruster]

nfs_client_open() implicitly converts the uint64_t value of
qemu_opt_get_number() to int64_t, then clamps it to range.  The
clamping is broken for negative values.

Fix by making NFSClient members @readahead and @pagecache uint64_t.

Signed-off-by: Markus Armbruster 
---
 block/nfs.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/block/nfs.c b/block/nfs.c
index d8db419..2776788 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -58,7 +58,8 @@ typedef struct NFSClient {
 bool cache_used;
 NFSServer *server;
 char *path;
-int64_t uid, gid, tcp_syncnt, readahead, pagecache, debug;
+int64_t uid, gid, tcp_syncnt, debug;
+uint64_t readahead, pagecache;
 } NFSClient;
 
 typedef struct NFSRPC {
@@ -856,10 +857,10 @@ static void nfs_refresh_filename(BlockDriverState *bs, 
QDict *options)
 qdict_put_int(opts, "tcp-syn-cnt", client->tcp_syncnt);
 }
 if (client->readahead) {
-qdict_put_int(opts, "readahead-size", client->readahead);
+qdict_put_uint(opts, "readahead-size", client->readahead);
 }
 if (client->pagecache) {
-qdict_put_int(opts, "page-cache-size", client->pagecache);
+qdict_put_uint(opts, "page-cache-size", client->pagecache);
 }
 if (client->debug) {
 qdict_put_int(opts, "debug", client->debug);
-- 
2.7.5

[Qemu-block] [RFC PATCH 55/56] block: Make MapEntry offsets and size unsigned in QAPI

[Markus Armbruster]

File offsets and sizes use QAPI type 'size' (uint64_t).  MapEntry
members @start, @length and @offset are 'int' (int64_t).
get_block_status() sets @start and @length to unsigned long long
values, and @offset to a non-negative int64_t value.

Change these MapEntry members to 'size'.

"qemu-img map" now reports them correctly above 2^63-1 instead of
their (negative) two's complement.

Signed-off-by: Markus Armbruster 
---
 qapi/block-core.json | 4 ++--
 qemu-img.c   | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 3482f8c..6f62723 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -236,8 +236,8 @@
 #
 ##
 { 'struct': 'MapEntry',
-  'data': {'start': 'int', 'length': 'int', 'data': 'bool',
-   'zero': 'bool', 'depth': 'int', '*offset': 'int',
+  'data': {'start': 'size', 'length': 'size', 'data': 'bool',
+   'zero': 'bool', 'depth': 'int', '*offset': 'size',
'*filename': 'str' } }
 
 ##
diff --git a/qemu-img.c b/qemu-img.c
index cf3ef3e..1c783c7 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2655,14 +2655,14 @@ static void dump_map_entry(OutputFormat output_format, 
MapEntry *e,
 }
 break;
 case OFORMAT_JSON:
-printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","
+printf("%s{ \"start\": %" PRIu64 ", \"length\": %" PRIu64 ","
" \"depth\": %"PRId64", \"zero\": %s, \"data\": %s",
(e->start == 0 ? "[" : ",\n"),
e->start, e->length, e->depth,
e->zero ? "true" : "false",
e->data ? "true" : "false");
 if (e->has_offset) {
-printf(", \"offset\": %"PRId64"", e->offset);
+printf(", \"offset\": %" PRIu64, e->offset);
 }
 putchar('}');
 
-- 
2.7.5

[Qemu-block] [RFC PATCH 42/56] blockjob: Lift speed sign conversion out of stream_start()

[Markus Armbruster]

stream_start() takes int64_t speed.  The underlying BlockJob
abstraction takes uint64_t.  stream_start() converts from int64_t to
uint64_t, rejecting negative speed.

Lift this check and conversion out of stream_start() into its caller.
I'm going to lift it further until it falls off the top.

Signed-off-by: Markus Armbruster 
---
 block/stream.c| 8 +---
 blockdev.c| 6 ++
 include/block/block_int.h | 2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index fefcdb9..16acb1e 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -223,7 +223,7 @@ static const BlockJobDriver stream_job_driver = {
 
 void stream_start(const char *job_id, BlockDriverState *bs,
   BlockDriverState *base, const char *backing_file_str,
-  int64_t speed, BlockdevOnError on_error, Error **errp)
+  uint64_t speed, BlockdevOnError on_error, Error **errp)
 {
 StreamBlockJob *s;
 BlockDriverState *iter;
@@ -237,12 +237,6 @@ void stream_start(const char *job_id, BlockDriverState *bs,
 }
 }
 
-if (speed < 0) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
-   "a non-negative rate limit");
-return;
-}
-
 /* Prevent concurrent jobs trying to modify the graph structure here, we
  * already have our own plans. Also don't allow resize as the image size is
  * queried only at the job start and then cached. */
diff --git a/blockdev.c b/blockdev.c
index 1deea49..4d9a665 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2989,6 +2989,12 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
 return;
 }
 
+if (speed < 0) {
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
+   "a non-negative rate limit");
+return;
+}
+
 aio_context = bdrv_get_aio_context(bs);
 aio_context_acquire(aio_context);
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 19639c0..695d7b3 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -821,7 +821,7 @@ int is_windows_drive(const char *filename);
  */
 void stream_start(const char *job_id, BlockDriverState *bs,
   BlockDriverState *base, const char *backing_file_str,
-  int64_t speed, BlockdevOnError on_error, Error **errp);
+  uint64_t speed, BlockdevOnError on_error, Error **errp);
 
 /**
  * commit_start:
-- 
2.7.5

[Qemu-block] [RFC PATCH 56/56] crypto: Make QCryptoBlockInfoLUKS offsets unsigned in QAPI/QMP

[Markus Armbruster]

Byte offsets should use QAPI type 'size' (uint64_t).
QCryptoBlockInfoLUKS member @payload-offset and
QCryptoBlockInfoLUKSSlot member @key-offset are 'int' (int64_t).
qcrypto_block_luks_get_info() gets the former QCryptoBlock member
@payload_offset, implicitly converting from uint64_t, and computes the
latter from QCryptoBlockLUKSKeySlot member @key_offset, implicitly
converting from long long.

Change both offsets to 'size'.

query-block and query-named-block-nodes now report @payload-offset
values above 2^63-1 correctly instead of their (negative) two's
complement.  Should never occur in practice.

Signed-off-by: Markus Armbruster 
---
 qapi/crypto.json | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qapi/crypto.json b/qapi/crypto.json
index 6b6fde3..57a10cb 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -266,7 +266,7 @@
   'data': {'active': 'bool',
'*iters': 'int',
'*stripes': 'int',
-   'key-offset': 'int' } }
+   'key-offset': 'size' } }
 
 
 ##
@@ -292,7 +292,7 @@
'ivgen-alg': 'QCryptoIVGenAlgorithm',
'*ivgen-hash-alg': 'QCryptoHashAlgorithm',
'hash-alg': 'QCryptoHashAlgorithm',
-   'payload-offset': 'int',
+   'payload-offset': 'size',
'master-key-iters': 'int',
'uuid': 'str',
'slots': [ 'QCryptoBlockInfoLUKSSlot' ] }}
-- 
2.7.5

[Qemu-block] [RFC PATCH 52/56] block/nfs: Reject negative readahead-size, page-cache-size

[Markus Armbruster]

The nfs block driver uses QEMU_OPT_NUMBER for these sizes.  All other
block drivers use QEMU_OPT_SIZE.  Both are uint64_t, but QEMU_OPT_SIZE
rejects negative numbers, while QEMU_OPT_NUMBER interprets them modulo
2^64.  Switch the nfs block driver to QEMU_OPT_SIZE.

Signed-off-by: Markus Armbruster 
---
 block/nfs.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/nfs.c b/block/nfs.c
index 2776788..0ed3e7c 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -394,12 +394,12 @@ static QemuOptsList runtime_opts = {
 },
 {
 .name = "readahead-size",
-.type = QEMU_OPT_NUMBER,
+.type = QEMU_OPT_SIZE,
 .help = "Set the readahead size in bytes",
 },
 {
 .name = "page-cache-size",
-.type = QEMU_OPT_NUMBER,
+.type = QEMU_OPT_SIZE,
 .help = "Set the pagecache size in bytes",
 },
 {
@@ -557,7 +557,7 @@ static int64_t nfs_client_open(NFSClient *client, QDict 
*options,
  "if cache.direct = on");
 goto fail;
 }
-client->readahead = qemu_opt_get_number(opts, "readahead-size", 0);
+client->readahead = qemu_opt_get_size(opts, "readahead-size", 0);
 if (client->readahead > QEMU_NFS_MAX_READAHEAD_SIZE) {
 warn_report("Truncating NFS readahead size to %d",
 QEMU_NFS_MAX_READAHEAD_SIZE);
@@ -578,7 +578,7 @@ static int64_t nfs_client_open(NFSClient *client, QDict 
*options,
  "if cache.direct = on");
 goto fail;
 }
-client->pagecache = qemu_opt_get_number(opts, "page-cache-size", 0);
+client->pagecache = qemu_opt_get_size(opts, "page-cache-size", 0);
 if (client->pagecache > QEMU_NFS_MAX_PAGECACHE_SIZE) {
 warn_report("Truncating NFS pagecache size to %d pages",
 QEMU_NFS_MAX_PAGECACHE_SIZE);
-- 
2.7.5

[Qemu-block] [RFC PATCH 49/56] block: Make ImageCheck file offset unsigned in QAPI

[Markus Armbruster]

File offsets should use QAPI type 'size' (uint64_t).  ImageCheck
member @image-end-offset is 'int' (int64_t).  collect_image_check()
gets it from BdrvCheckResult member @image_end_offset (also int64_t,
should never be negative).

Change the ImageCheck member to 'size', for QAPI/QMP consistency.

Signed-off-by: Markus Armbruster 
---
 qapi/block-core.json | 2 +-
 qemu-img.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 51caee9..3c6d448 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -208,7 +208,7 @@
 ##
 { 'struct': 'ImageCheck',
   'data': {'filename': 'str', 'format': 'str', 'check-errors': 'int',
-   '*image-end-offset': 'int', '*corruptions': 'int', '*leaks': 'int',
+   '*image-end-offset': 'size', '*corruptions': 'int', '*leaks': 'int',
'*corruptions-fixed': 'int', '*leaks-fixed': 'int',
'*total-clusters': 'int', '*allocated-clusters': 'int',
'*fragmented-clusters': 'int', '*compressed-clusters': 'int' } }
diff --git a/qemu-img.c b/qemu-img.c
index f4d5f0d..3ae5fe3 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -604,7 +604,7 @@ static void dump_human_image_check(ImageCheck *check, bool 
quiet)
 
 if (check->image_end_offset) {
 qprintf(quiet,
-"Image end offset: %" PRId64 "\n", check->image_end_offset);
+"Image end offset: %" PRIu64 "\n", check->image_end_offset);
 }
 }
 
-- 
2.7.5

[Qemu-block] [RFC PATCH 41/56] blockjob: Lift speed sign conversion out of mirror_start_job()

[Markus Armbruster]

mirror_start_job() takes int64_t speed.  The underlying BlockJob
abstraction takes uint64_t.  mirror_start_job() converts from int64_t
to uint64_t, rejecting negative speed.

Lift this check and conversion out of mirror_start_job() into its
callers.  I'm going to lift it further until it falls off the top.

Signed-off-by: Markus Armbruster 
---
 block/mirror.c | 19 ---
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index af54163..27b4c7f 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1119,7 +1119,7 @@ static BlockDriver bdrv_mirror_top = {
 
 static void mirror_start_job(const char *job_id, BlockDriverState *bs,
  int creation_flags, BlockDriverState *target,
- const char *replaces, int64_t speed,
+ const char *replaces, uint64_t speed,
  uint64_t granularity, int64_t buf_size,
  BlockMirrorBackingMode backing_mode,
  BlockdevOnError on_source_error,
@@ -1139,12 +1139,6 @@ static void mirror_start_job(const char *job_id, 
BlockDriverState *bs,
 Error *local_err = NULL;
 int ret;
 
-if (speed < 0) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
-   "a non-negative rate limit");
-return;
-}
-
 if (granularity == 0) {
 granularity = bdrv_get_default_bitmap_granularity(target);
 }
@@ -1298,6 +1292,11 @@ void mirror_start(const char *job_id, BlockDriverState 
*bs,
 bool is_none_mode;
 BlockDriverState *base;
 
+if (speed < 0) {
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
+   "a non-negative rate limit");
+return;
+}
 if (mode == MIRROR_SYNC_MODE_INCREMENTAL) {
 error_setg(errp, "Sync mode 'incremental' not supported");
 return;
@@ -1323,6 +1322,12 @@ void commit_active_start(const char *job_id, 
BlockDriverState *bs,
 
 orig_base_flags = bdrv_get_flags(base);
 
+if (speed < 0) {
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
+   "a non-negative rate limit");
+return;
+}
+
 if (bdrv_reopen(base, bs->open_flags, errp)) {
 return;
 }
-- 
2.7.5

[Qemu-block] [RFC PATCH 46/56] blockjob: Make job commands' speed parameter unsigned in QAPI/QMP

[Markus Armbruster]

Byte rates should use QAPI type 'size' (uint64_t).  drive-backup,
blockdev-backup, block-commit, drive-mirror, blockdev-mirror,
block-stream and block-job-set-speed parameter @size is 'int'
(int64_t).  Their QMP command handlers all ensure it's non-negative
before they pass it on to the next lower layer, which expects
uint64_t.

Change these @speed parameters to 'size', and drop the range checks.
You can now set rate limits between 2^63 and 2^64-1.  Good luck
finding hardware where that actually limits.

Negative values are accepted and interpreted modulo 2^64, because
that's how the QObject input visitor works for backward compatibility.
Drop the tests for negative size.

Signed-off-by: Markus Armbruster 
---
 blockdev.c | 50 --
 qapi/block-core.json   | 14 ++---
 tests/qemu-iotests/030 | 16 ---
 tests/qemu-iotests/030.out |  4 ++--
 tests/qemu-iotests/041 | 18 -
 tests/qemu-iotests/041.out |  4 ++--
 tests/qemu-iotests/055 | 26 
 tests/qemu-iotests/055.out |  4 ++--
 8 files changed, 17 insertions(+), 119 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index e679f5d..7feed7a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2970,7 +2970,7 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
   bool has_base, const char *base,
   bool has_base_node, const char *base_node,
   bool has_backing_file, const char *backing_file,
-  bool has_speed, int64_t speed,
+  bool has_speed, uint64_t speed,
   bool has_on_error, BlockdevOnError on_error,
   Error **errp)
 {
@@ -2989,12 +2989,6 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
 return;
 }
 
-if (speed < 0) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
-   "a non-negative rate limit");
-return;
-}
-
 aio_context = bdrv_get_aio_context(bs);
 aio_context_acquire(aio_context);
 
@@ -3063,7 +3057,7 @@ void qmp_block_commit(bool has_job_id, const char 
*job_id, const char *device,
   bool has_base, const char *base,
   bool has_top, const char *top,
   bool has_backing_file, const char *backing_file,
-  bool has_speed, int64_t speed,
+  bool has_speed, uint64_t speed,
   bool has_filter_node_name, const char *filter_node_name,
   Error **errp)
 {
@@ -3102,12 +3096,6 @@ void qmp_block_commit(bool has_job_id, const char 
*job_id, const char *device,
 return;
 }
 
-if (speed < 0) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
-   "a non-negative rate limit");
-goto out;
-}
-
 aio_context = bdrv_get_aio_context(bs);
 aio_context_acquire(aio_context);
 
@@ -3222,12 +3210,6 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
BlockJobTxn *txn,
 return NULL;
 }
 
-if (backup->speed < 0) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
-   "a non-negative rate limit");
-return NULL;
-}
-
 aio_context = bdrv_get_aio_context(bs);
 aio_context_acquire(aio_context);
 
@@ -3371,12 +3353,6 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, 
BlockJobTxn *txn,
 return NULL;
 }
 
-if (backup->speed < 0) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
-   "a non-negative rate limit");
-return NULL;
-}
-
 aio_context = bdrv_get_aio_context(bs);
 aio_context_acquire(aio_context);
 
@@ -3509,12 +3485,6 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
 return;
 }
 
-if (arg->speed < 0) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
-   "a non-negative rate limit");
-return;
-}
-
 aio_context = bdrv_get_aio_context(bs);
 aio_context_acquire(aio_context);
 
@@ -3647,7 +3617,7 @@ void qmp_blockdev_mirror(bool has_job_id, const char 
*job_id,
  const char *device, const char *target,
  bool has_replaces, const char *replaces,
  MirrorSyncMode sync,
- bool has_speed, int64_t speed,
+ bool has_speed, uint64_t speed,
  bool has_granularity, uint64_t granularity,
  bool has_buf_size, int64_t buf_size,
  bool has_on_source_error,
@@ -3674,12 +3644,6 @@ void qmp_blockdev_mirror(bool has_job_id, const char 
*job_id,
 return;
 }
 
-if (speed < 0) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
-   "a 

[Qemu-block] [RFC PATCH 47/56] blockjob: Make BlockJobInfo and event offsets unsigned in QAPI/QMP

[Markus Armbruster]

File offsets should use QAPI type 'size' (uint64_t).  BlockJobInfo
members @len, offset and parameters @len, @offset of events
BLOCK_JOB_COMPLETED, BLOCK_JOB_CANCELLED, BLOCK_JOB_READY are 'int'
(int64_t).  block_job_query(), block_job_event_completed(),
block_job_event_cancelled(), block_job_event_ready() get them from
BlockJob members @len and @offset, which are int64_t, but should never
be negative.

Change the BlockJobInfo members and the event parameters to 'size',
for QAPI/QMP consistency.

Signed-off-by: Markus Armbruster 
---
 hmp.c|  8 
 qapi/block-core.json | 16 
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/hmp.c b/hmp.c
index fae974e..867c50b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -966,16 +966,16 @@ void hmp_info_block_jobs(Monitor *mon, const QDict *qdict)
 
 while (list) {
 if (strcmp(list->value->type, "stream") == 0) {
-monitor_printf(mon, "Streaming device %s: Completed %" PRId64
-   " of %" PRId64 " bytes, speed limit %" PRIu64
+monitor_printf(mon, "Streaming device %s: Completed %" PRIu64
+   " of %" PRIu64 " bytes, speed limit %" PRIu64
" bytes/s\n",
list->value->device,
list->value->offset,
list->value->len,
list->value->speed);
 } else {
-monitor_printf(mon, "Type %s, device %s: Completed %" PRId64
-   " of %" PRId64 " bytes, speed limit %" PRIu64
+monitor_printf(mon, "Type %s, device %s: Completed %" PRIu64
+   " of %" PRIu64 " bytes, speed limit %" PRIu64
" bytes/s\n",
list->value->type,
list->value->device,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9e9f2d8..b8442f3 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -955,8 +955,8 @@
 # Since: 1.1
 ##
 { 'struct': 'BlockJobInfo',
-  'data': {'type': 'str', 'device': 'str', 'len': 'int',
-   'offset': 'int', 'busy': 'bool', 'paused': 'bool',
+  'data': {'type': 'str', 'device': 'str', 'len': 'size',
+   'offset': 'size', 'busy': 'bool', 'paused': 'bool',
'speed': 'size',
'io-status': 'BlockDeviceIoStatus', 'ready': 'bool'} }
 
@@ -3606,8 +3606,8 @@
 { 'event': 'BLOCK_JOB_COMPLETED',
   'data': { 'type'  : 'BlockJobType',
 'device': 'str',
-'len'   : 'int',
-'offset': 'int',
+'len'   : 'size',
+'offset': 'size',
 'speed' : 'size',
 '*error': 'str' } }
 
@@ -3642,8 +3642,8 @@
 { 'event': 'BLOCK_JOB_CANCELLED',
   'data': { 'type'  : 'BlockJobType',
 'device': 'str',
-'len'   : 'int',
-'offset': 'int',
+'len'   : 'size',
+'offset': 'size',
 'speed' : 'size' } }
 
 ##
@@ -3707,8 +3707,8 @@
 { 'event': 'BLOCK_JOB_READY',
   'data': { 'type'  : 'BlockJobType',
 'device': 'str',
-'len'   : 'int',
-'offset': 'int',
+'len'   : 'size',
+'offset': 'size',
 'speed' : 'size' } }
 
 ##
-- 
2.7.5

[Qemu-block] [RFC PATCH 40/56] blockjob: Lift speed sign conversion out of backup_job_create()

[Markus Armbruster]

backup_job_create() takes int64_t speed.  The underlying BlockJob
abstraction takes uint64_t.  backup_job_create() converts from int64_t
to uint64_t, rejecting negative speed.

Lift this check and conversion out of backup_job_create() into its
callers.  I'm going to lift it further until it falls off the top.

Signed-off-by: Markus Armbruster 
---
 block/backup.c|  7 +--
 blockdev.c| 12 
 include/block/block_int.h |  2 +-
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 3a97836..eddc17a 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -529,7 +529,7 @@ static const BlockJobDriver backup_job_driver = {
 };
 
 BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
-  BlockDriverState *target, int64_t speed,
+  BlockDriverState *target, uint64_t speed,
   MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap,
   bool compress,
   BlockdevOnError on_source_error,
@@ -577,11 +577,6 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 return NULL;
 }
 
-if (speed < 0) {
-error_setg(errp, QERR_INVALID_PARAMETER, "speed");
-return NULL;
-}
-
 if (sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
 if (!sync_bitmap) {
 error_setg(errp, "must provide a valid bitmap name for "
diff --git a/blockdev.c b/blockdev.c
index f9afc32..1deea49 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3210,6 +3210,12 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
BlockJobTxn *txn,
 return NULL;
 }
 
+if (backup->speed < 0) {
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
+   "a non-negative rate limit");
+return NULL;
+}
+
 aio_context = bdrv_get_aio_context(bs);
 aio_context_acquire(aio_context);
 
@@ -3353,6 +3359,12 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, 
BlockJobTxn *txn,
 return NULL;
 }
 
+if (backup->speed < 0) {
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
+   "a non-negative rate limit");
+return NULL;
+}
+
 aio_context = bdrv_get_aio_context(bs);
 aio_context_acquire(aio_context);
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c09076e..19639c0 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -923,7 +923,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
  * until the job is cancelled or manually completed.
  */
 BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
-BlockDriverState *target, int64_t speed,
+BlockDriverState *target, uint64_t speed,
 MirrorSyncMode sync_mode,
 BdrvDirtyBitmap *sync_bitmap,
 bool compress,
-- 
2.7.5

[Qemu-block] [RFC PATCH 31/56] block: Make throttle byte rates and sizes unsigned in QAPI/QMP

[Markus Armbruster]

Sizes and byte rates should use QAPI type 'size' (uint64_t).
BlockIOThrottle and BlockDeviceInfo members @bps, @bps_rd, @bps_wr,
@bps_max, @bps_rd_max, @bps_wr_max, @iops_size are 'int' (int64_t).
qmp_block_set_io_throttle() and bdrv_block_device_info() copy @bps,
@bps_rd, @bps_wr to / from LeakyBucket member @avg (implicit
conversion to / from double), @bps_max, @bps_rd_max, @bps_wr_max to /
from LeakyBucket member @max (implicit conversion to / from double),
and @iops_size to / from ThrottleConfig member op_size (implicit
conversion to / from uint64_t).

Change these BlockIOThrottle and BlockDeviceInfo members to 'size'.

block_set_io_throttle now accepts sizes and rates between 2^63 and
2^64-1.  It accepts negative values as before, because that's how the
QObject input visitor works for backward compatibility.

Doing the same for HMP's block_set_io_throttle deserves its own commit
(the next one).

query-block and query-named-block-nodes now report sizes and rates
above 2^63-1 correctly instead of their (negative) two's complement.

So does HMP's "info block".

Signed-off-by: Markus Armbruster 
---
 hmp.c| 12 ++--
 qapi/block-core.json | 20 ++--
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/hmp.c b/hmp.c
index 9bcdcb3..3253674 100644
--- a/hmp.c
+++ b/hmp.c
@@ -468,17 +468,17 @@ static void print_block_info(Monitor *mon, BlockInfo 
*info,
 if (inserted->bps  || inserted->bps_rd  || inserted->bps_wr  ||
 inserted->iops || inserted->iops_rd || inserted->iops_wr)
 {
-monitor_printf(mon, "I/O throttling:   bps=%" PRId64
-" bps_rd=%" PRId64  " bps_wr=%" PRId64
-" bps_max=%" PRId64
-" bps_rd_max=%" PRId64
-" bps_wr_max=%" PRId64
+monitor_printf(mon, "I/O throttling:   bps=%" PRIu64
+" bps_rd=%" PRIu64  " bps_wr=%" PRIu64
+" bps_max=%" PRIu64
+" bps_rd_max=%" PRIu64
+" bps_wr_max=%" PRIu64
 " iops=%" PRId64 " iops_rd=%" PRId64
 " iops_wr=%" PRId64
 " iops_max=%" PRId64
 " iops_rd_max=%" PRId64
 " iops_wr_max=%" PRId64
-" iops_size=%" PRId64
+" iops_size=%" PRIu64
 " group=%s\n",
 inserted->bps,
 inserted->bps_rd,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9e96590..1e26cb0 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -356,16 +356,16 @@
 '*backing_file': 'str', 'backing_file_depth': 'int',
 'encrypted': 'bool', 'encryption_key_missing': 'bool',
 'detect_zeroes': 'BlockdevDetectZeroesOptions',
-'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
+'bps': 'size', 'bps_rd': 'size', 'bps_wr': 'size',
 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
 'image': 'ImageInfo',
-'*bps_max': 'int', '*bps_rd_max': 'int',
-'*bps_wr_max': 'int', '*iops_max': 'int',
+'*bps_max': 'size', '*bps_rd_max': 'size',
+'*bps_wr_max': 'size', '*iops_max': 'int',
 '*iops_rd_max': 'int', '*iops_wr_max': 'int',
 '*bps_max_length': 'int', '*bps_rd_max_length': 'int',
 '*bps_wr_max_length': 'int', '*iops_max_length': 'int',
 '*iops_rd_max_length': 'int', '*iops_wr_max_length': 'int',
-'*iops_size': 'int', '*group': 'str', 'cache': 'BlockdevCacheInfo',
+'*iops_size': 'size', '*group': 'str', 'cache': 
'BlockdevCacheInfo',
 'write_threshold': 'size' } }
 
 ##
@@ -1866,15 +1866,15 @@
 # Since: 1.1
 ##
 { 'struct': 'BlockIOThrottle',
-  'data': { '*device': 'str', '*id': 'str', 'bps': 'int', 'bps_rd': 'int',
-'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
-'*bps_max': 'int', '*bps_rd_max': 'int',
-'*bps_wr_max': 'int', '*iops_max': 'int',
-'*iops_rd_max': 'int', '*iops_wr_max': 'int',
+  'data': { '*device': 'str', '*id': 'str',
+'bps': 'size', 'bps_rd': 'size', 'bps_wr': 'size',
+'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
+'*bps_max': 'size', '*bps_rd_max': 'size', '*bps_wr_max': 'size',
+'*iops_max': 'int', '*iops_rd_max': 'int', '*iops_wr_max': 'int',
 '*bps_max_length': 'int', '*bps_rd_max_length': 'int',
 '*bps_wr_max_length': 'int', '*iops_max_length': 'int',
 '*iops_rd_max_length': 'int', '*iops_wr_max_length': 'int',
-'*iops_size': 'int', '*group': 'str' } }
+'*iops_size': 'size', '*group': 'str' } }
 
 ##
 # @block-stream:
-- 
2.7.5

[Qemu-block] [RFC PATCH 34/56] block: Make BlockDeviceStats sizes, offsets unsigned in QAPI/QMP

[Markus Armbruster]

Byte counts and file offsets should use QAPI type 'size' (uint64_t).
BlockDeviceStats members @rd_bytes, @wr_bytes and @wr_highest_offset
are 'int' (int64_t).  bdrv_query_blk_stats() gets them from
BlockAcctStats member nr_bytes[] and stat64_get(), implicitly
converting from uint64_t.

Change all three to 'size'.

query-blockstats now report byte counts and file offsets above 2^63-1
correctly instead of their (negative) two's complement.

So does HMP's "info blockstats".

Signed-off-by: Markus Armbruster 
---
 hmp.c| 4 ++--
 qapi/block-core.json | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hmp.c b/hmp.c
index 599e816..ecacb7f 100644
--- a/hmp.c
+++ b/hmp.c
@@ -577,8 +577,8 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
 }
 
 monitor_printf(mon, "%s:", stats->value->device);
-monitor_printf(mon, " rd_bytes=%" PRId64
-   " wr_bytes=%" PRId64
+monitor_printf(mon, " rd_bytes=%" PRIu64
+   " wr_bytes=%" PRIu64
" rd_operations=%" PRId64
" wr_operations=%" PRId64
" flush_operations=%" PRId64
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 2e0d53c..1d68669 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -704,10 +704,10 @@
 # Since: 0.14.0
 ##
 { 'struct': 'BlockDeviceStats',
-  'data': {'rd_bytes': 'int', 'wr_bytes': 'int', 'rd_operations': 'int',
+  'data': {'rd_bytes': 'size', 'wr_bytes': 'size', 'rd_operations': 'int',
'wr_operations': 'int', 'flush_operations': 'int',
'flush_total_time_ns': 'int', 'wr_total_time_ns': 'int',
-   'rd_total_time_ns': 'int', 'wr_highest_offset': 'int',
+   'rd_total_time_ns': 'int', 'wr_highest_offset': 'size',
'rd_merged': 'int', 'wr_merged': 'int', '*idle_time_ns': 'int',
'failed_rd_operations': 'int', 'failed_wr_operations': 'int',
'failed_flush_operations': 'int', 'invalid_rd_operations': 'int',
-- 
2.7.5

[Qemu-block] [RFC PATCH 54/56] qemu-img: blk_getlength() can fail, fix img_map() to check

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 qemu-img.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index 3ae5fe3..cf3ef3e 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2838,6 +2838,10 @@ static int img_map(int argc, char **argv)
 }
 
 length = blk_getlength(blk);
+if (length < 0) {
+error_report("Couldn't get length of image '%s': %s",
+ filename, strerror(-length));
+}
 while (curr.start + curr.length < length) {
 int64_t nsectors_left;
 int64_t sector_num;
-- 
2.7.5

[Qemu-block] [RFC PATCH 44/56] blockjob: Lift speed sign conversion out of blockdev_mirror_common()

[Markus Armbruster]

blockdev_mirror_common() takes int64_t speed.  The underlying BlockJob
abstraction takes uint64_t.  blockdev_mirror_common() converts from
int64_t to uint64_t, rejecting negative speed.

Lift this check and conversion out of blockdev_mirror_common() into
its callers.  I'm going to lift it further until it falls off the top.

Signed-off-by: Markus Armbruster 
---
 blockdev.c | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 06f87e3..13df88b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3419,7 +3419,7 @@ static void blockdev_mirror_common(const char *job_id, 
BlockDriverState *bs,
bool has_replaces, const char *replaces,
enum MirrorSyncMode sync,
BlockMirrorBackingMode backing_mode,
-   bool has_speed, int64_t speed,
+   bool has_speed, uint64_t speed,
bool has_granularity, uint64_t granularity,
bool has_buf_size, int64_t buf_size,
bool has_on_source_error,
@@ -3454,11 +3454,6 @@ static void blockdev_mirror_common(const char *job_id, 
BlockDriverState *bs,
 filter_node_name = NULL;
 }
 
-if (speed < 0) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
-   "a non-negative rate limit");
-return;
-}
 if (granularity != 0 && (granularity < 512 || granularity > 1048576 * 64)) 
{
 error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "granularity",
"a value in range [512B, 64MB]");
@@ -3508,6 +3503,12 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
 return;
 }
 
+if (arg->speed < 0) {
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
+   "a non-negative rate limit");
+return;
+}
+
 aio_context = bdrv_get_aio_context(bs);
 aio_context_acquire(aio_context);
 
@@ -3667,6 +3668,12 @@ void qmp_blockdev_mirror(bool has_job_id, const char 
*job_id,
 return;
 }
 
+if (speed < 0) {
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
+   "a non-negative rate limit");
+return;
+}
+
 aio_context = bdrv_get_aio_context(bs);
 aio_context_acquire(aio_context);
 
-- 
2.7.5

[Qemu-block] [RFC PATCH 32/56] hmp: Make block_set_io_throttle's arguments unsigned

[Markus Armbruster]

The previous commit made them unsigned in QMP.  Switch HMP's args_type
from 'l' to 'o'.  Loses support for expressions (QEMU pocket
calculator), gains support for unit suffixes.  Negative values are no
longer accepted and interpreted modulo 2^64.  Instead, values between
2^63 and 2^64-1 are now accepted.

Signed-off-by: Markus Armbruster 
---
 hmp-commands.hx | 2 +-
 hmp.c   | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 46ce79c..bc3c066 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1668,7 +1668,7 @@ ETEXI
 
 {
 .name   = "block_set_io_throttle",
-.args_type  = 
"device:B,bps:l,bps_rd:l,bps_wr:l,iops:l,iops_rd:l,iops_wr:l",
+.args_type  = 
"device:B,bps:o,bps_rd:o,bps_wr:o,iops:l,iops_rd:l,iops_wr:l",
 .params = "device bps bps_rd bps_wr iops iops_rd iops_wr",
 .help   = "change I/O throttle limits for a block drive",
 .cmd= hmp_block_set_io_throttle,
diff --git a/hmp.c b/hmp.c
index 3253674..599e816 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1764,9 +1764,9 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict 
*qdict)
 BlockIOThrottle throttle = {
 .has_device = true,
 .device = (char *) qdict_get_str(qdict, "device"),
-.bps = qdict_get_int(qdict, "bps"),
-.bps_rd = qdict_get_int(qdict, "bps_rd"),
-.bps_wr = qdict_get_int(qdict, "bps_wr"),
+.bps = qdict_get_uint(qdict, "bps"),
+.bps_rd = qdict_get_uint(qdict, "bps_rd"),
+.bps_wr = qdict_get_uint(qdict, "bps_wr"),
 .iops = qdict_get_int(qdict, "iops"),
 .iops_rd = qdict_get_int(qdict, "iops_rd"),
 .iops_wr = qdict_get_int(qdict, "iops_wr"),
-- 
2.7.5

[Qemu-block] [RFC PATCH 14/56] migration: Fix migrate-set-cache-size error reporting

[Markus Armbruster]

qmp_migrate_set_cache_size() calls xbzrle_cache_resize() to do the
actual work, which in turn calls cache_init() to resize the cache.  If
cache_init() fails, xbzrle_cache_resize() reports that error with
error_report() and fails.  qmp_migrate_set_cache_size() detects the
failure and reports "Parameter 'cache size' expects is smaller than
page size" with error_setg().  The first error is accurate, the second
is bogus.  With HMP, we get both.  With QMP, we get the accurate one
on stderr, and the bogus one via QMP.

Fix by making xbzrle_cache_resize() use error_setg() instead of
error_report().

Signed-off-by: Markus Armbruster 
---
 migration/migration.c | 4 +---
 migration/ram.c   | 9 +++--
 migration/ram.h   | 2 +-
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index c3fe0ed..3ce68f3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1298,10 +1298,8 @@ void qmp_migrate_set_cache_size(int64_t value, Error 
**errp)
 return;
 }
 
-new_size = xbzrle_cache_resize(value);
+new_size = xbzrle_cache_resize(value, errp);
 if (new_size < 0) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
-   "is smaller than page size");
 return;
 }
 
diff --git a/migration/ram.c b/migration/ram.c
index e18b3e2..e9ab858 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -29,6 +29,7 @@
 #include "cpu.h"
 #include 
 #include "qapi-event.h"
+#include "qapi/qmp/qerror.h"
 #include "qemu/cutils.h"
 #include "qemu/bitops.h"
 #include "qemu/bitmap.h"
@@ -110,15 +111,19 @@ static void XBZRLE_cache_unlock(void)
  * hence changes to the cache are protected by XBZRLE.lock().
  *
  * Returns the new_size or negative in case of error.
+ * Returns the the new cache size on success, -1 on error.
  *
  * @new_size: new cache size
+ * @errp: return location for an Error
  */
-int64_t xbzrle_cache_resize(int64_t new_size)
+int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
 {
 PageCache *new_cache;
 int64_t ret;
 
 if (new_size < TARGET_PAGE_SIZE) {
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
+   "is smaller than page size");
 return -1;
 }
 
@@ -131,7 +136,7 @@ int64_t xbzrle_cache_resize(int64_t new_size)
 new_cache = cache_init(new_size / TARGET_PAGE_SIZE,
 TARGET_PAGE_SIZE);
 if (!new_cache) {
-error_report("Error creating cache");
+error_setg(errp, "Error creating cache");
 ret = -1;
 goto out;
 }
diff --git a/migration/ram.h b/migration/ram.h
index c081fde..97842bf 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -35,7 +35,7 @@
 extern MigrationStats ram_counters;
 extern XBZRLECacheStats xbzrle_counters;
 
-int64_t xbzrle_cache_resize(int64_t new_size);
+int64_t xbzrle_cache_resize(int64_t new_size, Error **errp);
 uint64_t ram_bytes_remaining(void);
 uint64_t ram_bytes_total(void);
 
-- 
2.7.5

[Qemu-block] [RFC PATCH 33/56] block: Make block_resize size unsigned in QAPI/QMP

[Markus Armbruster]

Sizes should use QAPI type 'size' (uint64_t).  block_resize parameter
@size is 'int' (int64_t).  qmp_block_resize() ensures it's
non-negative before it passes it on to blk_truncate().

Change parameter @size to 'size', and update the range check
accordingly.  Just cleanup; block_resize accepts the same size values
as before.

Signed-off-by: Markus Armbruster 
---
 blockdev.c   | 7 ---
 qapi/block-core.json | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 960c5be..98dbe51 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2916,7 +2916,7 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
 
 void qmp_block_resize(bool has_device, const char *device,
   bool has_node_name, const char *node_name,
-  int64_t size, Error **errp)
+  uint64_t size, Error **errp)
 {
 Error *local_err = NULL;
 BlockBackend *blk = NULL;
@@ -2940,8 +2940,9 @@ void qmp_block_resize(bool has_device, const char *device,
 goto out;
 }
 
-if (size < 0) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "size", "a >0 size");
+if (size > INT64_MAX) {
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "size",
+   "a size less than 8EiB");
 goto out;
 }
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1e26cb0..2e0d53c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1009,7 +1009,7 @@
 ##
 { 'command': 'block_resize', 'data': { '*device': 'str',
'*node-name': 'str',
-   'size': 'int' }}
+   'size': 'size' }}
 
 ##
 # @NewImageMode:
-- 
2.7.5

[Qemu-block] [RFC PATCH 48/56] block: Make mirror buffer size unsigned in QAPI/QMP

[Markus Armbruster]

Byte counts should use QAPI type 'size' (uint64_t).  Parameter
@buf-size of drive-mirror and blockdev-mirror is 'int' (int64_t).  The
underlying MirrorBlockJob abstraction takes size_t.
mirror_start_job() converts from int64_t to size_t, rejecting negative
sizes (but not values exceeding SIZE_MAX).

Change the two parameters to 'size', and lift the (fixed) check and
the conversion into blockdev_mirror_common().

Signed-off-by: Markus Armbruster 
---
 block/mirror.c| 11 +++
 blockdev.c|  9 +++--
 include/block/block_int.h |  2 +-
 qapi/block-core.json  |  4 ++--
 4 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index f1adda5..f9a5416 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -333,7 +333,7 @@ static uint64_t coroutine_fn 
mirror_iteration(MirrorBlockJob *s)
 int nb_chunks = 1;
 int sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
 bool write_zeroes_ok = bdrv_can_write_zeroes_with_unmap(blk_bs(s->target));
-int max_io_bytes = MAX(s->buf_size / MAX_IN_FLIGHT, MAX_IO_BYTES);
+unsigned max_io_bytes = MAX(s->buf_size / MAX_IN_FLIGHT, MAX_IO_BYTES);
 
 bdrv_dirty_bitmap_lock(s->dirty_bitmap);
 offset = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
@@ -1120,7 +1120,7 @@ static BlockDriver bdrv_mirror_top = {
 static void mirror_start_job(const char *job_id, BlockDriverState *bs,
  int creation_flags, BlockDriverState *target,
  const char *replaces, uint64_t speed,
- uint64_t granularity, int64_t buf_size,
+ uint64_t granularity, size_t buf_size,
  BlockMirrorBackingMode backing_mode,
  BlockdevOnError on_source_error,
  BlockdevOnError on_target_error,
@@ -1147,11 +1147,6 @@ static void mirror_start_job(const char *job_id, 
BlockDriverState *bs,
 /* Granularity must be large enough for sector-based dirty bitmap */
 assert(granularity >= BDRV_SECTOR_SIZE);
 
-if (buf_size < 0) {
-error_setg(errp, "Invalid parameter 'buf-size'");
-return;
-}
-
 if (buf_size == 0) {
 buf_size = DEFAULT_MIRROR_BUF_SIZE;
 }
@@ -1283,7 +1278,7 @@ fail:
 
 void mirror_start(const char *job_id, BlockDriverState *bs,
   BlockDriverState *target, const char *replaces,
-  uint64_t speed, uint64_t granularity, int64_t buf_size,
+  uint64_t speed, uint64_t granularity, size_t buf_size,
   MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
   BlockdevOnError on_source_error,
   BlockdevOnError on_target_error,
diff --git a/blockdev.c b/blockdev.c
index 7feed7a..ba1a960 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3403,7 +3403,7 @@ static void blockdev_mirror_common(const char *job_id, 
BlockDriverState *bs,
BlockMirrorBackingMode backing_mode,
bool has_speed, uint64_t speed,
bool has_granularity, uint64_t granularity,
-   bool has_buf_size, int64_t buf_size,
+   bool has_buf_size, uint64_t buf_size,
bool has_on_source_error,
BlockdevOnError on_source_error,
bool has_on_target_error,
@@ -3447,6 +3447,11 @@ static void blockdev_mirror_common(const char *job_id, 
BlockDriverState *bs,
 return;
 }
 
+if (buf_size > SIZE_MAX) {
+error_setg(errp, "Parameter 'buf-size' is too large");
+return;
+}
+
 if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
 return;
 }
@@ -3619,7 +3624,7 @@ void qmp_blockdev_mirror(bool has_job_id, const char 
*job_id,
  MirrorSyncMode sync,
  bool has_speed, uint64_t speed,
  bool has_granularity, uint64_t granularity,
- bool has_buf_size, int64_t buf_size,
+ bool has_buf_size, uint64_t buf_size,
  bool has_on_source_error,
  BlockdevOnError on_source_error,
  bool has_on_target_error,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 75116e5..8bde408 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -897,7 +897,7 @@ void commit_active_start(const char *job_id, 
BlockDriverState *bs,
  */
 void mirror_start(const char *job_id, BlockDriverState *bs,
   BlockDriverState *target, const char *replaces,
-  uint64_t speed, uint64_t granularity, int64_t buf_size,
+  uint64_t speed, uint64_t granularity, size_t 

[Qemu-block] [RFC PATCH 24/56] block/qcow2: Change align_offset() to operate on uint64_t

[Markus Armbruster]

align_offset() mixes different widths, and its callers pass both
signed and unsigned values.  It's best to stick to unsigned when
twiddling bits.

Signed-off-by: Markus Armbruster 
---
 block/qcow2.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 96a8d43..0d7043e 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -468,10 +468,9 @@ static inline int offset_to_l2_index(BDRVQcow2State *s, 
int64_t offset)
 return (offset >> s->cluster_bits) & (s->l2_size - 1);
 }
 
-static inline int64_t align_offset(int64_t offset, int n)
+static inline uint64_t align_offset(uint64_t offset, uint64_t n)
 {
-offset = (offset + n - 1) & ~(n - 1);
-return offset;
+return (offset + n - 1) & ~(n - 1);
 }
 
 static inline int64_t qcow2_vm_state_offset(BDRVQcow2State *s)
-- 
2.7.5

[Qemu-block] [RFC PATCH 39/56] blockjob: Lift speed sign conversion out of block_job_create()

[Markus Armbruster]

block_job_create() takes int64_t speed.  The underlying RateLimit
abstraction takes uint64_t.  block_job_create() converts from int64_t
to uint64_t, rejecting negative speed.

Lift this check and conversion out of block_job_create() into its
callers.  I'm going to lift it further until it falls off the top.

Signed-off-by: Markus Armbruster 
---
 block/backup.c   | 5 +
 block/commit.c   | 6 ++
 block/mirror.c   | 6 ++
 block/stream.c   | 6 ++
 blockjob.c   | 8 +---
 include/block/blockjob_int.h | 2 +-
 6 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 359e526..3a97836 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -577,6 +577,11 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 return NULL;
 }
 
+if (speed < 0) {
+error_setg(errp, QERR_INVALID_PARAMETER, "speed");
+return NULL;
+}
+
 if (sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
 if (!sync_bitmap) {
 error_setg(errp, "must provide a valid bitmap name for "
diff --git a/block/commit.c b/block/commit.c
index ae9191d..86d780e 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -309,6 +309,12 @@ void commit_start(const char *job_id, BlockDriverState *bs,
 return;
 }
 
+if (speed < 0) {
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
+   "a non-negative rate limit");
+return;
+}
+
 s = block_job_create(job_id, _job_driver, bs, 0, BLK_PERM_ALL,
  speed, BLOCK_JOB_DEFAULT, NULL, NULL, errp);
 if (!s) {
diff --git a/block/mirror.c b/block/mirror.c
index 6c3b446..af54163 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1139,6 +1139,12 @@ static void mirror_start_job(const char *job_id, 
BlockDriverState *bs,
 Error *local_err = NULL;
 int ret;
 
+if (speed < 0) {
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
+   "a non-negative rate limit");
+return;
+}
+
 if (granularity == 0) {
 granularity = bdrv_get_default_bitmap_granularity(target);
 }
diff --git a/block/stream.c b/block/stream.c
index 9a145f2..fefcdb9 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -237,6 +237,12 @@ void stream_start(const char *job_id, BlockDriverState *bs,
 }
 }
 
+if (speed < 0) {
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
+   "a non-negative rate limit");
+return;
+}
+
 /* Prevent concurrent jobs trying to modify the graph structure here, we
  * already have our own plans. Also don't allow resize as the image size is
  * queried only at the job start and then cached. */
diff --git a/blockjob.c b/blockjob.c
index 998ffef..335099e 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -604,7 +604,7 @@ static void block_job_event_completed(BlockJob *job, const 
char *msg)
 
 void *block_job_create(const char *job_id, const BlockJobDriver *driver,
BlockDriverState *bs, uint64_t perm,
-   uint64_t shared_perm, int64_t speed, int flags,
+   uint64_t shared_perm, uint64_t speed, int flags,
BlockCompletionFunc *cb, void *opaque, Error **errp)
 {
 BlockBackend *blk;
@@ -641,12 +641,6 @@ void *block_job_create(const char *job_id, const 
BlockJobDriver *driver,
 }
 }
 
-if (speed < 0) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
-   "a non-negative rate limit");
-return NULL;
-}
-
 blk = blk_new(perm, shared_perm);
 ret = blk_insert_bs(blk, bs, errp);
 if (ret < 0) {
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index dadfd8c..33472ba 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -133,7 +133,7 @@ struct BlockJobDriver {
  */
 void *block_job_create(const char *job_id, const BlockJobDriver *driver,
BlockDriverState *bs, uint64_t perm,
-   uint64_t shared_perm, int64_t speed, int flags,
+   uint64_t shared_perm, uint64_t speed, int flags,
BlockCompletionFunc *cb, void *opaque, Error **errp);
 
 /**
-- 
2.7.5

[Qemu-block] [RFC PATCH 43/56] blockjob: Lift speed sign conversion out of mirror_start()

[Markus Armbruster]

mirror_start() takes int64_t speed.  The underlying BlockJob
abstraction takes uint64_t.  mirror_start() converts from int64_t to
uint64_t, rejecting negative speed.

Lift this check and conversion out of mirror_start() into its caller.
I'm going to lift it further until it falls off the top.

Signed-off-by: Markus Armbruster 
---
 block/mirror.c| 7 +--
 blockdev.c| 5 +
 include/block/block_int.h | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 27b4c7f..af0c989 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1283,7 +1283,7 @@ fail:
 
 void mirror_start(const char *job_id, BlockDriverState *bs,
   BlockDriverState *target, const char *replaces,
-  int64_t speed, uint64_t granularity, int64_t buf_size,
+  uint64_t speed, uint64_t granularity, int64_t buf_size,
   MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
   BlockdevOnError on_source_error,
   BlockdevOnError on_target_error,
@@ -1292,11 +1292,6 @@ void mirror_start(const char *job_id, BlockDriverState 
*bs,
 bool is_none_mode;
 BlockDriverState *base;
 
-if (speed < 0) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
-   "a non-negative rate limit");
-return;
-}
 if (mode == MIRROR_SYNC_MODE_INCREMENTAL) {
 error_setg(errp, "Sync mode 'incremental' not supported");
 return;
diff --git a/blockdev.c b/blockdev.c
index 4d9a665..06f87e3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3454,6 +3454,11 @@ static void blockdev_mirror_common(const char *job_id, 
BlockDriverState *bs,
 filter_node_name = NULL;
 }
 
+if (speed < 0) {
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
+   "a non-negative rate limit");
+return;
+}
 if (granularity != 0 && (granularity < 512 || granularity > 1048576 * 64)) 
{
 error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "granularity",
"a value in range [512B, 64MB]");
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 695d7b3..3ff5536 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -896,7 +896,7 @@ void commit_active_start(const char *job_id, 
BlockDriverState *bs,
  */
 void mirror_start(const char *job_id, BlockDriverState *bs,
   BlockDriverState *target, const char *replaces,
-  int64_t speed, uint64_t granularity, int64_t buf_size,
+  uint64_t speed, uint64_t granularity, int64_t buf_size,
   MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
   BlockdevOnError on_source_error,
   BlockdevOnError on_target_error,
-- 
2.7.5

[Qemu-block] [RFC PATCH 50/56] block: Make BLOCK_IMAGE_CORRUPTED offset, size unsigned in QAPI/QMP

[Markus Armbruster]

File offsets should use QAPI type 'size' (uint64_t).
BLOCK_IMAGE_CORRUPTED parameters @offset and @size are 'int'
(int64_t).  qcow2_signal_corruption() passes non-negative int64_t
values.

Change the event parameters to 'size', for QAPI/QMP consistency.

Signed-off-by: Markus Armbruster 
---
 qapi/block-core.json | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 3c6d448..64b84a5 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3520,8 +3520,8 @@
   'data': { 'device' : 'str',
 '*node-name' : 'str',
 'msg': 'str',
-'*offset': 'int',
-'*size'  : 'int',
+'*offset': 'size',
+'*size'  : 'size',
 'fatal'  : 'bool' } }
 
 ##
-- 
2.7.5

[Qemu-block] [RFC PATCH 37/56] blockjob: Make BlockJobInfo and event speed unsigned in QAPI/QMP

[Markus Armbruster]

Byte rates should use QAPI type 'size' (uint64_t).  BlockJobInfo
member @speed and parameter @speed of events BLOCK_JOB_COMPLETED,
BLOCK_JOB_CANCELLED, BLOCK_JOB_READY are 'int' (int64_t).

block_job_query(), block_job_event_completed(),
block_job_event_cancelled(), block_job_event_ready() all get it from
BlockJob member @speed, implicitly converting from uint64_t (since the
commit before previous).  The conversion is safe, because
block_job_set_speed() won't set speeds above INT_MAX.

Change the BlockJobInfo member and the event parameters to 'size', for
QAPI/QMP consistency.

Signed-off-by: Markus Armbruster 
---
 hmp.c| 4 ++--
 qapi/block-core.json | 9 +
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/hmp.c b/hmp.c
index ecacb7f..fae974e 100644
--- a/hmp.c
+++ b/hmp.c
@@ -967,7 +967,7 @@ void hmp_info_block_jobs(Monitor *mon, const QDict *qdict)
 while (list) {
 if (strcmp(list->value->type, "stream") == 0) {
 monitor_printf(mon, "Streaming device %s: Completed %" PRId64
-   " of %" PRId64 " bytes, speed limit %" PRId64
+   " of %" PRId64 " bytes, speed limit %" PRIu64
" bytes/s\n",
list->value->device,
list->value->offset,
@@ -975,7 +975,7 @@ void hmp_info_block_jobs(Monitor *mon, const QDict *qdict)
list->value->speed);
 } else {
 monitor_printf(mon, "Type %s, device %s: Completed %" PRId64
-   " of %" PRId64 " bytes, speed limit %" PRId64
+   " of %" PRId64 " bytes, speed limit %" PRIu64
" bytes/s\n",
list->value->type,
list->value->device,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1d68669..ceaab43 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -956,7 +956,8 @@
 ##
 { 'struct': 'BlockJobInfo',
   'data': {'type': 'str', 'device': 'str', 'len': 'int',
-   'offset': 'int', 'busy': 'bool', 'paused': 'bool', 'speed': 'int',
+   'offset': 'int', 'busy': 'bool', 'paused': 'bool',
+   'speed': 'size',
'io-status': 'BlockDeviceIoStatus', 'ready': 'bool'} }
 
 ##
@@ -3607,7 +3608,7 @@
 'device': 'str',
 'len'   : 'int',
 'offset': 'int',
-'speed' : 'int',
+'speed' : 'size',
 '*error': 'str' } }
 
 ##
@@ -3643,7 +3644,7 @@
 'device': 'str',
 'len'   : 'int',
 'offset': 'int',
-'speed' : 'int' } }
+'speed' : 'size' } }
 
 ##
 # @BLOCK_JOB_ERROR:
@@ -3708,7 +3709,7 @@
 'device': 'str',
 'len'   : 'int',
 'offset': 'int',
-'speed' : 'int' } }
+'speed' : 'size' } }
 
 ##
 # @PreallocMode:
-- 
2.7.5

[Qemu-block] [RFC PATCH 30/56] block: Make write thresholds unsigned in QAPI/QMP

[Markus Armbruster]

File offsets should use QAPI type 'size' (uint64_t).
block-set-write-threshold parameter @write-threshold is 'int'
(int64_t).  qmp_block_set_write_threshold() passes it on to
bdrv_write_threshold_set(), implicitly converting to uint64_t.
BLOCK_WRITE_THRESHOLD parameters @write-threshold, @amount-exceeded
are 'int'.  before_write_notify() gets them from BlockDriverState
member write_threshold_offset and bdrv_write_threshold_exceeded(),
implicitly converting from uint64_t.  BlockDeviceInfo members
@write_threshold is 'int'.  bdrv_block_device_info() gets it from
bdrv_write_threshold_get(), implicitly converting from uint64_t.

Change them all to 'size'.  Drop a redundant initializer while there.

block-set-write-threshold now accepts file offsets between 2^63 and
2^64-1.  It accepts negative values as before, because that's how the
QObject input visitor works for backward compatibility.

There is no matching HMP command.

BLOCK_WRITE_THRESHOLD, query-block and query-named-block-nodes now
report write threshold values above 2^63-1 correctly instead of their
(negative) two's complement.

HMP's info block does not report write thresholds.

Signed-off-by: Markus Armbruster 
---
 block/write-threshold.c | 2 +-
 qapi/block-core.json| 8 
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/write-threshold.c b/block/write-threshold.c
index 0bd1a01..c4572d9 100644
--- a/block/write-threshold.c
+++ b/block/write-threshold.c
@@ -56,7 +56,7 @@ static int coroutine_fn 
before_write_notify(NotifierWithReturn *notifier,
 {
 BdrvTrackedRequest *req = opaque;
 BlockDriverState *bs = req->bs;
-uint64_t amount = 0;
+uint64_t amount;
 
 amount = bdrv_write_threshold_exceeded(bs, req);
 if (amount > 0) {
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 60e1b6f..9e96590 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -366,7 +366,7 @@
 '*bps_wr_max_length': 'int', '*iops_max_length': 'int',
 '*iops_rd_max_length': 'int', '*iops_wr_max_length': 'int',
 '*iops_size': 'int', '*group': 'str', 'cache': 'BlockdevCacheInfo',
-'write_threshold': 'int' } }
+'write_threshold': 'size' } }
 
 ##
 # @BlockDeviceIoStatus:
@@ -3748,8 +3748,8 @@
 ##
 { 'event': 'BLOCK_WRITE_THRESHOLD',
   'data': { 'node-name': 'str',
-'amount-exceeded': 'uint64',
-'write-threshold': 'uint64' } }
+'amount-exceeded': 'size',
+'write-threshold': 'size' } }
 
 ##
 # @block-set-write-threshold:
@@ -3779,7 +3779,7 @@
 #
 ##
 { 'command': 'block-set-write-threshold',
-  'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
+  'data': { 'node-name': 'str', 'write-threshold': 'size' } }
 
 ##
 # @x-blockdev-change:
-- 
2.7.5

[Qemu-block] [RFC PATCH 23/56] option: Fix type of qemu_opt_set_number() parameter @val

[Markus Armbruster]

Parameter @val is int64_t.  It's assigned to opt->value.uint, which is
uint64_t, because that's what QemuOpts integers are.  Screwed up when
the function was added in commit b83c18e.  Change @val to uint64_t.

Signed-off-by: Markus Armbruster 
---
 include/qemu/option.h | 2 +-
 util/qemu-option.c| 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/qemu/option.h b/include/qemu/option.h
index f7338db..d812da4 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -92,7 +92,7 @@ void qemu_opt_set(QemuOpts *opts, const char *name, const 
char *value,
   Error **errp);
 void qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val,
Error **errp);
-void qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val,
+void qemu_opt_set_number(QemuOpts *opts, const char *name, uint64_t val,
  Error **errp);
 typedef int (*qemu_opt_loopfunc)(void *opaque,
  const char *name, const char *value,
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 9b1dc80..ca4f737 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -579,7 +579,7 @@ void qemu_opt_set_bool(QemuOpts *opts, const char *name, 
bool val,
 QTAILQ_INSERT_TAIL(>head, opt, next);
 }
 
-void qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val,
+void qemu_opt_set_number(QemuOpts *opts, const char *name, uint64_t val,
  Error **errp)
 {
 QemuOpt *opt;
-- 
2.7.5

[Qemu-block] [RFC PATCH 35/56] blockjob: Lift speed sign conversion into block_job_set_speed()

[Markus Armbruster]

The BlockJob abstraction takes int64_t speed.  The underlying
RateLimit abstraction takes uint64_t.  We convert from int64_t to
uint64_t in the BlockJobDriver speed_set() methods.  They all reject
negative speed.

Lift this check and conversion up into the method's caller
block_job_set_speed().  I'm going to lift it further until it falls
off the top.

Improve the error message while there.

Signed-off-by: Markus Armbruster 
---
 block/backup.c   | 6 +-
 block/commit.c   | 6 +-
 block/mirror.c   | 6 +-
 block/stream.c   | 6 +-
 blockjob.c   | 7 +++
 include/block/blockjob.h | 2 +-
 include/block/blockjob_int.h | 2 +-
 7 files changed, 13 insertions(+), 22 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index a37c944..b76143d 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -188,14 +188,10 @@ static int coroutine_fn backup_before_write_notify(
 return backup_do_cow(job, req->offset, req->bytes, NULL, true);
 }
 
-static void backup_set_speed(BlockJob *job, int64_t speed, Error **errp)
+static void backup_set_speed(BlockJob *job, uint64_t speed, Error **errp)
 {
 BackupBlockJob *s = container_of(job, BackupBlockJob, common);
 
-if (speed < 0) {
-error_setg(errp, QERR_INVALID_PARAMETER, "speed");
-return;
-}
 ratelimit_set_speed(>limit, speed, SLICE_TIME);
 }
 
diff --git a/block/commit.c b/block/commit.c
index c7857c3..5dc1c73 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -220,14 +220,10 @@ out:
 block_job_defer_to_main_loop(>common, commit_complete, data);
 }
 
-static void commit_set_speed(BlockJob *job, int64_t speed, Error **errp)
+static void commit_set_speed(BlockJob *job, uint64_t speed, Error **errp)
 {
 CommitBlockJob *s = container_of(job, CommitBlockJob, common);
 
-if (speed < 0) {
-error_setg(errp, QERR_INVALID_PARAMETER, "speed");
-return;
-}
 ratelimit_set_speed(>limit, speed, SLICE_TIME);
 }
 
diff --git a/block/mirror.c b/block/mirror.c
index 37a8744..7959a7f 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -928,14 +928,10 @@ immediate_exit:
 block_job_defer_to_main_loop(>common, mirror_exit, data);
 }
 
-static void mirror_set_speed(BlockJob *job, int64_t speed, Error **errp)
+static void mirror_set_speed(BlockJob *job, uint64_t speed, Error **errp)
 {
 MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
 
-if (speed < 0) {
-error_setg(errp, QERR_INVALID_PARAMETER, "speed");
-return;
-}
 ratelimit_set_speed(>limit, speed, SLICE_TIME);
 }
 
diff --git a/block/stream.c b/block/stream.c
index e6f7234..11b6673 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -207,14 +207,10 @@ out:
 block_job_defer_to_main_loop(>common, stream_complete, data);
 }
 
-static void stream_set_speed(BlockJob *job, int64_t speed, Error **errp)
+static void stream_set_speed(BlockJob *job, uint64_t speed, Error **errp)
 {
 StreamBlockJob *s = container_of(job, StreamBlockJob, common);
 
-if (speed < 0) {
-error_setg(errp, QERR_INVALID_PARAMETER, "speed");
-return;
-}
 ratelimit_set_speed(>limit, speed, SLICE_TIME);
 }
 
diff --git a/blockjob.c b/blockjob.c
index 70a7818..7f77b7e 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -460,6 +460,13 @@ void block_job_set_speed(BlockJob *job, int64_t speed, 
Error **errp)
 error_setg(errp, QERR_UNSUPPORTED);
 return;
 }
+
+if (speed < 0) {
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
+   "a non-negative rate limit");
+return;
+}
+
 job->driver->set_speed(job, speed, _err);
 if (local_err) {
 error_propagate(errp, local_err);
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 67c0968..bf5314b 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -110,7 +110,7 @@ typedef struct BlockJob {
 int64_t len;
 
 /** Speed that was set with @block_job_set_speed.  */
-int64_t speed;
+uint64_t speed;
 
 /** The completion function that will be called when the job completes.  */
 BlockCompletionFunc *cb;
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index f13ad05..2d10794 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -42,7 +42,7 @@ struct BlockJobDriver {
 BlockJobType job_type;
 
 /** Optional callback for job types that support setting a speed limit */
-void (*set_speed)(BlockJob *job, int64_t speed, Error **errp);
+void (*set_speed)(BlockJob *job, uint64_t speed, Error **errp);
 
 /** Mandatory: Entrypoint for the Coroutine. */
 CoroutineEntry *start;
-- 
2.7.5

[Qemu-block] [RFC PATCH 06/56] char: Don't truncate -chardev and HMP chardev-add ringbuf size

[Markus Armbruster]

qemu_chr_parse_ringbuf() initializes the new ChardevRingbuf's @size to
the value of qemu_opt_get_size().  Except it first truncates the value
from uint64_t to int.  Fix that, so you can waste your RAM on
multi-gigabyte ring buffers.

Signed-off-by: Markus Armbruster 
---
 chardev/char-ringbuf.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/chardev/char-ringbuf.c b/chardev/char-ringbuf.c
index a9205ea..9275ae9 100644
--- a/chardev/char-ringbuf.c
+++ b/chardev/char-ringbuf.c
@@ -198,18 +198,14 @@ char *qmp_ringbuf_read(const char *device, uint64_t size,
 static void qemu_chr_parse_ringbuf(QemuOpts *opts, ChardevBackend *backend,
Error **errp)
 {
-int val;
 ChardevRingbuf *ringbuf;
 
 backend->type = CHARDEV_BACKEND_KIND_RINGBUF;
 ringbuf = backend->u.ringbuf.data = g_new0(ChardevRingbuf, 1);
 qemu_chr_parse_common(opts, qapi_ChardevRingbuf_base(ringbuf));
 
-val = qemu_opt_get_size(opts, "size", 0);
-if (val != 0) {
-ringbuf->has_size = true;
-ringbuf->size = val;
-}
+ringbuf->size = qemu_opt_get_size(opts, "size", 0);
+ringbuf->has_size = ringbuf->size != 0;
 }
 
 static void char_ringbuf_class_init(ObjectClass *oc, void *data)
-- 
2.7.5

[Qemu-block] [RFC PATCH 10/56] hmp: Make balloon's argument unsigned

[Markus Armbruster]

The previous commit made it unsigned in QMP.  Switch HMP's args_type
from 'M' to 'o'.  Loses support for expressions (QEMU pocket
calculator), gains support for units other than mebibytes.  Negative
values are no longer accepted and interpreted modulo 2^64.  Instead,
values between 2^63 and 2^64-1 are now accepted.

Signed-off-by: Markus Armbruster 
---
 hmp-commands.hx | 2 +-
 hmp.c   | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 1941e19..46ce79c 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1433,7 +1433,7 @@ ETEXI
 
 {
 .name   = "balloon",
-.args_type  = "value:M",
+.args_type  = "value:o",
 .params = "target",
 .help   = "request VM to change its memory allocation (in MB)",
 .cmd= hmp_balloon,
diff --git a/hmp.c b/hmp.c
index 4556045..1932a11 100644
--- a/hmp.c
+++ b/hmp.c
@@ -781,7 +781,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict)
 return;
 }
 
-monitor_printf(mon, "balloon: actual=%" PRIu64 "\n", info->actual >> 20);
+monitor_printf(mon, "balloon: actual=%" PRId64 "\n", info->actual >> 20);
 
 qapi_free_BalloonInfo(info);
 }
@@ -1178,7 +1178,7 @@ void hmp_block_passwd(Monitor *mon, const QDict *qdict)
 
 void hmp_balloon(Monitor *mon, const QDict *qdict)
 {
-int64_t value = qdict_get_int(qdict, "value");
+uint64_t value = qdict_get_uint(qdict, "value");
 Error *err = NULL;
 
 qmp_balloon(value, );
-- 
2.7.5

[Qemu-block] [RFC PATCH 19/56] block: Make snapshot VM state size unsigned in QAPI/QMP

[Markus Armbruster]

Sizes should use QAPI type 'size' (uint64_t).  SnapshotInfo member
@vm-state-size is 'int' (int64_t).  QEMUSnapshotInfo member
@vm_state_size is uint64_t.  bdrv_query_snapshot_info_list(),
bdrv_image_info_dump(), qmp_blockdev_snapshot_delete_internal_sync()
convert implicitly between the two.

Change the SnapshotInfo member to 'size'.

query-named-block-nodes, query-block and
blockdev-snapshot-delete-internal-sync now report VM state sizes above
2^63-1 correctly instead of their (negative) two's complement.

HMP's "info snapshots" and "info block" still report negative values,
because bdrv_snapshot_dump() passes the size to
get_human_readable_size(), which is still signed.  To be fixed soon.

Same for "qemu-img snapshot -l" and "qemu-img info".

Signed-off-by: Markus Armbruster 
---
 qapi/block-core.json | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 27790f3..ecfeecd 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -28,7 +28,7 @@
 #
 ##
 { 'struct': 'SnapshotInfo',
-  'data': { 'id': 'str', 'name': 'str', 'vm-state-size': 'int',
+  'data': { 'id': 'str', 'name': 'str', 'vm-state-size': 'size',
 'date-sec': 'int', 'date-nsec': 'int',
 'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
 
-- 
2.7.5

[Qemu-block] [RFC PATCH 28/56] block: Widen dirty bitmap granularity to uint64_t for safety

[Markus Armbruster]

Block dirty bitmaps represent granularity in bytes as uint32_t.  It
must be a power of two and a multiple of BDRV_SECTOR_SIZE.

The trouble with uint32_t is computations like this one in
mirror_do_read():

uint64_t max_bytes;

max_bytes = s->granularity * s->max_iov;

The operands of * are uint32_t and int, so the product is computed in
uint32_t (assuming 32 bit int), then zero-extended to uint64_t.

Since granularity is generally combined with 64 bit file offsets, it's
best to make it 64 bits, too.  Less opportunity to screw up.

Signed-off-by: Markus Armbruster 
---
 block.c  |  2 +-
 block/backup.c   |  2 +-
 block/dirty-bitmap.c | 19 +++
 block/mirror.c   |  6 +++---
 block/qcow2-bitmap.c | 18 +-
 block/qcow2.h|  2 +-
 blockdev.c   |  6 +++---
 include/block/block.h|  2 +-
 include/block/block_int.h|  4 ++--
 include/block/dirty-bitmap.h |  7 +++
 qapi/block-core.json |  8 
 11 files changed, 35 insertions(+), 41 deletions(-)

diff --git a/block.c b/block.c
index 04cce0d..fe4b089 100644
--- a/block.c
+++ b/block.c
@@ -4922,7 +4922,7 @@ void bdrv_del_child(BlockDriverState *parent_bs, 
BdrvChild *child, Error **errp)
 }
 
 bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
- uint32_t granularity, Error **errp)
+ uint64_t granularity, Error **errp)
 {
 BlockDriver *drv = bs->drv;
 
diff --git a/block/backup.c b/block/backup.c
index 504a089..a37c944 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -363,7 +363,7 @@ static int coroutine_fn 
backup_run_incremental(BackupBlockJob *job)
 bool error_is_read;
 int ret = 0;
 int clusters_per_iter;
-uint32_t granularity;
+uint64_t granularity;
 int64_t offset;
 int64_t cluster;
 int64_t end;
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 5b1699c..c0b5952 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -109,13 +109,13 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
 
 /* Called with BQL taken.  */
 BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
-  uint32_t granularity,
+  uint64_t granularity,
   const char *name,
   Error **errp)
 {
 int64_t bitmap_size;
 BdrvDirtyBitmap *bitmap;
-uint32_t sector_granularity;
+uint64_t sector_granularity;
 
 assert((granularity & (granularity - 1)) == 0);
 
@@ -133,7 +133,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState 
*bs,
 }
 bitmap = g_new0(BdrvDirtyBitmap, 1);
 bitmap->mutex = >dirty_bitmap_mutex;
-bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(sector_granularity));
+bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz64(sector_granularity));
 bitmap->size = bitmap_size;
 bitmap->name = g_strdup(name);
 bitmap->disabled = false;
@@ -178,7 +178,7 @@ int bdrv_dirty_bitmap_get_meta_locked(BlockDriverState *bs,
   int nb_sectors)
 {
 uint64_t i;
-int sectors_per_bit = 1 << hbitmap_granularity(bitmap->meta);
+uint64_t sectors_per_bit = 1ull << hbitmap_granularity(bitmap->meta);
 
 /* To optimize: we can make hbitmap to internally check the range in a
  * coarse level, or at least do it word by word. */
@@ -491,10 +491,10 @@ int bdrv_get_dirty_locked(BlockDriverState *bs, 
BdrvDirtyBitmap *bitmap,
  * but clamped between [4K, 64K]. Defaults to 64K in the case that there
  * is no cluster size information available.
  */
-uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
+uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
 {
 BlockDriverInfo bdi;
-uint32_t granularity;
+uint64_t granularity;
 
 if (bdrv_get_info(bs, ) >= 0 && bdi.cluster_size > 0) {
 granularity = MAX(4096, bdi.cluster_size);
@@ -506,16 +506,11 @@ uint32_t 
bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
 return granularity;
 }
 
-uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap)
+uint64_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap)
 {
 return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
 }
 
-uint32_t bdrv_dirty_bitmap_meta_granularity(BdrvDirtyBitmap *bitmap)
-{
-return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->meta);
-}
-
 BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
  uint64_t first_sector)
 {
diff --git a/block/mirror.c b/block/mirror.c
index 14c34e9..37a8744 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -53,7 +53,7 @@ typedef struct MirrorBlockJob {
 BlockdevOnError on_source_error, on_target_error;
 

[Qemu-block] [RFC PATCH 12/56] pc-dimm: Make size and address unsigned in QAPI/QMP

[Markus Armbruster]

Sizes and addresses should use QAPI type 'size' (uint64_t).
PCDIMMDeviceInfo members @addr and @size are 'int' (int64_t).
qmp_pc_dimm_device_list() implicitly converts from uint64_t.

Change these PCDIMMDeviceInfo members to 'size'.

query-memory-devices now reports sizes and addresses above 2^63-1
correctly instead of their (negative) two's complement.

HMP's "info memory-devices" already reported them correctly, because
it printed the signed integers with PRIx64 and PRIu32.

Signed-off-by: Markus Armbruster 
---
 qapi-schema.json | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 23eb60d..6aa6be9 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -6057,8 +6057,8 @@
 ##
 { 'struct': 'PCDIMMDeviceInfo',
   'data': { '*id': 'str',
-'addr': 'int',
-'size': 'int',
+'addr': 'size',
+'size': 'size',
 'slot': 'int',
 'node': 'int',
 'memdev': 'str',
-- 
2.7.5

[Qemu-block] [RFC PATCH 27/56] block/dirty-bitmap: Clean up signed vs. unsigned dirty counts

[Markus Armbruster]

hbitmap_count() returns uint64_t.

Clean up test-hbitmap.c to check its value with g_assert_cmpuint()
instead of g_assert_cmpint().

bdrv_get_dirty_count() and bdrv_get_meta_dirty_count() return its
value converted to int64_t.  Clean them up to return it unadulterated.

This moves the implicit conversion to some callers, so clean them up,
too.

mirror_run() assigns the value of bdrv_get_meta_dirty_count() to a
local int64_t variable.  Change it to uint64_t.  Signedness still gets
mixed up in the computation of s->common.len, but messing with that is
more than I can handle right now.

get_remaining_dirty() tallies bdrv_get_dirty_count() values in
int64_t.  Its caller block_save_pending() converts it back to
uint64_t.  Change get_remaining_dirty() to uint64_t.

Signed-off-by: Markus Armbruster 
---
 block/dirty-bitmap.c |  4 ++--
 block/mirror.c   |  4 ++--
 block/trace-events   |  8 
 include/block/dirty-bitmap.h |  4 ++--
 migration/block.c|  4 ++--
 tests/test-hbitmap.c | 16 +---
 6 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 30462d4..5b1699c 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -681,12 +681,12 @@ void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, 
int64_t sector_num)
 hbitmap_iter_init(>hbi, iter->hbi.hb, sector_num);
 }
 
-int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
+uint64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
 {
 return hbitmap_count(bitmap->bitmap);
 }
 
-int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
+uint64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
 {
 return hbitmap_count(bitmap->meta);
 }
diff --git a/block/mirror.c b/block/mirror.c
index c9a6a3c..14c34e9 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -798,8 +798,8 @@ static void coroutine_fn mirror_run(void *opaque)
 assert(!s->dbi);
 s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap, 0);
 for (;;) {
-uint64_t delay_ns = 0;
-int64_t cnt, delta;
+uint64_t cnt, delay_ns = 0;
+int64_t delta;
 bool should_complete;
 
 if (s->ret < 0) {
diff --git a/block/trace-events b/block/trace-events
index 071a8d7..464a11f 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -24,13 +24,13 @@ commit_start(void *bs, void *base, void *top, void *s) "bs 
%p base %p top %p s %
 
 # block/mirror.c
 mirror_start(void *bs, void *s, void *opaque) "bs %p s %p opaque %p"
-mirror_restart_iter(void *s, int64_t cnt) "s %p dirty count %"PRId64
+mirror_restart_iter(void *s, uint64_t cnt) "s %p dirty count %" PRIu64
 mirror_before_flush(void *s) "s %p"
-mirror_before_drain(void *s, int64_t cnt) "s %p dirty count %"PRId64
-mirror_before_sleep(void *s, int64_t cnt, int synced, uint64_t delay_ns) "s %p 
dirty count %"PRId64" synced %d delay %"PRIu64"ns"
+mirror_before_drain(void *s, uint64_t cnt) "s %p dirty count %" PRIu64
+mirror_before_sleep(void *s, uint64_t cnt, int synced, uint64_t delay_ns) "s 
%p dirty count %" PRIu64 " synced %d delay %" PRIu64 "ns"
 mirror_one_iteration(void *s, int64_t offset, uint64_t bytes) "s %p offset %" 
PRId64 " bytes %" PRIu64
 mirror_iteration_done(void *s, int64_t offset, uint64_t bytes, int ret) "s %p 
offset %" PRId64 " bytes %" PRIu64 " ret %d"
-mirror_yield(void *s, int64_t cnt, int buf_free_count, int in_flight) "s %p 
dirty count %"PRId64" free buffers %d in_flight %d"
+mirror_yield(void *s, uint64_t cnt, int buf_free_count, int in_flight) "s %p 
dirty count %" PRIu64 " free buffers %d in_flight %d"
 mirror_yield_in_flight(void *s, int64_t offset, int in_flight) "s %p offset %" 
PRId64 " in_flight %d"
 
 # block/backup.c
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index a79a58d..d7e0f61 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -91,8 +91,8 @@ void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
 int64_t cur_sector, int64_t nr_sectors);
 int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
 void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t sector_num);
-int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
-int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
+uint64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
+uint64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
 bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
 bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
diff --git a/migration/block.c b/migration/block.c
index 9171f60..59b7551 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -656,10 +656,10 @@ static int flush_blks(QEMUFile *f)
 
 /* Called with iothread lock taken.  */
 
-static int64_t get_remaining_dirty(void)
+static uint64_t get_remaining_dirty(void)
 {
 BlkMigDevState *bmds;
-int64_t dirty = 

[Qemu-block] [RFC PATCH 26/56] block: Make BlockMeasureInfo sizes unsigned in QAPI

[Markus Armbruster]

Sizes should use QAPI type 'size' (uint64_t).  BlockMeasureInfo
members @required and @fully-allocated are 'int' (int64_t).

qcow2_measure() computes their values from qcow2_calc_prealloc_size(),
@virtual_size and @required, all uint64_t (the former only since the
previous commit).

raw_measure() computes them either from bdrv_getlength() or from
qemu_opt_get_size_del().  The former is int64_t, but we error out if
it's negative.  The latter is uint64_t.

Change these BlockMeasureInfo members to 'size'.

qemu-img now reports them correctly above 2^63-1 instead of their
(negative) two's complement.

Signed-off-by: Markus Armbruster 
---
 block/raw-format.c   | 10 ++
 qapi/block-core.json |  2 +-
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/raw-format.c b/block/raw-format.c
index 142649e..6b73d5b 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -316,14 +316,16 @@ static BlockMeasureInfo *raw_measure(QemuOpts *opts, 
BlockDriverState *in_bs,
  Error **errp)
 {
 BlockMeasureInfo *info;
-int64_t required;
+int64_t size;
+uint64_t required;
 
 if (in_bs) {
-required = bdrv_getlength(in_bs);
-if (required < 0) {
-error_setg_errno(errp, -required, "Unable to get image size");
+size = bdrv_getlength(in_bs);
+if (size < 0) {
+error_setg_errno(errp, -size, "Unable to get image size");
 return NULL;
 }
+required = size;
 } else {
 required = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
 BDRV_SECTOR_SIZE);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 02e12f7..bc8e5b6 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -485,7 +485,7 @@
 # Since: 2.10
 ##
 { 'struct': 'BlockMeasureInfo',
-  'data': {'required': 'int', 'fully-allocated': 'int'} }
+  'data': {'required': 'size', 'fully-allocated': 'size'} }
 
 ##
 # @query-block:
-- 
2.7.5

[Qemu-block] [RFC PATCH 15/56] migration: Make XBZRLE cache size unsigned in QAPI/QMP

[Markus Armbruster]

Sizes should use QAPI type 'size' (uint64_t).  migrate-set-cache-size
parameter @value is 'int' (int64_t).  qmp_migrate_set_cache_size()
ensures it fits into size_t.  page_cache.c implicitly converts the
signed size to unsigned types (it can't quite decide whether to use
uint64_t or size_t for cache offsets, but that's not something I can
tackle now).

XBZRLECacheStats member @cache-size and query-migrate-cache-size's
result are also 'int'.

Change the migrate-set-cache-size parameter and the XBZRLECacheStats
members to 'size', fix up hmp_migrate_set_cache_size(), and tweak a
few variable types to reduce implicit conversions.

migrate-set-cache-size now accepts size values between 2^63 and
2^64-1.  It accepts negative values as before, because that's how the
QObject input visitor works for backward compatibility.

So does HMP's migrate_set_cache_size.

query-migrate and query-migrate-cache-size now report cache sizes
above 2^63-1 correctly instead of their (negative) two's complement.

So does HMP's "info migrate_cache_size".  HMP's "info migrate" already
reported the cache size correctly, because it printed the signed
integer with PRIu32.

Signed-off-by: Markus Armbruster 
---
 hmp.c  |  4 ++--
 migration/migration.c  | 10 +-
 migration/migration.h  |  4 ++--
 migration/page_cache.c | 20 +---
 migration/page_cache.h |  2 +-
 migration/ram.c|  5 ++---
 migration/ram.h|  2 +-
 qapi-schema.json   |  6 +++---
 8 files changed, 25 insertions(+), 28 deletions(-)

diff --git a/hmp.c b/hmp.c
index 1932a11..184fb8b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -344,7 +344,7 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict 
*qdict)
 
 void hmp_info_migrate_cache_size(Monitor *mon, const QDict *qdict)
 {
-monitor_printf(mon, "xbzrel cache size: %" PRId64 " kbytes\n",
+monitor_printf(mon, "xbzrel cache size: %" PRIu64 " kbytes\n",
qmp_query_migrate_cache_size(NULL) >> 10);
 }
 
@@ -1504,7 +1504,7 @@ void hmp_migrate_set_downtime(Monitor *mon, const QDict 
*qdict)
 
 void hmp_migrate_set_cache_size(Monitor *mon, const QDict *qdict)
 {
-int64_t value = qdict_get_int(qdict, "value");
+uint64_t value = qdict_get_uint(qdict, "value");
 Error *err = NULL;
 
 qmp_migrate_set_cache_size(value, );
diff --git a/migration/migration.c b/migration/migration.c
index 3ce68f3..2d7f3a2 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1279,13 +1279,13 @@ void qmp_migrate_cancel(Error **errp)
 migrate_fd_cancel(migrate_get_current());
 }
 
-void qmp_migrate_set_cache_size(int64_t value, Error **errp)
+void qmp_migrate_set_cache_size(uint64_t value, Error **errp)
 {
 MigrationState *s = migrate_get_current();
-int64_t new_size;
+ssize_t new_size;
 
 /* Check for truncation */
-if (value != (size_t)value) {
+if (value != (ssize_t)value) {
 error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
"exceeding address space");
 return;
@@ -1306,7 +1306,7 @@ void qmp_migrate_set_cache_size(int64_t value, Error 
**errp)
 s->xbzrle_cache_size = new_size;
 }
 
-int64_t qmp_query_migrate_cache_size(Error **errp)
+uint64_t qmp_query_migrate_cache_size(Error **errp)
 {
 return migrate_xbzrle_cache_size();
 }
@@ -1431,7 +1431,7 @@ int migrate_use_xbzrle(void)
 return s->enabled_capabilities[MIGRATION_CAPABILITY_XBZRLE];
 }
 
-int64_t migrate_xbzrle_cache_size(void)
+size_t migrate_xbzrle_cache_size(void)
 {
 MigrationState *s;
 
diff --git a/migration/migration.h b/migration/migration.h
index 148c9fa..e4708e1 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -106,7 +106,7 @@ struct MigrationState
 int64_t downtime;
 int64_t expected_downtime;
 bool enabled_capabilities[MIGRATION_CAPABILITY__MAX];
-int64_t xbzrle_cache_size;
+size_t xbzrle_cache_size;
 int64_t setup_time;
 
 /* Flag set once the migration has been asked to enter postcopy */
@@ -172,7 +172,7 @@ bool migrate_zero_blocks(void);
 bool migrate_auto_converge(void);
 
 int migrate_use_xbzrle(void);
-int64_t migrate_xbzrle_cache_size(void);
+size_t migrate_xbzrle_cache_size(void);
 bool migrate_colo_enabled(void);
 
 bool migrate_use_block(void);
diff --git a/migration/page_cache.c b/migration/page_cache.c
index ba984c4..226ea72 100644
--- a/migration/page_cache.c
+++ b/migration/page_cache.c
@@ -40,18 +40,17 @@ struct CacheItem {
 struct PageCache {
 CacheItem *page_cache;
 unsigned int page_size;
-int64_t max_num_items;
+uint64_t max_num_items;
 uint64_t max_item_age;
-int64_t num_items;
+uint64_t num_items;
 };
 
-PageCache *cache_init(int64_t num_pages, unsigned int page_size)
+PageCache *cache_init(uint64_t num_pages, unsigned page_size)
 {
-int64_t i;
-
+uint64_t i;
 PageCache *cache;
 
-if (num_pages <= 0) {
+if (!num_pages) {
 DPRINTF("invalid number of 

[Qemu-block] [RFC PATCH 29/56] block: Make BlockDirtyInfo byte count unsigned in QAPI/QMP

[Markus Armbruster]

Byte counts should use QAPI type 'size' (uint64_t).  BlockDirtyInfo
member @count is 'int' (int64_t).  bdrv_query_dirty_bitmaps() computes
@count from bdrv_get_dirty_count() in uint64_t, then implicitly
converts to int64_t.  Before the commit before previous, the
conversion was in bdrv_get_dirty_count() instead.

Change member @count to 'size'.

query-block now reports @count values above 2^63-1 correctly instead
of their (negative) two's complement.

Signed-off-by: Markus Armbruster 
---
 qapi/block-core.json | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1c4a08d..60e1b6f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -418,7 +418,7 @@
 # Since: 1.3
 ##
 { 'struct': 'BlockDirtyInfo',
-  'data': {'*name': 'str', 'count': 'int', 'granularity': 'size',
+  'data': {'*name': 'str', 'count': 'size', 'granularity': 'size',
'status': 'DirtyBitmapStatus'} }
 
 ##
-- 
2.7.5

[Qemu-block] [RFC PATCH 21/56] block: Clean up get_human_readable_size()

[Markus Armbruster]

get_human_readable_size() formats all negative numbers as if they were
small.

The previous two commits changed all callers to pass unsigned
arguments.  Change the parameter type to from int64_t to uint64_t.

Also change the buffer size parameter from int (ahem!) to size_t.

Signed-off-by: Markus Armbruster 
---
 block/qapi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index 1c6123c..0f6620e 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -566,10 +566,11 @@ BlockStatsList *qmp_query_blockstats(bool has_query_nodes,
 
 #define NB_SUFFIXES 4
 
-static char *get_human_readable_size(char *buf, int buf_size, int64_t size)
+static char *get_human_readable_size(char *buf, size_t buf_size,
+ uint64_t size)
 {
 static const char suffixes[NB_SUFFIXES] = {'K', 'M', 'G', 'T'};
-int64_t base;
+uint64_t base;
 int i;
 
 if (size <= 999) {
-- 
2.7.5

[Qemu-block] [RFC PATCH 36/56] blockjob: Drop unused parameter @errp of method set_speed()

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 block/backup.c   | 2 +-
 block/commit.c   | 2 +-
 block/mirror.c   | 2 +-
 block/stream.c   | 2 +-
 blockjob.c   | 9 +
 include/block/blockjob_int.h | 2 +-
 6 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index b76143d..359e526 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -188,7 +188,7 @@ static int coroutine_fn backup_before_write_notify(
 return backup_do_cow(job, req->offset, req->bytes, NULL, true);
 }
 
-static void backup_set_speed(BlockJob *job, uint64_t speed, Error **errp)
+static void backup_set_speed(BlockJob *job, uint64_t speed)
 {
 BackupBlockJob *s = container_of(job, BackupBlockJob, common);
 
diff --git a/block/commit.c b/block/commit.c
index 5dc1c73..ae9191d 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -220,7 +220,7 @@ out:
 block_job_defer_to_main_loop(>common, commit_complete, data);
 }
 
-static void commit_set_speed(BlockJob *job, uint64_t speed, Error **errp)
+static void commit_set_speed(BlockJob *job, uint64_t speed)
 {
 CommitBlockJob *s = container_of(job, CommitBlockJob, common);
 
diff --git a/block/mirror.c b/block/mirror.c
index 7959a7f..6c3b446 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -928,7 +928,7 @@ immediate_exit:
 block_job_defer_to_main_loop(>common, mirror_exit, data);
 }
 
-static void mirror_set_speed(BlockJob *job, uint64_t speed, Error **errp)
+static void mirror_set_speed(BlockJob *job, uint64_t speed)
 {
 MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
 
diff --git a/block/stream.c b/block/stream.c
index 11b6673..9a145f2 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -207,7 +207,7 @@ out:
 block_job_defer_to_main_loop(>common, stream_complete, data);
 }
 
-static void stream_set_speed(BlockJob *job, uint64_t speed, Error **errp)
+static void stream_set_speed(BlockJob *job, uint64_t speed)
 {
 StreamBlockJob *s = container_of(job, StreamBlockJob, common);
 
diff --git a/blockjob.c b/blockjob.c
index 7f77b7e..e653eef 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -454,8 +454,6 @@ static void block_job_completed_txn_success(BlockJob *job)
 
 void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
 {
-Error *local_err = NULL;
-
 if (!job->driver->set_speed) {
 error_setg(errp, QERR_UNSUPPORTED);
 return;
@@ -467,12 +465,7 @@ void block_job_set_speed(BlockJob *job, int64_t speed, 
Error **errp)
 return;
 }
 
-job->driver->set_speed(job, speed, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-return;
-}
-
+job->driver->set_speed(job, speed);
 job->speed = speed;
 }
 
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index 2d10794..dadfd8c 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -42,7 +42,7 @@ struct BlockJobDriver {
 BlockJobType job_type;
 
 /** Optional callback for job types that support setting a speed limit */
-void (*set_speed)(BlockJob *job, uint64_t speed, Error **errp);
+void (*set_speed)(BlockJob *job, uint64_t speed);
 
 /** Mandatory: Entrypoint for the Coroutine. */
 CoroutineEntry *start;
-- 
2.7.5

[Qemu-block] [RFC PATCH 08/56] dump: Make sizes and addresses unsigned in QAPI/QMP

[Markus Armbruster]

Sizes, virtual and physical addresses should use QAPI type 'size'
(uint64_t).  dump-guest-memory parameters @begin, @length are 'int'
(int64_t).  They get implicitly converted to unsigned types somewhere
down in the bowels of the dump machinery.  DumpQueryResult members
@completed and @total are also 'int', and also implicitly converted.

Change these dump-guest-memory parameters and DumpQueryResult members
to 'size'.

dump-guest-memory now accepts size and address values between 2^63 and
2^64-1.  They accept negative values as before, because that's how the
QObject input visitor works for backward compatibility.

query-dump and DUMP_COMPLETED now report sizes above 2^63-1 correctly
instead of their (negative) two's complement.

So does HMP's "info dump".

Drop a few redundant initializers and an incorrect, disabled debug
print while there.

Parameters of HMP's dump-guest-memory remain uint32_t, as HMP
args_type strings can't do uint64_t byte counts: 'l' is signed, and
'o' multiplies by 2^20.

Signed-off-by: Markus Armbruster 
---
 dump.c  | 26 --
 hmp.c   |  2 +-
 include/sysemu/dump.h   |  8 
 include/sysemu/memory_mapping.h |  4 ++--
 memory_mapping.c|  4 ++--
 qapi-schema.json|  6 +++---
 6 files changed, 24 insertions(+), 26 deletions(-)

diff --git a/dump.c b/dump.c
index d9090a2..452f20f 100644
--- a/dump.c
+++ b/dump.c
@@ -331,7 +331,7 @@ static void write_elf_section(DumpState *s, int type, Error 
**errp)
 }
 }
 
-static void write_data(DumpState *s, void *buf, int length, Error **errp)
+static void write_data(DumpState *s, void *buf, size_t length, Error **errp)
 {
 int ret;
 
@@ -345,9 +345,9 @@ static void write_data(DumpState *s, void *buf, int length, 
Error **errp)
 
 /* write the memory to vmcore. 1 page per I/O. */
 static void write_memory(DumpState *s, GuestPhysBlock *block, ram_addr_t start,
- int64_t size, Error **errp)
+ uint64_t size, Error **errp)
 {
-int64_t i;
+uint64_t i;
 Error *local_err = NULL;
 
 for (i = 0; i < size / s->dump_info.page_size; i++) {
@@ -378,7 +378,7 @@ static void get_offset_range(hwaddr phys_addr,
 {
 GuestPhysBlock *block;
 hwaddr offset = s->memory_offset;
-int64_t size_in_block, start;
+uint64_t size_in_block, start;
 
 /* When the memory is not stored into vmcore, offset will be -1 */
 *p_offset = -1;
@@ -602,7 +602,7 @@ static int get_next_block(DumpState *s, GuestPhysBlock 
*block)
 static void dump_iterate(DumpState *s, Error **errp)
 {
 GuestPhysBlock *block;
-int64_t size;
+uint64_t size;
 Error *local_err = NULL;
 
 do {
@@ -1466,10 +1466,10 @@ bool dump_in_progress(void)
 
 /* calculate total size of memory to be dumped (taking filter into
  * acoount.) */
-static int64_t dump_calculate_size(DumpState *s)
+static uint64_t dump_calculate_size(DumpState *s)
 {
 GuestPhysBlock *block;
-int64_t size = 0, total = 0, left = 0, right = 0;
+uint64_t total = 0, size, left, right;
 
 QTAILQ_FOREACH(block, >guest_phys_blocks.head, next) {
 if (s->has_filter) {
@@ -1490,7 +1490,7 @@ static int64_t dump_calculate_size(DumpState *s)
 
 static void dump_init(DumpState *s, int fd, bool has_format,
   DumpGuestMemoryFormat format, bool paging, bool 
has_filter,
-  int64_t begin, int64_t length, Error **errp)
+  uint64_t begin, uint64_t length, Error **errp)
 {
 CPUState *cpu;
 int nr_cpus;
@@ -1532,9 +1532,6 @@ static void dump_init(DumpState *s, int fd, bool 
has_format,
 guest_phys_blocks_init(>guest_phys_blocks);
 guest_phys_blocks_append(>guest_phys_blocks);
 s->total_size = dump_calculate_size(s);
-#ifdef DEBUG_DUMP_GUEST_MEMORY
-fprintf(stderr, "DUMP: total memory to dump: %lu\n", s->total_size);
-#endif
 
 s->start = get_start_block(s);
 if (s->start == -1) {
@@ -1667,7 +1664,7 @@ cleanup:
 static void dump_process(DumpState *s, Error **errp)
 {
 Error *local_err = NULL;
-DumpQueryResult *result = NULL;
+DumpQueryResult *result;
 
 if (s->has_format && s->format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
 create_kdump_vmcore(s, _err);
@@ -1706,6 +1703,7 @@ DumpQueryResult *qmp_query_dump(Error **errp)
 {
 DumpQueryResult *result = g_new(DumpQueryResult, 1);
 DumpState *state = _state_global;
+
 result->status = atomic_read(>status);
 /* make sure we are reading status and written_size in order */
 smp_rmb();
@@ -1716,8 +1714,8 @@ DumpQueryResult *qmp_query_dump(Error **errp)
 
 void qmp_dump_guest_memory(bool paging, const char *file,
bool has_detach, bool detach,
-   bool has_begin, int64_t begin, bool has_length,
-   int64_t length, bool has_format,
+   

[Qemu-block] [RFC PATCH 18/56] migration: Make parameter max-bandwidth unsigned in QAPI/QMP

[Markus Armbruster]

Byte rates should use QAPI type 'size' (uint64_t).
migrate_set_speed's parameter @value and member @max-bandwidth of
MigrationParameters and MigrateSetParameters are 'int' (int64_t).

Change them all to 'size'.

migrate_set_speed and migrate-set-parameters now accept bandwidth
values between 2^63 and SIZE_MAX (commonly 2^64-1).  They accept
negative values as before, because that's how the QObject input
visitor works for backward compatibility.

So does HMP's migrate_set_speed, except it continues to reject
negative values.

query-migrate-parameters now reports bandwidth values above 2^63-1
correctly instead of their (negative) two's complement.

So does HMP's "info migrate_params".

Signed-off-by: Markus Armbruster 
---
 hmp.c | 2 +-
 migration/migration.c | 9 -
 qapi-schema.json  | 6 +++---
 3 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/hmp.c b/hmp.c
index 184fb8b..9bcdcb3 100644
--- a/hmp.c
+++ b/hmp.c
@@ -322,7 +322,7 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict 
*qdict)
 MigrationParameter_lookup[MIGRATION_PARAMETER_TLS_HOSTNAME],
 params->tls_hostname);
 assert(params->has_max_bandwidth);
-monitor_printf(mon, "%s: %" PRId64 " bytes/second\n",
+monitor_printf(mon, "%s: %" PRIu64 " bytes/second\n",
 MigrationParameter_lookup[MIGRATION_PARAMETER_MAX_BANDWIDTH],
 params->max_bandwidth);
 assert(params->has_downtime_limit);
diff --git a/migration/migration.c b/migration/migration.c
index 2d7f3a2..0b47371 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -716,8 +716,7 @@ static bool migrate_params_check(MigrationParameters 
*params, Error **errp)
 return false;
 }
 
-if (params->has_max_bandwidth &&
-(params->max_bandwidth < 0 || params->max_bandwidth > SIZE_MAX)) {
+if (params->has_max_bandwidth && params->max_bandwidth > SIZE_MAX) {
 error_setg(errp, "Parameter 'max_bandwidth' expects an integer in the"
  " range of 0 to %zu bytes/second", SIZE_MAX);
 return false;
@@ -1311,7 +1310,7 @@ uint64_t qmp_query_migrate_cache_size(Error **errp)
 return migrate_xbzrle_cache_size();
 }
 
-void qmp_migrate_set_speed(int64_t value, Error **errp)
+void qmp_migrate_set_speed(uint64_t value, Error **errp)
 {
 MigrateSetParameters p = {
 .has_max_bandwidth = true,
@@ -2179,8 +2178,8 @@ static Property migration_properties[] = {
 DEFINE_PROP_INT64("x-cpu-throttle-increment", MigrationState,
   parameters.cpu_throttle_increment,
   DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT),
-DEFINE_PROP_INT64("x-max-bandwidth", MigrationState,
-  parameters.max_bandwidth, MAX_THROTTLE),
+DEFINE_PROP_UINT64("x-max-bandwidth", MigrationState,
+   parameters.max_bandwidth, MAX_THROTTLE),
 DEFINE_PROP_INT64("x-downtime-limit", MigrationState,
   parameters.downtime_limit,
   DEFAULT_MIGRATE_SET_DOWNTIME),
diff --git a/qapi-schema.json b/qapi-schema.json
index 2eee676..c18e574 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1116,7 +1116,7 @@
 '*cpu-throttle-increment': 'int',
 '*tls-creds': 'StrOrNull',
 '*tls-hostname': 'StrOrNull',
-'*max-bandwidth': 'int',
+'*max-bandwidth': 'size',
 '*downtime-limit': 'int',
 '*x-checkpoint-delay': 'int',
 '*block-incremental': 'bool' } }
@@ -1200,7 +1200,7 @@
 '*cpu-throttle-increment': 'int',
 '*tls-creds': 'str',
 '*tls-hostname': 'str',
-'*max-bandwidth': 'int',
+'*max-bandwidth': 'size',
 '*downtime-limit': 'int',
 '*x-checkpoint-delay': 'int',
 '*block-incremental': 'bool' } }
@@ -2852,7 +2852,7 @@
 # <- { "return": {} }
 #
 ##
-{ 'command': 'migrate_set_speed', 'data': {'value': 'int'} }
+{ 'command': 'migrate_set_speed', 'data': {'value': 'size'} }
 
 ##
 # @migrate-set-cache-size:
-- 
2.7.5

[Qemu-block] [RFC PATCH 07/56] cpus: Make memsave, pmemsave sizes, addresses unsigned in QAPI/QMP

[Markus Armbruster]

Sizes, virtual and physical addresses should use QAPI type 'size'
(uint64_t).  memsave, pmemsave parameters @val, @size are 'int'
(int64_t).  qmp_memsave() and qmp_pmemsave() implicitly convert to
target_ulong or hwaddr.

Change the parameters to 'size'.

Both commands now accept size and address values between 2^63 and
2^64-1.  They accept negative values as before, because that's how the
QObject input visitor works for backward compatibility.

The HMP commands' size parameters remain uint32_t, as HMP args_type
strings can't do uint64_t byte counts: 'l' is signed, and 'o'
multiplies by 2^20.  Their address parameters remain int64_t for the
same reason.

Signed-off-by: Markus Armbruster 
---
 cpus.c   | 6 +++---
 qapi-schema.json | 5 +++--
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/cpus.c b/cpus.c
index 9bed61e..8c5ee05 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1947,14 +1947,14 @@ CpuInfoList *qmp_query_cpus(Error **errp)
 return head;
 }
 
-void qmp_memsave(int64_t addr, int64_t size, const char *filename,
+void qmp_memsave(uint64_t addr, uint64_t size, const char *filename,
  bool has_cpu, int64_t cpu_index, Error **errp)
 {
 FILE *f;
 uint32_t l;
 CPUState *cpu;
 uint8_t buf[1024];
-int64_t orig_addr = addr, orig_size = size;
+uint64_t orig_addr = addr, orig_size = size;
 
 if (!has_cpu) {
 cpu_index = 0;
@@ -1994,7 +1994,7 @@ exit:
 fclose(f);
 }
 
-void qmp_pmemsave(int64_t addr, int64_t size, const char *filename,
+void qmp_pmemsave(uint64_t addr, uint64_t size, const char *filename,
   Error **errp)
 {
 FILE *f;
diff --git a/qapi-schema.json b/qapi-schema.json
index f4a71df..80458fa 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2460,7 +2460,8 @@
 #
 ##
 { 'command': 'memsave',
-  'data': {'val': 'int', 'size': 'int', 'filename': 'str', '*cpu-index': 
'int'} }
+  'data': {'val': 'size', 'size': 'size', 'filename': 'str',
+   '*cpu-index': 'int'} }
 
 ##
 # @pmemsave:
@@ -2489,7 +2490,7 @@
 #
 ##
 { 'command': 'pmemsave',
-  'data': {'val': 'int', 'size': 'int', 'filename': 'str'} }
+  'data': {'val': 'size', 'size': 'size', 'filename': 'str'} }
 
 ##
 # @cont:
-- 
2.7.5

[Qemu-block] [RFC PATCH 11/56] monitor: Drop unused HMP .args_type 'M'

[Markus Armbruster]

The previous commit switched balloon from 'M' to 'o', rendering 'M'
unused.  It was never used for anything else.  Drop it.

Signed-off-by: Markus Armbruster 
---
 monitor.c | 11 +--
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/monitor.c b/monitor.c
index 8b54ba1..3b2757e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -101,8 +101,6 @@
  * TYPEs that put an int64_t value with key NAME:
  * 'l'Argument is an expression (QEMU pocket calculator).
  * 'i'Like 'l' except value must fit into 32 bit unsigned.
- * 'M'Like 'l' except value must not be negative and is multiplied
- *by 2^20 (think "mebibyte").
  *
  * TYPEs that put an uint64_t value with key NAME:
  * 'o'Argument is a size (think "octets").  Without suffix the
@@ -134,7 +132,7 @@
  * '?'Argument is optional, nothing is put when it is absent
  *(all types except 'O', '/', 'b').
  * '.'Argument is optional, must be preceded by '.' if present
- *(only types 'i', 'l', 'M')
+ *(only types 'i', 'l')
  */
 
 typedef struct mon_cmd_t {
@@ -2913,7 +2911,6 @@ static QDict *monitor_parse_arguments(Monitor *mon,
 break;
 case 'i':
 case 'l':
-case 'M':
 {
 int64_t val;
 
@@ -2944,12 +2941,6 @@ static QDict *monitor_parse_arguments(Monitor *mon,
 monitor_printf(mon, "\'%s\' has failed: ", cmd->name);
 monitor_printf(mon, "integer is for 32-bit values\n");
 goto fail;
-} else if (c == 'M') {
-if (val < 0) {
-monitor_printf(mon, "enter a positive value\n");
-goto fail;
-}
-val <<= 20;
 }
 qdict_put_int(qdict, key, val);
 }
-- 
2.7.5

[Qemu-block] [RFC PATCH 22/56] block: Mix up signed and unsigned less in bdrv_img_create()

[Markus Armbruster]

@size is declared int64_t.  It's set in two places.

The second one assigns the (signed) value of bdrv_getlength(), then
errors out if its negative.

The first one assigns qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0),
i.e. an uint64_t value.  What if it exceeds INT64_MAX?  Is that even
possible?  Turns out it is:

$ qemu-img create -o size=9223372036854775808 foo.img

On closer examination, the code still works as long as converting from
uint64_t to int64_t and back doesn't change the value.
Implementation-defined behavior, but sane implementations behave.
Things actually break elsewhere for such sizes, e.g. file-posix.c's
raw_create().

Clean this up.

Signed-off-by: Markus Armbruster 
---
 block.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index ce9cce7..04cce0d 100644
--- a/block.c
+++ b/block.c
@@ -4309,7 +4309,8 @@ void bdrv_img_create(const char *filename, const char 
*fmt,
 QemuOptsList *create_opts = NULL;
 QemuOpts *opts = NULL;
 const char *backing_fmt, *backing_file;
-int64_t size;
+uint64_t size;
+int64_t backing_size;
 BlockDriver *drv, *proto_drv;
 Error *local_err = NULL;
 int ret = 0;
@@ -4414,7 +4415,7 @@ void bdrv_img_create(const char *filename, const char 
*fmt,
 bs = bdrv_open(full_backing, NULL, backing_options, back_flags,
_err);
 g_free(full_backing);
-if (!bs && size != -1) {
+if (!bs && size != UINT64_MAX) {
 /* Couldn't open BS, but we have a size, so it's nonfatal */
 warn_reportf_err(local_err,
 "Could not verify backing image. "
@@ -4426,22 +4427,24 @@ void bdrv_img_create(const char *filename, const char 
*fmt,
   "Could not open backing image to determine 
size.\n");
 goto out;
 } else {
-if (size == -1) {
+if (size == UINT64_MAX) {
 /* Opened BS, have no size */
-size = bdrv_getlength(bs);
-if (size < 0) {
-error_setg_errno(errp, -size, "Could not get size of '%s'",
+backing_size = bdrv_getlength(bs);
+if (backing_size < 0) {
+error_setg_errno(errp, -backing_size,
+ "Could not get size of '%s'",
  backing_file);
 bdrv_unref(bs);
 goto out;
 }
+size = backing_size;
 qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size, _abort);
 }
 bdrv_unref(bs);
 }
 } /* (backing_file && !(flags & BDRV_O_NO_BACKING)) */
 
-if (size == -1) {
+if (size == UINT64_MAX) {
 error_setg(errp, "Image creation needs a size parameter");
 goto out;
 }
-- 
2.7.5

[Qemu-block] [RFC PATCH 01/56] qobject: Touch up comments to say @param instead of 'param'

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 qobject/qdict.c | 68 -
 qobject/qlist.c |  2 +-
 2 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/qobject/qdict.c b/qobject/qdict.c
index 576018e..d795079 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -116,13 +116,13 @@ static QDictEntry *qdict_find(const QDict *qdict,
 /**
  * qdict_put_obj(): Put a new QObject into the dictionary
  *
- * Insert the pair 'key:value' into 'qdict', if 'key' already exists
- * its 'value' will be replaced.
+ * Insert the pair @key:@value into @qdict, if @key already exists
+ * its value will be replaced.
  *
  * This is done by freeing the reference to the stored QObject and
  * storing the new one in the same entry.
  *
- * NOTE: ownership of 'value' is transferred to the QDict
+ * NOTE: ownership of @value is transferred to the QDict
  */
 void qdict_put_obj(QDict *qdict, const char *key, QObject *value)
 {
@@ -144,10 +144,10 @@ void qdict_put_obj(QDict *qdict, const char *key, QObject 
*value)
 }
 
 /**
- * qdict_get(): Lookup for a given 'key'
+ * qdict_get(): Lookup for a given @key
  *
- * Return a weak reference to the QObject associated with 'key' if
- * 'key' is present in the dictionary, NULL otherwise.
+ * Return a weak reference to the QObject associated with @key if
+ * @key is present in the dictionary, NULL otherwise.
  */
 QObject *qdict_get(const QDict *qdict, const char *key)
 {
@@ -158,9 +158,9 @@ QObject *qdict_get(const QDict *qdict, const char *key)
 }
 
 /**
- * qdict_haskey(): Check if 'key' exists
+ * qdict_haskey(): Check if @key exists
  *
- * Return 1 if 'key' exists in the dict, 0 otherwise
+ * Return 1 if @key exists in the dict, 0 otherwise
  */
 int qdict_haskey(const QDict *qdict, const char *key)
 {
@@ -177,11 +177,11 @@ size_t qdict_size(const QDict *qdict)
 }
 
 /**
- * qdict_get_double(): Get an number mapped by 'key'
+ * qdict_get_double(): Get an number mapped by @key
  *
- * This function assumes that 'key' exists and it stores a QNum.
+ * This function assumes that @key exists and it stores a QNum.
  *
- * Return number mapped by 'key'.
+ * Return number mapped by @key.
  */
 double qdict_get_double(const QDict *qdict, const char *key)
 {
@@ -189,12 +189,12 @@ double qdict_get_double(const QDict *qdict, const char 
*key)
 }
 
 /**
- * qdict_get_int(): Get an integer mapped by 'key'
+ * qdict_get_int(): Get an integer mapped by @key
  *
- * This function assumes that 'key' exists and it stores a
+ * This function assumes that @key exists and it stores a
  * QNum representable as int.
  *
- * Return integer mapped by 'key'.
+ * Return integer mapped by @key.
  */
 int64_t qdict_get_int(const QDict *qdict, const char *key)
 {
@@ -202,12 +202,12 @@ int64_t qdict_get_int(const QDict *qdict, const char *key)
 }
 
 /**
- * qdict_get_bool(): Get a bool mapped by 'key'
+ * qdict_get_bool(): Get a bool mapped by @key
  *
- * This function assumes that 'key' exists and it stores a
+ * This function assumes that @key exists and it stores a
  * QBool object.
  *
- * Return bool mapped by 'key'.
+ * Return bool mapped by @key.
  */
 bool qdict_get_bool(const QDict *qdict, const char *key)
 {
@@ -232,12 +232,12 @@ QDict *qdict_get_qdict(const QDict *qdict, const char 
*key)
 
 /**
  * qdict_get_str(): Get a pointer to the stored string mapped
- * by 'key'
+ * by @key
  *
- * This function assumes that 'key' exists and it stores a
+ * This function assumes that @key exists and it stores a
  * QString object.
  *
- * Return pointer to the string mapped by 'key'.
+ * Return pointer to the string mapped by @key.
  */
 const char *qdict_get_str(const QDict *qdict, const char *key)
 {
@@ -245,11 +245,11 @@ const char *qdict_get_str(const QDict *qdict, const char 
*key)
 }
 
 /**
- * qdict_get_try_int(): Try to get integer mapped by 'key'
+ * qdict_get_try_int(): Try to get integer mapped by @key
  *
- * Return integer mapped by 'key', if it is not present in the
+ * Return integer mapped by @key, if it is not present in the
  * dictionary or if the stored object is not a QNum representing an
- * integer, 'def_value' will be returned.
+ * integer, @def_value will be returned.
  */
 int64_t qdict_get_try_int(const QDict *qdict, const char *key,
   int64_t def_value)
@@ -265,11 +265,11 @@ int64_t qdict_get_try_int(const QDict *qdict, const char 
*key,
 }
 
 /**
- * qdict_get_try_bool(): Try to get a bool mapped by 'key'
+ * qdict_get_try_bool(): Try to get a bool mapped by @key
  *
- * Return bool mapped by 'key', if it is not present in the
+ * Return bool mapped by @key, if it is not present in the
  * dictionary or if the stored object is not of QBool type
- * 'def_value' will be returned.
+ * @def_value will be returned.
  */
 bool qdict_get_try_bool(const QDict *qdict, const char *key, bool def_value)
 {
@@ -280,9 +280,9 @@ bool qdict_get_try_bool(const QDict *qdict, const char 
*key, bool 

[Qemu-block] [RFC PATCH 03/56] monitor: Rewrite comment describing HMP .args_type

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 monitor.c | 75 +++
 1 file changed, 47 insertions(+), 28 deletions(-)

diff --git a/monitor.c b/monitor.c
index e0f8801..8b54ba1 100644
--- a/monitor.c
+++ b/monitor.c
@@ -85,37 +85,56 @@
 #endif
 
 /*
- * Supported types:
+ * Command handlers (mon_cmd_t member @cmd) receive actual arguments
+ * in a QDict, which is built by the HMP core according to mon_cmd_t
+ * member @args_type.  It's a list of NAME:TYPE separated by comma.
  *
- * 'F'  filename
- * 'B'  block device name
- * 's'  string (accept optional quote)
- * 'S'  it just appends the rest of the string (accept optional quote)
- * 'O'  option string of the form NAME=VALUE,...
- *  parsed according to QemuOptsList given by its name
- *  Example: 'device:O' uses qemu_device_opts.
- *  Restriction: only lists with empty desc are supported
- *  TODO lift the restriction
- * 'i'  32 bit integer
- * 'l'  target long (32 or 64 bit)
- * 'M'  Non-negative target long (32 or 64 bit), in user mode the
- *  value is multiplied by 2^20 (think Mebibyte)
- * 'o'  octets (aka bytes)
- *  user mode accepts an optional E, e, P, p, T, t, G, g, M, m,
- *  K, k suffix, which multiplies the value by 2^60 for suffixes E
- *  and e, 2^50 for suffixes P and p, 2^40 for suffixes T and t,
- *  2^30 for suffixes G and g, 2^20 for M and m, 2^10 for K and k
- * 'T'  double
- *  user mode accepts an optional ms, us, ns suffix,
- *  which divides the value by 1e3, 1e6, 1e9, respectively
- * '/'  optional gdb-like print format (like "/10x")
+ * TYPEs that put a string value with key NAME into the QDict:
+ * 's'Argument is enclosed in '"' or delimited by whitespace.  In
+ *the former case, escapes \n \r \\ \' and \" are recognized.
+ * 'F'File name, like 's' except for completion.
+ * 'B'BlockBackend name, like 's' except for completion.
+ * 'S'Argument is the remainder of the line, less leading
+ *whitespace.
+
  *
- * '?'  optional type (for all types, except '/')
- * '.'  other form of optional type (for 'i' and 'l')
- * 'b'  boolean
- *  user mode accepts "on" or "off"
- * '-'  optional parameter (eg. '-f')
+ * TYPEs that put an int64_t value with key NAME:
+ * 'l'Argument is an expression (QEMU pocket calculator).
+ * 'i'Like 'l' except value must fit into 32 bit unsigned.
+ * 'M'Like 'l' except value must not be negative and is multiplied
+ *by 2^20 (think "mebibyte").
  *
+ * TYPEs that put an uint64_t value with key NAME:
+ * 'o'Argument is a size (think "octets").  Without suffix the
+ *value is multiplied by 2^20 (mebibytes), with suffix E or e
+ *by 2^60 (exbibytes), with P or p by 2^50 (pebibytes), with T
+ *or t by 2^40 (tebibytes), with G or g by 2^30 (gibibytes),
+ *with M or m by 2^10 (mebibytes), with K or k by 2^10
+ *(kibibytes).
+ *
+ * TYPEs that put a double value with key NAME:
+ * 'T'Argument is a time in seconds.  With optional ms, us, ns
+ *suffix, the value divided by 1e3, 1e6, 1e9 respectively.
+ *
+ * TYPEs that put a bool value with key NAME:
+ * 'b'Argument is either "on" (true) or "off" (false).
+ * '-' CHAR
+ *Argument is either "-CHAR" (true) or absent (false).
+ *
+ * TYPEs that put multiple values:
+ * 'O'Option string of the form NAME=VALUE,... parsed according to
+ *the QemuOptsList given by its name.
+ *Example: 'device:O' uses qemu_device_opts.
+ *Restriction: only lists with empty desc are supported.
+ *Puts all the NAME=VALUE.
+ * '/'Gdb-like print format (like "/10x"), always optional.
+ *Puts keys "count", "format", "size", all int.
+ *
+ * Modifier character following the type string:
+ * '?'Argument is optional, nothing is put when it is absent
+ *(all types except 'O', '/', 'b').
+ * '.'Argument is optional, must be preceded by '.' if present
+ *(only types 'i', 'l', 'M')
  */
 
 typedef struct mon_cmd_t {
-- 
2.7.5

[Qemu-block] [RFC PATCH 16/56] migration: Make XBZRLE transferred size unsigned in QAPI/QMP

[Markus Armbruster]

Sizes should use QAPI type 'size' (uint64_t).  XBZRLECacheStats member
@bytes is 'int' (int64_t).  save_xbzrle_page() computes the byte count
increment in size_t, implicitly converts it to int, then adds that to
@bytes.

Change the XBZRLECacheStats member to 'size' and clean up
save_xbzrle_page().

query-migrate now reports transferred sizes above 2^63-1 correctly
instead of their (negative) two's complement.

HMP's "info migrate" already reported them correctly, because it
printed the signed integer with PRIu64.

Signed-off-by: Markus Armbruster 
---
 migration/ram.c  | 3 ++-
 qapi-schema.json | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index ce38be4..5c247f8 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -461,7 +461,8 @@ static int save_xbzrle_page(RAMState *rs, uint8_t 
**current_data,
 ram_addr_t current_addr, RAMBlock *block,
 ram_addr_t offset, bool last_stage)
 {
-int encoded_len = 0, bytes_xbzrle;
+int encoded_len;
+size_t bytes_xbzrle;
 uint8_t *prev_cached_page;
 
 if (!cache_is_cached(XBZRLE.cache, current_addr,
diff --git a/qapi-schema.json b/qapi-schema.json
index ecabff6..4a3d07e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -646,7 +646,7 @@
 # Since: 1.2
 ##
 { 'struct': 'XBZRLECacheStats',
-  'data': {'cache-size': 'size', 'bytes': 'int', 'pages': 'int',
+  'data': {'cache-size': 'size', 'bytes': 'size', 'pages': 'int',
'cache-miss': 'int', 'cache-miss-rate': 'number',
'overflow': 'int' } }
 
-- 
2.7.5

[Qemu-block] [RFC PATCH 02/56] qdict: New helpers to put and get unsigned integers

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 include/qapi/qmp/qdict.h |  5 +
 qobject/qdict.c  | 43 ---
 2 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 363e431..3b52481 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -56,6 +56,8 @@ void qdict_destroy_obj(QObject *obj);
 /* Helpers for int, bool, and string */
 #define qdict_put_int(qdict, key, value) \
 qdict_put(qdict, key, qnum_from_int(value))
+#define qdict_put_uint(qdict, key, value) \
+qdict_put(qdict, key, qnum_from_uint(value))
 #define qdict_put_bool(qdict, key, value) \
 qdict_put(qdict, key, qbool_from_bool(value))
 #define qdict_put_str(qdict, key, value) \
@@ -64,12 +66,15 @@ void qdict_destroy_obj(QObject *obj);
 /* High level helpers */
 double qdict_get_double(const QDict *qdict, const char *key);
 int64_t qdict_get_int(const QDict *qdict, const char *key);
+uint64_t qdict_get_uint(const QDict *qdict, const char *key);
 bool qdict_get_bool(const QDict *qdict, const char *key);
 QList *qdict_get_qlist(const QDict *qdict, const char *key);
 QDict *qdict_get_qdict(const QDict *qdict, const char *key);
 const char *qdict_get_str(const QDict *qdict, const char *key);
 int64_t qdict_get_try_int(const QDict *qdict, const char *key,
   int64_t def_value);
+uint64_t qdict_get_try_uint(const QDict *qdict, const char *key,
+uint64_t def_value);
 bool qdict_get_try_bool(const QDict *qdict, const char *key, bool def_value);
 const char *qdict_get_try_str(const QDict *qdict, const char *key);
 
diff --git a/qobject/qdict.c b/qobject/qdict.c
index d795079..be919b9 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -189,10 +189,9 @@ double qdict_get_double(const QDict *qdict, const char 
*key)
 }
 
 /**
- * qdict_get_int(): Get an integer mapped by @key
+ * qdict_get_int(): Get a signed integer mapped by @key
  *
- * This function assumes that @key exists and it stores a
- * QNum representable as int.
+ * @qdict must map @key to an integer QNum that fits into int64_t.
  *
  * Return integer mapped by @key.
  */
@@ -202,6 +201,18 @@ int64_t qdict_get_int(const QDict *qdict, const char *key)
 }
 
 /**
+ * qdict_get_uint(): Get an unsigned integer mapped by 'key'
+ *
+ * @qdict must map @key to an integer QNum that fits into uint64_t.
+ *
+ * Return integer mapped by 'key'.
+ */
+uint64_t qdict_get_uint(const QDict *qdict, const char *key)
+{
+return qnum_get_uint(qobject_to_qnum(qdict_get(qdict, key)));
+}
+
+/**
  * qdict_get_bool(): Get a bool mapped by @key
  *
  * This function assumes that @key exists and it stores a
@@ -245,11 +256,10 @@ const char *qdict_get_str(const QDict *qdict, const char 
*key)
 }
 
 /**
- * qdict_get_try_int(): Try to get integer mapped by @key
+ * qdict_get_try_int(): Try to get signed integer mapped by @key
  *
- * Return integer mapped by @key, if it is not present in the
- * dictionary or if the stored object is not a QNum representing an
- * integer, @def_value will be returned.
+ * If @qdict maps @key to an integer QNum that fits into int64_t,
+ * return it.  Else return @def_value.
  */
 int64_t qdict_get_try_int(const QDict *qdict, const char *key,
   int64_t def_value)
@@ -265,6 +275,25 @@ int64_t qdict_get_try_int(const QDict *qdict, const char 
*key,
 }
 
 /**
+ * qdict_get_try_uint(): Try to get unsigned integer mapped by 'key'
+ *
+ * If @qdict maps @key to an integer QNum that fits into uint64_t,
+ * return it.  Else return @def_value.
+ */
+uint64_t qdict_get_try_uint(const QDict *qdict, const char *key,
+uint64_t def_value)
+{
+QNum *qnum = qobject_to_qnum(qdict_get(qdict, key));
+uint64_t val;
+
+if (!qnum || !qnum_get_try_uint(qnum, )) {
+return def_value;
+}
+
+return val;
+}
+
+/**
  * qdict_get_try_bool(): Try to get a bool mapped by @key
  *
  * Return bool mapped by @key, if it is not present in the
-- 
2.7.5

[Qemu-block] [RFC PATCH 04/56] char: Make ringbuf-read size unsigned in QAPI/QMP

[Markus Armbruster]

Sizes should use QAPI type 'size' (uint64_t).  ringbuf-read parameter
@size is 'int' (int64_t).  qmp_ringbuf_read() rejects negative values,
then implicitly converts to size_t.

Change the parameter to 'size' and drop the check for negative values.

ringbuf-read now accepts size values between 2^63 and 2^64-1.  It
accepts negative values as before, because that's how the QObject
input visitor works for backward compatibility.

The HMP command's size parameter remains uint32_t, as HMP args_type
strings can't do uint64_t byte counts: 'l' is signed, and 'o'
multiplies by 2^20.

Signed-off-by: Markus Armbruster 
---
 chardev/char-ringbuf.c | 11 +++
 qapi-schema.json   |  2 +-
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/chardev/char-ringbuf.c b/chardev/char-ringbuf.c
index df52b04..a9205ea 100644
--- a/chardev/char-ringbuf.c
+++ b/chardev/char-ringbuf.c
@@ -65,10 +65,10 @@ static int ringbuf_chr_write(Chardev *chr, const uint8_t 
*buf, int len)
 return len;
 }
 
-static int ringbuf_chr_read(Chardev *chr, uint8_t *buf, int len)
+static int ringbuf_chr_read(Chardev *chr, uint8_t *buf, size_t len)
 {
 RingBufChardev *d = RINGBUF_CHARDEV(chr);
-int i;
+size_t i;
 
 qemu_mutex_lock(>chr_write_lock);
 for (i = 0; i < len && d->cons != d->prod; i++) {
@@ -151,7 +151,7 @@ void qmp_ringbuf_write(const char *device, const char *data,
 }
 }
 
-char *qmp_ringbuf_read(const char *device, int64_t size,
+char *qmp_ringbuf_read(const char *device, uint64_t size,
bool has_format, enum DataFormat format,
Error **errp)
 {
@@ -171,11 +171,6 @@ char *qmp_ringbuf_read(const char *device, int64_t size,
 return NULL;
 }
 
-if (size <= 0) {
-error_setg(errp, "size must be greater than zero");
-return NULL;
-}
-
 count = ringbuf_count(chr);
 size = size > count ? count : size;
 read_data = g_malloc(size + 1);
diff --git a/qapi-schema.json b/qapi-schema.json
index febe70e..18ec301 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -543,7 +543,7 @@
 #
 ##
 { 'command': 'ringbuf-read',
-  'data': {'device': 'str', 'size': 'int', '*format': 'DataFormat'},
+  'data': {'device': 'str', 'size': 'size', '*format': 'DataFormat'},
   'returns': 'str' }
 
 ##
-- 
2.7.5

[Qemu-block] [RFC PATCH 17/56] migration: Make MigrationStats sizes unsigned in QAPI/QMP

[Markus Armbruster]

Sizes should use QAPI type 'size' (uint64_t).  MigrationStats members
@transferred, @remaining, @total, @normal-bytes, @page-size are 'int'
(int64_t).  populate_ram_info(), populate_disk_info() and and many
places that update them in global variable @ram_counters implicitly
convert from unsigned types.

Change these MigrationStats members to 'size'.

query-migrate now reports them correctly above 2^63-1 instead of their
(negative) two's complement.

HMP's "info migrate" already reported them correctly, because it
printed the signed integer with PRIu64.

Signed-off-by: Markus Armbruster 
---
 qapi-schema.json | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 4a3d07e..2eee676 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -620,11 +620,11 @@
 # Since: 0.14.0
 ##
 { 'struct': 'MigrationStats',
-  'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
+  'data': {'transferred': 'size', 'remaining': 'size', 'total': 'size' ,
'duplicate': 'int', 'skipped': 'int', 'normal': 'int',
-   'normal-bytes': 'int', 'dirty-pages-rate' : 'int',
+   'normal-bytes': 'size', 'dirty-pages-rate' : 'int',
'mbps' : 'number', 'dirty-sync-count' : 'int',
-   'postcopy-requests' : 'int', 'page-size' : 'int' } }
+   'postcopy-requests' : 'int', 'page-size' : 'size' } }
 
 ##
 # @XBZRLECacheStats:
-- 
2.7.5

[Qemu-block] [RFC PATCH 05/56] char: Make ringbuf size unsigned in QAPI

[Markus Armbruster]

Sizes should use QAPI type 'size' (uint64_t).  ChardevRingbuf member
@size is 'int' (int64_t).  Doesn't really matter, as its users
chardev-add and chardev-change manually reject sizes that aren't
powers of two.

Change the ChardevRingbuf member to 'size' anyway.

Signed-off-by: Markus Armbruster 
---
 qapi-schema.json | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 18ec301..f4a71df 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -5106,7 +5106,7 @@
 #
 # Since: 1.5
 ##
-{ 'struct': 'ChardevRingbuf', 'data': { '*size'  : 'int' },
+{ 'struct': 'ChardevRingbuf', 'data': { '*size'  : 'size' },
   'base': 'ChardevCommon' }
 
 ##
-- 
2.7.5

[Qemu-block] [RFC PATCH 00/56] qapi: Use 'size' for byte counts & offsets

[Markus Armbruster]

Byte sizes, offsets and the like should use QAPI type 'size'
(uint64_t).  This rule is more honored in the breach than in the
observance.  Fix the obvious offenders.

The series is RFC for at least two reasons:

1. It's only lightly tested.  Commit message claims like "FOO now
   works" haven't been verified, yet.

2. The block layer represents file offsets and sizes as int64_t in
   many places.  Must not be negative, except for function return
   values, where it means failure.

   If you pass negative values via QMP, things explode.  Rejecting
   them cleanly would be better, but we do that only haphazardly, as
   far as I can tell.

   Changing the QAPI schema from 'int' (C: int64_t) to 'size' (C:
   uint64_t) arguably makes things slightly worse: you can't
   reasonably expect negative offsets and sizes to work, but expecting
   offsets and sizes between 2^63 and 2^64-1 to work is less
   unreasonable.  The explosions stay the same.

   Perhaps we should have a dedicated QAPI type for file offsets, just
   like libc has off_t.  Which is also signed, by the way.

Based on "[PATCH v2 0/3] Proactive pow2floor() fix, and dead code
removal" and "[PATCH 0/3] Don't QAPI without need".

Markus Armbruster (56):
  qobject: Touch up comments to say @param instead of 'param'
  qdict: New helpers to put and get unsigned integers
  monitor: Rewrite comment describing HMP .args_type
  char: Make ringbuf-read size unsigned in QAPI/QMP
  char: Make ringbuf size unsigned in QAPI
  char: Don't truncate -chardev and HMP chardev-add ringbuf size
  cpus: Make memsave, pmemsave sizes, addresses unsigned in QAPI/QMP
  dump: Make sizes and addresses unsigned in QAPI/QMP
  balloon: Make balloon size unsigned in QAPI/QMP
  hmp: Make balloon's argument unsigned
  monitor: Drop unused HMP .args_type 'M'
  pc-dimm: Make size and address unsigned in QAPI/QMP
  pci: Make PCI addresses and sizes unsigned in QAPI/QMP
  migration: Fix migrate-set-cache-size error reporting
  migration: Make XBZRLE cache size unsigned in QAPI/QMP
  migration: Make XBZRLE transferred size unsigned in QAPI/QMP
  migration: Make MigrationStats sizes unsigned in QAPI/QMP
  migration: Make parameter max-bandwidth unsigned in QAPI/QMP
  block: Make snapshot VM state size unsigned in QAPI/QMP
  block: Make ImageInfo sizes unsigned in QAPI/QMP
  block: Clean up get_human_readable_size()
  block: Mix up signed and unsigned less in bdrv_img_create()
  option: Fix type of qemu_opt_set_number() parameter @val
  block/qcow2: Change align_offset() to operate on uint64_t
  block/qcow2: Change qcow2_calc_prealloc_size() to uint64_t
  block: Make BlockMeasureInfo sizes unsigned in QAPI
  block/dirty-bitmap: Clean up signed vs. unsigned dirty counts
  block: Widen dirty bitmap granularity to uint64_t for safety
  block: Make BlockDirtyInfo byte count unsigned in QAPI/QMP
  block: Make write thresholds unsigned in QAPI/QMP
  block: Make throttle byte rates and sizes unsigned in QAPI/QMP
  hmp: Make block_set_io_throttle's arguments unsigned
  block: Make block_resize size unsigned in QAPI/QMP
  block: Make BlockDeviceStats sizes, offsets unsigned in QAPI/QMP
  blockjob: Lift speed sign conversion into block_job_set_speed()
  blockjob: Drop unused parameter @errp of method set_speed()
  blockjob: Make BlockJobInfo and event speed unsigned in QAPI/QMP
  blockjob: Lift speed sign conversion out of block_job_set_speed()
  blockjob: Lift speed sign conversion out of block_job_create()
  blockjob: Lift speed sign conversion out of backup_job_create()
  blockjob: Lift speed sign conversion out of mirror_start_job()
  blockjob: Lift speed sign conversion out of stream_start()
  blockjob: Lift speed sign conversion out of mirror_start()
  blockjob: Lift speed sign conversion out of blockdev_mirror_common()
  blockjob: Lift speed sign conversion out of commit_start() etc.
  blockjob: Make job commands' speed parameter unsigned in QAPI/QMP
  blockjob: Make BlockJobInfo and event offsets unsigned in QAPI/QMP
  block: Make mirror buffer size unsigned in QAPI/QMP
  block: Make ImageCheck file offset unsigned in QAPI
  block: Make BLOCK_IMAGE_CORRUPTED offset, size unsigned in QAPI/QMP
  block/nfs: Fix for readahead-size, page-cache-size > INT64_MAX
  block/nfs: Reject negative readahead-size, page-cache-size
  block: Make blockdev-add byte counts unsigned in QAPI/QMP
  qemu-img: blk_getlength() can fail, fix img_map() to check
  block: Make MapEntry offsets and size unsigned in QAPI
  crypto: Make QCryptoBlockInfoLUKS offsets unsigned in QAPI/QMP

 balloon.c   |   2 +-
 block.c |  19 ---
 block/backup.c  |  10 +---
 block/commit.c  |   9 +--
 block/dirty-bitmap.c|  23 +++-
 block/mirror.c  |  27 +++--
 block/nfs.c |  15 ++---
 block/null.c|   2 +-
 block/qapi.c|  20 ---
 block/qcow2-bitmap.c|  18 

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10] iotests: fix 185

[Eric Blake]

On 08/07/2017 09:16 AM, Vladimir Sementsov-Ogievskiy wrote:
> 185 iotest is broken.
> 
> How to test:
>> i=0; while ./check -qcow2 -nocache 185; do ((i+=1)); echo N = $i; \
>   done; echo N = $i
> 
> finished for me like this:
> 
> 185 2s ... - output mismatch (see 185.out.bad)
> --- /work/src/qemu/master/tests/qemu-iotests/185.out2017-07-14 \
> 15:14:29.520343805 +0300
> +++ 185.out.bad 2017-08-07 16:51:02.231922900 +0300
> @@ -37,7 +37,7 @@
>  {"return": {}}
>  {"return": {}}
>  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, \
>  "event": "SHUTDOWN", "data": {"guest": false}}
> -{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, \
> "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", \
> "len": 4194304, "offset": 4194304, "speed": 65536, "type": \
> "mirror"}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, \
> "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", \
> "len": 0, "offset": 0, "speed": 65536, "type": "mirror"}}

This diff says both 'len' and 'offset' differ...

> 
>  === Start backup job and exit qemu ===
> 
> Failures: 185
> Failed 1 of 1 tests
> N = 8
> 
> It doesn't seems logical to expect the constant offset on cancel,
> so let filter it out.

s/let/let's/

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/185 | 2 +-
>  tests/qemu-iotests/185.out | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

> -{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
> "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 4194304, "offset": 
> 4194304, "speed": 65536, "type": "mirror"}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
> "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 4194304, "offset": 
> OFFSET, "speed": 65536, "type": "mirror"}}

...while this diff only touched 'offset'.  Did you copy-and-paste
incorrectly in the commit message?  If so, then with the commit message
fixed, I'm okay with:
Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH] iotests: fix 185

[Vladimir Sementsov-Ogievskiy]

185 iotest is broken.

How to test:
> i=0; while ./check -qcow2 -nocache 185; do ((i+=1)); echo N = $i; \
  done; echo N = $i

finished for me like this:

185 2s ... - output mismatch (see 185.out.bad)
--- /work/src/qemu/master/tests/qemu-iotests/185.out2017-07-14 \
15:14:29.520343805 +0300
+++ 185.out.bad 2017-08-07 16:51:02.231922900 +0300
@@ -37,7 +37,7 @@
 {"return": {}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, \
 "event": "SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, \
"event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", \
"len": 4194304, "offset": 4194304, "speed": 65536, "type": \
"mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, \
"event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", \
"len": 0, "offset": 0, "speed": 65536, "type": "mirror"}}

 === Start backup job and exit qemu ===

Failures: 185
Failed 1 of 1 tests
N = 8

It doesn't seems logical to expect the constant offset on cancel,
so let filter it out.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/185 | 2 +-
 tests/qemu-iotests/185.out | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/185 b/tests/qemu-iotests/185
index 0eda371f27..446d78447e 100755
--- a/tests/qemu-iotests/185
+++ b/tests/qemu-iotests/185
@@ -157,7 +157,7 @@ _send_qemu_cmd $h \
 "return"
 
 _send_qemu_cmd $h "{ 'execute': 'quit' }" "return"
-wait=1 _cleanup_qemu
+wait=1 _cleanup_qemu | _filter_block_job_offset
 
 echo
 echo === Start backup job and exit qemu ===
diff --git a/tests/qemu-iotests/185.out b/tests/qemu-iotests/185.out
index 57eaf8d699..f51af627fe 100644
--- a/tests/qemu-iotests/185.out
+++ b/tests/qemu-iotests/185.out
@@ -37,7 +37,7 @@ Formatting 'TEST_DIR/t.qcow2.copy', fmt=qcow2 size=67108864 
cluster_size=65536 l
 {"return": {}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 4194304, "offset": 
4194304, "speed": 65536, "type": "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 4194304, "offset": 
OFFSET, "speed": 65536, "type": "mirror"}}
 
 === Start backup job and exit qemu ===
 
-- 
2.11.1

Re: [Qemu-block] [Qemu-devel] [PATCH v2 for-2.10 2/4] block/vhdx: check for offset overflow to bdrv_truncate()

[Philippe Mathieu-Daudé]

On Mon, Aug 7, 2017 at 9:38 AM, Jeff Cody  wrote:
> VHDX uses uint64_t types for most offsets, following the VHDX spec.
> However, bdrv_truncate() takes an int64_t value for the truncating
> offset.  Check for overflow before calling bdrv_truncate().
>
> While we are here, replace the bit shifting with QEMU_ALIGN_UP as well.
>
> N.B.: For a compliant image this is not an issue, as the maximum VHDX
> image size is defined per the spec to be 64TB.
>
> Reviewed-by: Eric Blake 
> Reviewed-by: Kevin Wolf 
> Signed-off-by: Jeff Cody 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  block/vhdx-log.c | 6 +-
>  block/vhdx.c | 3 +++
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/block/vhdx-log.c b/block/vhdx-log.c
> index 2e26fd4..9597223 100644
> --- a/block/vhdx-log.c
> +++ b/block/vhdx-log.c
> @@ -553,7 +553,11 @@ static int vhdx_log_flush(BlockDriverState *bs, 
> BDRVVHDXState *s,
>  new_file_size = desc_entries->hdr.last_file_offset;
>  if (new_file_size % (1024*1024)) {
>  /* round up to nearest 1MB boundary */
> -new_file_size = ((new_file_size >> 20) + 1) << 20;
> +new_file_size = QEMU_ALIGN_UP(new_file_size, MiB);
> +if (new_file_size > INT64_MAX) {
> +ret = -EINVAL;
> +goto exit;
> +}
>  bdrv_truncate(bs->file, new_file_size, PREALLOC_MODE_OFF, 
> NULL);
>  }
>  }
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 37224b8..7ae4589 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -1177,6 +1177,9 @@ static int vhdx_allocate_block(BlockDriverState *bs, 
> BDRVVHDXState *s,
>
>  /* per the spec, the address for a block is in units of 1MB */
>  *new_offset = ROUND_UP(*new_offset, 1024 * 1024);
> +if (*new_offset > INT64_MAX) {
> +return -EINVAL;
> +}
>
>  return bdrv_truncate(bs->file, *new_offset + s->block_size,
>   PREALLOC_MODE_OFF, NULL);
> --
> 2.9.4
>
>

Re: [Qemu-block] [PATCH] block: drop bdrv_set_key from BlockDriver

[Kevin Wolf]

Am 04.08.2017 um 17:26 hat Paolo Bonzini geschrieben:
> This is not used anymore since c01c214b69 ("block: remove all encryption
> handling APIs", 2017-07-11).
> 
> Signed-off-by: Paolo Bonzini 

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [PATCH v2 for-2.10 0/4] VHDX bugfixes

[Kevin Wolf]

Am 07.08.2017 um 14:38 hat Jeff Cody geschrieben:
> 
> Some VHDX bug fixes, including:
> 
> 1. Checking bdrv_getlength(), bdrv_flush(), and bdrv_truncate() return values

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [PATCH for-2.10] quorum: Set sectors-count to 0 when reporting a flush error

[Kevin Wolf]

Am 07.08.2017 um 14:36 hat Alberto Garcia geschrieben:
> The QUORUM_REPORT_BAD event has fields to report the sector in which
> the error was detected and the number of affected sectors starting
> from that one. This is important for read and write errors, but not
> for flush errors.
> 
> For flush errors the current code reports the total size of the disk
> image. That is however not useful information in this case. Moreover,
> the bdrv_getlength() call can fail, and there's no good way of
> handling that failure.
> 
> Since we're reporting useless information and we cannot even guarantee
> to do it in a consistent way, this patch changes the code to report 0
> instead in all cases.
> 
> Reported-by: Markus Armbruster 
> Signed-off-by: Alberto Garcia 

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [PATCH 11/17] block/nbd-client: fix nbd_co_request: set s->reply.handle to 0 on error

[Vladimir Sementsov-Ogievskiy]

07.08.2017 14:55, Eric Blake wrote:

On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:

We set s->reply.handle to 0 on one error path and don't set on another.
For consistancy and to avoid assert in nbd_read_reply_entry let's
set s->reply.handle to 0 in case of wrong handle too.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/nbd-client.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

Can this assertion be triggered now (presumably, with a broken server)?
I'm trying to figure out if this is 2.10 material.

[Urgh. If a broken server is able to cause an assertion failure that
causes a client to abort on an assertion failure, that probably deserves
a CVE]

Hmm looks like I've mistaken, if handle is wrong than read_reply_co 
should be already finished, so it's impossible



--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH 06/17] block/nbd-client: fix nbd_read_reply_entry

[Vladimir Sementsov-Ogievskiy]

07.08.2017 14:52, Eric Blake wrote:

On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:

Set reply.handle to 0 on error path to prevent normal path of
nbd_co_receive_reply.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/nbd-client.c | 1 +
  1 file changed, 1 insertion(+)

Can you document a case where not fixing this would be an observable bug
(even if it requires using gdb and single-stepping between client and
server to make what is otherwise a racy situation easy to see)?  I'm
trying to figure out if this is 2.10 material.


it is simple enough:

run qemu-nbd in gdb, set break on nbd_send_reply, and when it shoot s,
next up to "stl_be_p(buf, NBD_REPLY_MAGIC);" and after it do "call 
stl_be_p(buf, 1000)"


run qemu-io with some read in gdb, set break on
br block/nbd-client.c:83

( it is break; after failed nbd_receive_reply call)

and on
br block/nbd-client.c:170

(it is in nbd_co_receive_reply after yield)

on first break we will be sure that  nbd_receive_reply failed,
on second we will be sure by
(gdb) p s->reply
$1 = {handle = 93825000680144, error = 0}
(gdb) p request->handle
$2 = 93825000680144

that we are on normal receiving path.




diff --git a/block/nbd-client.c b/block/nbd-client.c
index dc19894a7c..0c88d84de6 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -107,6 +107,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
  qemu_coroutine_yield();
  }
  
+s->reply.handle = 0;

  nbd_recv_coroutines_enter_all(s);
  s->read_reply_co = NULL;
  }



--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v2 for-2.10 4/4] block/vhdx: check error return of bdrv_truncate()

[Eric Blake]

On 08/07/2017 07:38 AM, Jeff Cody wrote:
> Signed-off-by: Jeff Cody 
> ---
>  block/vhdx-log.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake 

> diff --git a/block/vhdx-log.c b/block/vhdx-log.c
> index a27dc05..14b724e 100644
> --- a/block/vhdx-log.c
> +++ b/block/vhdx-log.c
> @@ -558,7 +558,11 @@ static int vhdx_log_flush(BlockDriverState *bs, 
> BDRVVHDXState *s,
>  ret = -EINVAL;
>  goto exit;
>  }
> -bdrv_truncate(bs->file, new_file_size, PREALLOC_MODE_OFF, 
> NULL);
> +ret = bdrv_truncate(bs->file, new_file_size, 
> PREALLOC_MODE_OFF,
> +NULL);
> +if (ret < 0) {
> +goto exit;
> +}
>  }
>  }
>  qemu_vfree(desc_entries);
> 

-- 
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-block] [PATCH v2 for-2.10 3/4] block/vhdx: check error return of bdrv_flush()

[Eric Blake]

On 08/07/2017 07:38 AM, Jeff Cody wrote:
> Reported-by: Kevin Wolf 
> Signed-off-by: Jeff Cody 
> ---
>  block/vhdx-log.c | 17 ++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
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-block] [PATCH for-2.10] quorum: Set sectors-count to 0 when reporting a flush error

[Eric Blake]

On 08/07/2017 07:36 AM, Alberto Garcia wrote:
> The QUORUM_REPORT_BAD event has fields to report the sector in which
> the error was detected and the number of affected sectors starting
> from that one. This is important for read and write errors, but not
> for flush errors.
> 
> For flush errors the current code reports the total size of the disk
> image. That is however not useful information in this case. Moreover,
> the bdrv_getlength() call can fail, and there's no good way of
> handling that failure.
> 
> Since we're reporting useless information and we cannot even guarantee
> to do it in a consistent way, this patch changes the code to report 0
> instead in all cases.
> 
> Reported-by: Markus Armbruster 
> Signed-off-by: Alberto Garcia 
> ---
>  block/quorum.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature