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

[Kevin Wolf]

Am 03.08.2017 um 17:02 hat Kevin Wolf geschrieben:
> 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.

Applied to the block branch.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Markus Armbruster]

Cleber Rosa  writes:

> On 07/21/2017 08:33 AM, Stefan Hajnoczi wrote:
>> On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
>>> This is a follow up to a previous discussion about reported failures when
>>> running some qemu-iotests.  Turns out the failures were due to missing
>>> libraries, which in turn, reflected on the host build configuration.
>>>
>>> This series introduces a tool that can check both host and target level
>>> build configurations.  On top of that, it adds a function to to be used
>>> on qemu-iotests.  Finally, as an example, it sets a test to be skipped
>>> if the required feature is not enable on the host build configuration.
>>>
>>> Cleber Rosa (3):
>>>   scripts: introduce buildconf.py
>>>   qemu-iotests: add _require_feature() function
>>>   qemu-iotests: require CONFIG_LINUX_AIO for test 087
>>>
>>>  scripts/buildconf.py | 278 
>>> +++
>>>  tests/qemu-iotests/087   |   1 +
>>>  tests/qemu-iotests/check |   2 +
>>>  tests/qemu-iotests/common.rc |   7 ++
>>>  4 files changed, 288 insertions(+)
>>>
>> 
>> It should be possible to run iotests against any
>> qemu/qemu-img/qemu-io/qemu-nbd binaries - even if no build root is
>> available.
>> 
>
> Yes, I actually overlooked that point.
>
>> How about invoking qemu-img and tools to determine their capabilities?
>> 
>
> Can capabilities be consistently queried?  I would love to not count on
> a build root if the same information can be consistently queried from
> the binaries themselves.
>
>> At the beginning of ./check, query the qemu/qemu-img/qemu-io/qemu-nbd
>> binaries for specific features.  This produces a set of available
>> features and tests can say:
>> 
>
> Which would be another ad-hoc thing, limited to qemu-iotests.  From a
> test writer perspective, QEMU lacks is a uniform way to introspect its
> capabilities.

The closest we have is query-qmp-schema.  It's uniform, but limited to
the qapified part of QMP (see my KVM Forum 2015 talk[*] for details).
Something similar for the command line would be nice, and I hope to get
there some day.  Until then, you can often reason like "if QMP supports
X, then surely the command line supports X'".

We commonly reason "if INTERFACE supports FEATURE-API, then QEMU surely
supports FEATURE".

[...]

[*] 
https://events.linuxfoundation.org/sites/events/files/slides/armbru-qemu-introspection.pdf

Re: [Qemu-block] [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Markus Armbruster]

Stefan Hajnoczi  writes:

> On Wed, Jul 26, 2017 at 02:24:02PM -0400, Cleber Rosa wrote:
>> 
>> 
>> On 07/26/2017 01:58 PM, Stefan Hajnoczi wrote:
>> > On Tue, Jul 25, 2017 at 12:16:13PM -0400, Cleber Rosa wrote:
>> >> On 07/25/2017 11:49 AM, Stefan Hajnoczi wrote:
>> >>> On Fri, Jul 21, 2017 at 10:21:24AM -0400, Cleber Rosa wrote:
>>  On 07/21/2017 10:01 AM, Daniel P. Berrange wrote:
>> > On Fri, Jul 21, 2017 at 01:33:25PM +0100, Stefan Hajnoczi wrote:
>> >> On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
>>  Without the static capabilities defined, the dynamic check would be
>>  influenced by the run time environment.  It would really mean "qemu-io
>>  running on this environment (filesystem?) can do native aio".  Again,
>>  that's not the best type of information to depend on when writing tests.
>> >>>
>> >>> Can you explain this more?
>> >>>
>> >>> It seems logical to me that if qemu-io in this environment cannot do
>> >>> aio=native then we must skip those tests.
>> >>>
>> >>> Stefan
>> >>>
>> >>
>> >> OK, let's abstract a bit more.  Let's take this part of your statement:
>> >>
>> >>  "if qemu-io in this environment cannot do aio=native"
>> >>
>> >> Let's call that a feature check.  Depending on how the *feature check*
>> >> is written, a negative result may hide a test failure, because it would
>> >> now be skipped.
>> > 
>> > You are saying a pass->skip transition can hide a failure but ./check
>> > tracks skipped tests.  See tests/qemu-iotests/check.log for a
>> > pass/fail/skip history.
>> > 
>> 
>> You're not focusing on the problem here.  The problem is that a test
>> that *was not* supposed to be skipped, would be skipped.
>
> As Daniel Berrange mentioned, ./configure has the same problem.  You
> cannot just run it blindly because it silently disables features.
>
> What I'm saying is that in addition to watching ./configure closely, you
> also need to look at the skipped tests that ./check reports.  If you do
> that then you can be sure the expected set of tests is passing.
>
>> > It is the job of the CI system to flag up pass/fail/skip transitions.
>> > You're no worse off using feature tests.
>> > 
>> > Stefan
>> > 
>> 
>> What I'm trying to help us achieve here is a reliable and predictable
>> way for the same test job execution to be comparable across
>> environments.  From the individual developer workstation, CI, QA etc.
>
> 1. Use ./configure --enable-foo options for all desired features.
> 2. Run the ./check command-line and there should be no unexpected skips
>like this:
>
> 087 [not run] missing aio=native support
>
> To me this seems to address the problem.
>
> I have mentioned the issues with the build flags solution: it creates a
> dependency on the build environment and forces test feature checks to
> duplicate build dependency logic.  This is why I think feature tests are
> a cleaner solution.

I suspect the actual problem here is that the qemu-iotests harness is
not integrated in the build process.  For other tests, we specify the
tests to run in a Makefile, and use the same configuration mechanism as
for building stuff conditionally.

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

[Kevin Wolf]

Am 07.08.2017 um 22:30 hat Eric Blake geschrieben:
> 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)

qcow2 ended up with this prototype:

int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
 unsigned int *bytes, uint64_t *cluster_offset)

Separating a full uin64_t offset by-reference parameter and an 0/-errno
status return code feels a little cleaner to me.

And in fact, it's not only cleaner, but bit 63 is QCOW_OFLAG_COMPRESSED,
so this patch breaks compressed images.

>  {
>  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(&s->lock);
>  cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
>  qemu_co_mutex_unlock(&s->lock);
> +if (cluster_offset < 0) {
> +return cluster_offset;
> +}
>  index_in_cluster = sector_num & (s->cluster_sectors -

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

[Kevin Wolf]

Am 07.08.2017 um 22:30 hat Eric Blake geschrieben:
> 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, &child_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");

I would start the error message with a capital letter for consistency
with other messages in this function. (It has obviously nothing to do
with my general preference for that style.)

> +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;

Kevin

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

[Kevin Wolf]

Am 07.08.2017 um 22:30 hat Eric Blake geschrieben:
> 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: Kevin Wolf 

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

[Kevin Wolf]

Am 07.08.2017 um 22:30 hat Eric Blake geschrieben:
> 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: Kevin Wolf 

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

[Vladimir Sementsov-Ogievskiy]

07.08.2017 18:57, Kevin Wolf wrote:

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


Throttling "guaranties" that there will not be more than one request. 
But what prevent less than one, i.e. zero, like in my reproduction?




(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



--
Best regards,
Vladimir

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

[Kevin Wolf]

Am 08.08.2017 um 10:42 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 07.08.2017 18:57, Kevin Wolf wrote:
> > 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'?
> 
> Throttling "guaranties" that there will not be more than one request. But
> what prevent less than one, i.e. zero, like in my reproduction?

Yes, I understand. Can we somehow make sure that at least one iteration
is made? I'd really like to keep the functional test for block job
throttling. I suppose a simple 'sleep 0.1' would do the trick, though
it's not very clean.

Kevin

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

[Vladimir Sementsov-Ogievskiy]

08.08.2017 12:04, Vladimir Sementsov-Ogievskiy wrote:

08.08.2017 11:53, Kevin Wolf wrote:

Am 08.08.2017 um 10:42 hat Vladimir Sementsov-Ogievskiy geschrieben:

07.08.2017 18:57, Kevin Wolf wrote:

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.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"}}

   === 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'?
Throttling "guaranties" that there will not be more than one 
request. But

what prevent less than one, i.e. zero, like in my reproduction?

Yes, I understand. Can we somehow make sure that at least one iteration
is made? I'd really like to keep the functional test for block job
throttling. I suppose a simple 'sleep 0.1' would do the trick, though
it's not very clean.

Kevin



I've started with 'sleep 0.5', now there are >100 successful 
iterations... The other way is to check in test that there was 0 or 1 
requests, but for this it looks better to rewrite it in python.





is sleep for ms portable?



--
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

08.08.2017 11:53, Kevin Wolf wrote:

Am 08.08.2017 um 10:42 hat Vladimir Sementsov-Ogievskiy geschrieben:

07.08.2017 18:57, Kevin Wolf wrote:

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

Throttling "guaranties" that there will not be more than one request. But
what prevent less than one, i.e. zero, like in my reproduction?

Yes, I understand. Can we somehow make sure that at least one iteration
is made? I'd really like to keep the functional test for block job
throttling. I suppose a simple 'sleep 0.1' would do the trick, though
it's not very clean.

Kevin



I've started with 'sleep 0.5', now there are >100 successful 
iterations... The other way is to check in test that there was 0 or 1 
requests, but for this it looks better to rewrite it in python.



--
Best regards,
Vladimir

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

[Daniel P. Berrange]

On Tue, Aug 08, 2017 at 10:39:29AM +0800, Fam Zheng wrote:
> 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.

That will have BDRV_REQ_ZERO_WRITE flag set though, so we don't end up
calling the bdrv_co_pwritev() callback function registered by the block
driver.

> So for write we cannot remove the bytes parameter.

We can't remove it from the bdrv_co_pwritev() function, but we can remove
it from bdrv_co_pwritev block driver callback AFAICT.

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

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

[Stefan Hajnoczi]

On Mon, Aug 07, 2017 at 06:29:09PM -0400, 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 
> ---
>  block/nfs.c | 21 +
>  1 file changed, 13 insertions(+), 8 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

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

[Stefan Hajnoczi]

On Mon, Aug 07, 2017 at 07:15:29PM +0300, 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(-)

Why is this marked for-2.10?  Does it fix a bug?

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH] block/null: Remove 'filename' option

[Stefan Hajnoczi]

On Fri, Aug 04, 2017 at 04:43:54PM +0200, Kevin Wolf wrote:
> This option was only added to allow 'null-co://' and 'null-aio://' as
> filenames, its value never served any actual purpose and was ignored.
> Nevertheless it was accepted as '-drive driver=null,filename=foo'.
> 
> The correct way to enable the protocol prefixes (and that without adding
> a useless -drive option) is implementing .bdrv_parse_filename. This is
> what this patch does.
> 
> Technically, this is an incompatible change, but the null block driver
> is only used for benchmarking, testing and debugging, and an option
> without effect isn't likely to be used by anyone anyway, so no bad
> effects are to be expected.
> 
> Reported-by: Markus Armbruster 
> Signed-off-by: Kevin Wolf 
> ---
>  block/null.c | 31 ++-
>  1 file changed, 26 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v2] qemu-img: Clarify about relative backing file options

[Stefan Hajnoczi]

On Fri, Aug 04, 2017 at 10:36:58PM +0800, Fam Zheng wrote:
> It's not too surprising when a user specifies the backing file relative
> to the current working directory instead of the top layer image. This
> causes error when they differ. Though the error message has enough
> information to infer the fact about the misunderstanding, it is better
> if we document this explicitly, so that users don't have to learn from
> mistakes.
> 
> Signed-off-by: Fam Zheng 
> 
> ---
> v2: Improve wording. [Eric]
> ---
>  qemu-img.texi | 9 +
>  1 file changed, 9 insertions(+)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

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

[Stefan Hajnoczi]

On Fri, Aug 04, 2017 at 03:08:26PM +0100, Daniel P. Berrange wrote:
> Signed-off-by: Daniel P. Berrange 
> ---
> 
>  - Clarify that @bytes matches @qiov total size (Kevin)
> 
>  include/block/block_int.h | 31 +++
>  1 file changed, 31 insertions(+)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

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

[Alberto Garcia]

On Tue 08 Aug 2017 12:00:30 PM CEST, Stefan Hajnoczi wrote:
> On Mon, Aug 07, 2017 at 07:15:29PM +0300, 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.
>> 
> Why is this marked for-2.10?  Does it fix a bug?

I was under the impression that Markus wanted to change the QAPI types
of the throttling fields in BlockDeviceInfo for 2.10 as well, so this
patch is relevant.

If that's not the case we can ignore it for now and leave it for the
next cycle. Sorry for the noise!

Berto

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

[Vladimir Sementsov-Ogievskiy]

08.08.2017 12:04, Vladimir Sementsov-Ogievskiy wrote:

08.08.2017 12:04, Vladimir Sementsov-Ogievskiy wrote:

08.08.2017 11:53, Kevin Wolf wrote:

Am 08.08.2017 um 10:42 hat Vladimir Sementsov-Ogievskiy geschrieben:

07.08.2017 18:57, Kevin Wolf wrote:

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.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"}}

   === 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'?
Throttling "guaranties" that there will not be more than one 
request. But

what prevent less than one, i.e. zero, like in my reproduction?

Yes, I understand. Can we somehow make sure that at least one iteration
is made? I'd really like to keep the functional test for block job
throttling. I suppose a simple 'sleep 0.1' would do the trick, though
it's not very clean.

Kevin



I've started with 'sleep 0.5', now there are >100 successful 
iterations... The other way is to check in test that there was 0 or 1 
requests, but for this it looks better to rewrite it in python.





is sleep for ms portable?




>1500 successful iterations, so, 'sleep 0.5' is OK for me.


--
Best regards,
Vladimir

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

[Alberto Garcia]

On Tue 08 Aug 2017 12:17:12 PM CEST, Alberto Garcia wrote:

> I was under the impression that Markus wanted to change the QAPI types
> of the throttling fields in BlockDeviceInfo for 2.10 as well, so this
> patch is relevant.

I just saw that his series is still an RFC, so we can leave this patch
for 2.11. Sorry again!

Berto

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

[Peter Lieven]

Am 08.08.2017 um 00:29 schrieb 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(&client->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(&client->mutex);

  opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
  qemu_opts_absorb_qdict(opts, options, &local_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(&client->mutex);
+
  bs->total_sectors = ret;
  ret = 0;
  return ret;


Reviewed-by: Peter Lieven 

Also CC'ing qemu-stable as this affects 2.9.0 as well.

Peter

Re: [Qemu-block] [Qemu-devel] [PATCH 3/3] maint: Include bug-reporting info in --help output.

[Markus Armbruster]

Eric Blake  writes:

> These days, many programs are including a bug-reporting address,
> or better yet, a link to the project web site, at the tail of
> their --help output.  However, we were not very consistent at
> doing so: only qemu-nbd and qemu-qa mentioned anything, with the
> latter pointing to an individual person instead of the project.
>
> Add a new #define that sets up a uniform string, mentioning both
> bug reporting instructions and overall project details, and which
> a downstream vendor could tweak if they want bugs to go to a
> downstream database.  Then use it in all of our binaries which
> have --help output.
>
> The canned text intentionally references http:// instead of https://
> because our https website currently causes certificate errors in
> some browsers.  That can be tweaked later once we have resolved the
> web site issued.
>
> Signed-off-by: Eric Blake 
> ---
>  include/qemu-common.h | 5 +
>  vl.c  | 4 +++-
>  bsd-user/main.c   | 2 ++
>  linux-user/main.c | 4 +++-
>  qemu-img.c| 2 +-
>  qemu-io.c | 5 +++--
>  qemu-nbd.c| 2 +-
>  qga/main.c| 2 +-
>  8 files changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index b5adbfa5e9..e751361458 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -22,6 +22,11 @@
>  #define QEMU_COPYRIGHT "Copyright (c) 2003-2017 " \
>  "Fabrice Bellard and the QEMU Project developers"
>
> +/* Bug reporting information for --help arguments, About dialogs, etc */
> +#define QEMU_BUGREPORTS \
> +"See  for bug reports.\n" \

"See ... for bug reports" sounds like it's about browsing existing bugs.
The web page is actually about reporting bugs.  What about "for how to
report bugs"?

Since I'm basically bikeshedding already: the macro expands into more
than just bug reporting.  Call it QEMU_HELP_BOTTOM?  Feel free to ignore
this one.

> +"More information on the qemu project at "

"QEMU project"

> +
>  /* main function, renamed */
>  #if defined(CONFIG_COCOA)
>  int qemu_main(int argc, char **argv, char **envp);

Getting late for 2.10, but it's such a lovely little improvement...

Re: [Qemu-block] [Qemu-devel] [PATCH 3/3] maint: Include bug-reporting info in --help output.

[Paolo Bonzini]

On 08/08/2017 13:06, Markus Armbruster wrote:
> Eric Blake  writes:
> 
>> These days, many programs are including a bug-reporting address,
>> or better yet, a link to the project web site, at the tail of
>> their --help output.  However, we were not very consistent at
>> doing so: only qemu-nbd and qemu-qa mentioned anything, with the
>> latter pointing to an individual person instead of the project.
>>
>> Add a new #define that sets up a uniform string, mentioning both
>> bug reporting instructions and overall project details, and which
>> a downstream vendor could tweak if they want bugs to go to a
>> downstream database.  Then use it in all of our binaries which
>> have --help output.
>>
>> The canned text intentionally references http:// instead of https://
>> because our https website currently causes certificate errors in
>> some browsers.  That can be tweaked later once we have resolved the
>> web site issued.
>>
>> Signed-off-by: Eric Blake 
>> ---
>>  include/qemu-common.h | 5 +
>>  vl.c  | 4 +++-
>>  bsd-user/main.c   | 2 ++
>>  linux-user/main.c | 4 +++-
>>  qemu-img.c| 2 +-
>>  qemu-io.c | 5 +++--
>>  qemu-nbd.c| 2 +-
>>  qga/main.c| 2 +-
>>  8 files changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/qemu-common.h b/include/qemu-common.h
>> index b5adbfa5e9..e751361458 100644
>> --- a/include/qemu-common.h
>> +++ b/include/qemu-common.h
>> @@ -22,6 +22,11 @@
>>  #define QEMU_COPYRIGHT "Copyright (c) 2003-2017 " \
>>  "Fabrice Bellard and the QEMU Project developers"
>>
>> +/* Bug reporting information for --help arguments, About dialogs, etc */
>> +#define QEMU_BUGREPORTS \
>> +"See  for bug reports.\n" \
> 
> "See ... for bug reports" sounds like it's about browsing existing bugs.
> The web page is actually about reporting bugs.  What about "for how to
> report bugs"?
> 
> Since I'm basically bikeshedding already: the macro expands into more
> than just bug reporting.  Call it QEMU_HELP_BOTTOM?  Feel free to ignore
> this one.

Easily squashed:

diff --git a/bsd-user/main.c b/bsd-user/main.c
index 4db10cb376..8a6706a1c8 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -687,7 +687,7 @@ static void usage(void)
"Note that if you provide several changes to single variable\n"
"last change will stay in effect.\n"
"\n"
-   QEMU_BUGREPORTS "\n"
+   QEMU_HELP_BOTTOM "\n"
,
TARGET_NAME,
interp_prefix,
diff --git a/include/qemu-common.h b/include/qemu-common.h
index d29045631f..0456c79df4 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -23,8 +23,8 @@
 "Fabrice Bellard and the QEMU Project developers"
 
 /* Bug reporting information for --help arguments, About dialogs, etc */
-#define QEMU_BUGREPORTS \
-"See  for bug reports.\n" \
+#define QEMU_HELP_BOTTOM \
+"See  for how to report bugs.\n" \
 "More information on the QEMU project at ."
 
 /* main function, renamed */
diff --git a/linux-user/main.c b/linux-user/main.c
index 7d6e481277..03666ef657 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -4138,7 +4138,7 @@ static void usage(int exitcode)
"Note that if you provide several changes to a single variable\n"
"the last change will stay in effect.\n"
"\n"
-   QEMU_BUGREPORTS "\n");
+   QEMU_HELP_BOTTOM "\n");
 
 exit(exitcode);
 }
diff --git a/qemu-img.c b/qemu-img.c
index 758719e083..56ef49e214 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -201,7 +201,7 @@ static void QEMU_NORETURN help(void)
 
 printf("%s\nSupported formats:", help_msg);
 bdrv_iterate_format(format_print, NULL);
-printf("\n\n" QEMU_BUGREPORTS "\n");
+printf("\n\n" QEMU_HELP_BOTTOM "\n");
 exit(EXIT_SUCCESS);
 }
 
diff --git a/qemu-io.c b/qemu-io.c
index b93553a603..265445ad89 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -264,7 +264,7 @@ static void usage(const char *name)
 "\n"
 "See '%s -c help' for information on available commands.\n"
 "\n"
-QEMU_BUGREPORTS "\n",
+QEMU_HELP_BOTTOM "\n",
 name, name);
 }
 
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 052eb4d067..27164b8205 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -123,7 +123,7 @@ static void usage(const char *name)
 "  --detect-zeroes=MODE  set detect-zeroes mode (off, on, unmap)\n"
 "  --image-opts  treat FILE as a full set of image options\n"
 "\n"
-QEMU_BUGREPORTS "\n"
+QEMU_HELP_BOTTOM "\n"
 , name, NBD_DEFAULT_PORT, "DEVICE");
 }
 
diff --git a/qga/main.c b/qga/main.c
index 56d5633c13..62a62755bd 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -249,7 +249,7 @@ QEMU_COPYRIGHT "\n"
 "options / command-line parameters to stdout\n"
 "  -h, --helpdisplay this help and exit\n"
 "\n"
-QEMU_BUGREPOR

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

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:
> 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).

'o' is messy.  It using qemu_strtosz_MiB which uses a 'double' intermediate
so I fear it can round.  It also has a note it can't take all f's due to
an overflow from the conversion.   Two things not mentioned are that
it also takes hex (as explicit 0x) and that it also does 'b' as a suffix
to multiply by 1.  Those two combine in bad ways - i.e. 0x1b is 27MB,
1b is 1 byte (same for 'e').  These are probably OK except if you were
to start replacing 'l' by 'o' because you really wanted 64bit addresses
say.
(I also wouldn't bother expanding the size names and powers).

> + *
> + * 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).

I found the previous description clearer.

> + * 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', '/',

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

[Kevin Wolf]

Am 08.08.2017 um 00:29 hat Jeff Cody geschrieben:
> 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 

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] Unchecked blk_getlength() in device models and board code

[Stefan Hajnoczi]

On Fri, Aug 04, 2017 at 04:01:18PM +0200, Markus Armbruster wrote:
> blk_getlength() can fail.  I figure the following need fixing:
> 
> hw/arm/musicpal.c: musicpal_init()

Seems okay:

flash_size = blk_getlength(blk);
if (flash_size != 8*1024*1024 && flash_size != 16*1024*1024 &&
flash_size != 32*1024*1024) {
fprintf(stderr, "Invalid flash image size\n");
exit(1);
}

> hw/block/virtio-blk.c: virtio_blk_update_config()

Will send a fix.


signature.asc
Description: PGP signature

Re: [Qemu-block] [Qemu-devel] Unchecked blk_getlength() in device models and board code

[Peter Maydell]

On 8 August 2017 at 13:04, Stefan Hajnoczi  wrote:
> On Fri, Aug 04, 2017 at 04:01:18PM +0200, Markus Armbruster wrote:
>> blk_getlength() can fail.  I figure the following need fixing:
>>
>> hw/arm/musicpal.c: musicpal_init()
>
> Seems okay:
>
> flash_size = blk_getlength(blk);
> if (flash_size != 8*1024*1024 && flash_size != 16*1024*1024 &&
> flash_size != 32*1024*1024) {
> fprintf(stderr, "Invalid flash image size\n");
> exit(1);
> }

A lot of the time for flash devices it's not possible to
get an error code out of blk_getlength() anyway, because
AFAIK it only happens for cases like "cdrom with CD
ejected" and "sd card emulating the card-ejected state".
Still better to handle the error value than not, though.

thanks
-- PMM

[Qemu-block] [PATCH] virtio-blk: handle blk_getlength() errors

[Stefan Hajnoczi]

If blk_getlength() fails in virtio_blk_update_config() consider the disk
image length to be 0 bytes.

Signed-off-by: Stefan Hajnoczi 
---
 hw/block/virtio-blk.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index b750bd8b53..a16ac75090 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -730,6 +730,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, 
uint8_t *config)
 BlockConf *conf = &s->conf.conf;
 struct virtio_blk_config blkcfg;
 uint64_t capacity;
+int64_t length;
 int blk_size = conf->logical_block_size;
 
 blk_get_geometry(s->blk, &capacity);
@@ -752,7 +753,8 @@ static void virtio_blk_update_config(VirtIODevice *vdev, 
uint8_t *config)
  * divided by 512 - instead it is the amount of blk_size blocks
  * per track (cylinder).
  */
-if (blk_getlength(s->blk) /  conf->heads / conf->secs % blk_size) {
+length = blk_getlength(s->blk);
+if (length > 0 && length / conf->heads / conf->secs % blk_size) {
 blkcfg.geometry.sectors = conf->secs & ~s->sector_mask;
 } else {
 blkcfg.geometry.sectors = conf->secs;
-- 
2.13.3

Re: [Qemu-block] [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Stefan Hajnoczi]

On Tue, Aug 08, 2017 at 10:06:04AM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi  writes:
> 
> > On Wed, Jul 26, 2017 at 02:24:02PM -0400, Cleber Rosa wrote:
> >> 
> >> 
> >> On 07/26/2017 01:58 PM, Stefan Hajnoczi wrote:
> >> > On Tue, Jul 25, 2017 at 12:16:13PM -0400, Cleber Rosa wrote:
> >> >> On 07/25/2017 11:49 AM, Stefan Hajnoczi wrote:
> >> >>> On Fri, Jul 21, 2017 at 10:21:24AM -0400, Cleber Rosa wrote:
> >>  On 07/21/2017 10:01 AM, Daniel P. Berrange wrote:
> >> > On Fri, Jul 21, 2017 at 01:33:25PM +0100, Stefan Hajnoczi wrote:
> >> >> On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
> >>  Without the static capabilities defined, the dynamic check would be
> >>  influenced by the run time environment.  It would really mean "qemu-io
> >>  running on this environment (filesystem?) can do native aio".  Again,
> >>  that's not the best type of information to depend on when writing 
> >>  tests.
> >> >>>
> >> >>> Can you explain this more?
> >> >>>
> >> >>> It seems logical to me that if qemu-io in this environment cannot do
> >> >>> aio=native then we must skip those tests.
> >> >>>
> >> >>> Stefan
> >> >>>
> >> >>
> >> >> OK, let's abstract a bit more.  Let's take this part of your statement:
> >> >>
> >> >>  "if qemu-io in this environment cannot do aio=native"
> >> >>
> >> >> Let's call that a feature check.  Depending on how the *feature check*
> >> >> is written, a negative result may hide a test failure, because it would
> >> >> now be skipped.
> >> > 
> >> > You are saying a pass->skip transition can hide a failure but ./check
> >> > tracks skipped tests.  See tests/qemu-iotests/check.log for a
> >> > pass/fail/skip history.
> >> > 
> >> 
> >> You're not focusing on the problem here.  The problem is that a test
> >> that *was not* supposed to be skipped, would be skipped.
> >
> > As Daniel Berrange mentioned, ./configure has the same problem.  You
> > cannot just run it blindly because it silently disables features.
> >
> > What I'm saying is that in addition to watching ./configure closely, you
> > also need to look at the skipped tests that ./check reports.  If you do
> > that then you can be sure the expected set of tests is passing.
> >
> >> > It is the job of the CI system to flag up pass/fail/skip transitions.
> >> > You're no worse off using feature tests.
> >> > 
> >> > Stefan
> >> > 
> >> 
> >> What I'm trying to help us achieve here is a reliable and predictable
> >> way for the same test job execution to be comparable across
> >> environments.  From the individual developer workstation, CI, QA etc.
> >
> > 1. Use ./configure --enable-foo options for all desired features.
> > 2. Run the ./check command-line and there should be no unexpected skips
> >like this:
> >
> > 087 [not run] missing aio=native support
> >
> > To me this seems to address the problem.
> >
> > I have mentioned the issues with the build flags solution: it creates a
> > dependency on the build environment and forces test feature checks to
> > duplicate build dependency logic.  This is why I think feature tests are
> > a cleaner solution.
> 
> I suspect the actual problem here is that the qemu-iotests harness is
> not integrated in the build process.  For other tests, we specify the
> tests to run in a Makefile, and use the same configuration mechanism as
> for building stuff conditionally.

The ability to run tests against QEMU binaries without a build
environment is useful though.  It would still be possible to symlink to
external binaries but then the build feature information could be
incorrect.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v3 4/7] block: convert ThrottleGroup to object with QOM

[Alberto Garcia]

On Wed 02 Aug 2017 12:57:04 PM CEST, Manos Pitsidianakis wrote:
>> At the moment I think throttle_groups_lock isn't strictly needed
>> because incref/decref callers hold the QEMU global mutex anyway.
>>
>> But code accessing throttle_groups still has to be disciplined.
>> Since throttle_groups_lock exists, please use it consistently in all
>> code paths.
>>
>> Alternatively you could remove the lock and document that
>> throttle_groups is protected by the global mutex.  What we can't do
>> is sometimes use throttle_groups_lock and sometimes not use it.
>
> If we use throttle_groups_lock in throttle_group_obj_init() then we
> must give it up in throttle_group_incref() and retake it in
> throttle_group_obj_init(). Maybe indeed it's better to drop
> throttle_groups_lock altogether, since the ThrottleGroup refcounting
> always happens in a QMP or startup/cleanup context.

I checked the code and I also don't see any manipulation of the group
list outside the global mutex, so you can remove throttle_groups_lock,
but please document very clearly that all these calls can only happen
when they are protected by the global mutex.

Berto

Re: [Qemu-block] [PATCH v3 5/7] block: add throttle block filter driver

[Alberto Garcia]

On Mon 31 Jul 2017 11:54:41 AM CEST, Manos Pitsidianakis wrote:
> block/throttle.c uses existing I/O throttle infrastructure inside a
> block filter driver. I/O operations are intercepted in the filter's
> read/write coroutines, and referred to block/throttle-groups.c
>
> The driver can be used with the syntax
> -drive driver=throttle,file.filename=foo.qcow2, \
> limits.iops-total=...,throttle-group=bar

Sorry for not having noticed this earlier, but can't you define the
throttling group (and its limits) using -object throttle-group ... as
shown in the previous patch, and simply reference it here? Or would we
have two alternative ways of setting the throttling limits?

What happens if you have many -drive lines each one with a different set
of limits but with the same throttling group?

Berto

[Qemu-block] [PATCH for-2.11 6/7] block/curl: fix minor memory leaks

[Jeff Cody]

Signed-off-by: Jeff Cody 
---
 block/curl.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/block/curl.c b/block/curl.c
index 00a9879..35cf417 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -857,6 +857,9 @@ out_noclean:
 qemu_mutex_destroy(&s->mutex);
 g_free(s->cookie);
 g_free(s->url);
+g_free(s->username);
+g_free(s->proxyusername);
+g_free(s->proxypassword);
 qemu_opts_del(opts);
 return -EINVAL;
 }
@@ -955,6 +958,9 @@ static void curl_close(BlockDriverState *bs)
 
 g_free(s->cookie);
 g_free(s->url);
+g_free(s->username);
+g_free(s->proxyusername);
+g_free(s->proxypassword);
 }
 
 static int64_t curl_getlength(BlockDriverState *bs)
-- 
2.9.4

[Qemu-block] [PATCH for-2.11 5/7] block/curl: check error return of curl_global_init()

[Jeff Cody]

If curl_global_init() fails, per the documentation no other curl
functions may be called, so make sure to check the return value.

Also, some minor changes to the initialization latch variable 'inited':

- Make it static in the file, for clarity
- Change the name for clarity
- Make it a bool

Signed-off-by: Jeff Cody 
---
 block/curl.c | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 2a244e2..00a9879 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -89,6 +89,8 @@ static CURLMcode __curl_multi_socket_action(CURLM 
*multi_handle,
 
 struct BDRVCURLState;
 
+static bool libcurl_initialized;
+
 typedef struct CURLAIOCB {
 Coroutine *co;
 QEMUIOVector *qiov;
@@ -686,14 +688,23 @@ static int curl_open(BlockDriverState *bs, QDict 
*options, int flags,
 double d;
 const char *secretid;
 const char *protocol_delimiter;
+int ret;
 
-static int inited = 0;
 
 if (flags & BDRV_O_RDWR) {
 error_setg(errp, "curl block device does not support writes");
 return -EROFS;
 }
 
+if (!libcurl_initialized) {
+ret = curl_global_init(CURL_GLOBAL_ALL);
+if (ret) {
+error_setg(errp, "libcurl initialization failed with %d", ret);
+return -EIO;
+}
+libcurl_initialized = true;
+}
+
 qemu_mutex_init(&s->mutex);
 opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
 qemu_opts_absorb_qdict(opts, options, &local_err);
@@ -772,11 +783,6 @@ static int curl_open(BlockDriverState *bs, QDict *options, 
int flags,
 }
 }
 
-if (!inited) {
-curl_global_init(CURL_GLOBAL_ALL);
-inited = 1;
-}
-
 DPRINTF("CURL: Opening %s\n", file);
 QSIMPLEQ_INIT(&s->free_state_waitq);
 s->aio_context = bdrv_get_aio_context(bs);
-- 
2.9.4

[Qemu-block] [PATCH for-2.11 0/7] Code cleanup and minor fixes

[Jeff Cody]

Some minor cleanup for a few network protocols, with a few bug fixes thrown in.
I don't think there is anything in here that needs to be for 2.10, however.

Jeff Cody (7):
  block/ssh: don't call libssh2_init() in block_init()
  block/ssh: make compliant with coding guidelines
  block/sheepdog: remove spurious NULL check
  block/sheepdog: code beautification
  block/curl: check error return of curl_global_init()
  block/curl: fix minor memory leaks
  block/curl: code cleanup to comply with coding style

 block/curl.c | 124 +++--
 block/sheepdog.c | 164 +++
 block/ssh.c  |  72 +++-
 3 files changed, 199 insertions(+), 161 deletions(-)

-- 
2.9.4

[Qemu-block] [PATCH for-2.11 1/7] block/ssh: don't call libssh2_init() in block_init()

[Jeff Cody]

We don't need libssh2 failure to be fatal (we could just opt to not
register the driver on failure). But, it is probably a good idea to
avoid external library calls during the block_init(), and call the
libssh2 global init function on the first usage, returning any errors.

Signed-off-by: Jeff Cody 
---
 block/ssh.c | 40 +---
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index e8f0404..cbb0e34 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -83,12 +83,28 @@ typedef struct BDRVSSHState {
 bool unsafe_flush_warning;
 } BDRVSSHState;
 
-static void ssh_state_init(BDRVSSHState *s)
+static bool ssh_libinit_called;
+
+static int ssh_state_init(BDRVSSHState *s, Error **errp)
 {
+int ret;
+
+if (!ssh_libinit_called) {
+ret = libssh2_init(0);
+if (ret) {
+error_setg(errp, "libssh2 initialization failed with %d", ret);
+return ret;
+}
+ssh_libinit_called = true;
+}
+
+
 memset(s, 0, sizeof *s);
 s->sock = -1;
 s->offset = -1;
 qemu_co_mutex_init(&s->lock);
+
+return 0;
 }
 
 static void ssh_state_free(BDRVSSHState *s)
@@ -772,8 +788,13 @@ static int ssh_file_open(BlockDriverState *bs, QDict 
*options, int bdrv_flags,
 BDRVSSHState *s = bs->opaque;
 int ret;
 int ssh_flags;
+Error *local_err = NULL;
 
-ssh_state_init(s);
+ret = ssh_state_init(s, &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return ret;
+}
 
 ssh_flags = LIBSSH2_FXF_READ;
 if (bdrv_flags & BDRV_O_RDWR) {
@@ -821,8 +842,13 @@ static int ssh_create(const char *filename, QemuOpts 
*opts, Error **errp)
 BDRVSSHState s;
 ssize_t r2;
 char c[1] = { '\0' };
+Error *local_err = NULL;
 
-ssh_state_init(&s);
+ret = ssh_state_init(&s, &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return ret;
+}
 
 /* Get desired file size. */
 total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
@@ -1213,14 +1239,6 @@ static BlockDriver bdrv_ssh = {
 
 static void bdrv_ssh_init(void)
 {
-int r;
-
-r = libssh2_init(0);
-if (r != 0) {
-fprintf(stderr, "libssh2 initialization failed, %d\n", r);
-exit(EXIT_FAILURE);
-}
-
 bdrv_register(&bdrv_ssh);
 }
 
-- 
2.9.4

[Qemu-block] [PATCH for-2.11 7/7] block/curl: code cleanup to comply with coding style

[Jeff Cody]

This addresses non-functional changes to help curl.c better comply
with the coding styles (comments, indentation, brackets, etc.).

One minor code change is the combination of two if statements into
a single if statement.

Signed-off-by: Jeff Cody 
---
 block/curl.c | 100 +++
 1 file changed, 52 insertions(+), 48 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 35cf417..c557b59 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -32,8 +32,10 @@
 #include 
 #include "qemu/cutils.h"
 
-// #define DEBUG_CURL
-// #define DEBUG_VERBOSE
+/*
+ #define DEBUG_CURL
+ #define DEBUG_VERBOSE
+*/
 
 #ifdef DEBUG_CURL
 #define DEBUG_CURL_PRINT 1
@@ -76,15 +78,15 @@ static CURLMcode __curl_multi_socket_action(CURLM 
*multi_handle,
 #define CURL_TIMEOUT_DEFAULT 5
 #define CURL_TIMEOUT_MAX 1
 
-#define CURL_BLOCK_OPT_URL   "url"
-#define CURL_BLOCK_OPT_READAHEAD "readahead"
-#define CURL_BLOCK_OPT_SSLVERIFY "sslverify"
-#define CURL_BLOCK_OPT_TIMEOUT "timeout"
-#define CURL_BLOCK_OPT_COOKIE"cookie"
-#define CURL_BLOCK_OPT_COOKIE_SECRET "cookie-secret"
-#define CURL_BLOCK_OPT_USERNAME "username"
-#define CURL_BLOCK_OPT_PASSWORD_SECRET "password-secret"
-#define CURL_BLOCK_OPT_PROXY_USERNAME "proxy-username"
+#define CURL_BLOCK_OPT_URL   "url"
+#define CURL_BLOCK_OPT_READAHEAD "readahead"
+#define CURL_BLOCK_OPT_SSLVERIFY "sslverify"
+#define CURL_BLOCK_OPT_TIMEOUT   "timeout"
+#define CURL_BLOCK_OPT_COOKIE"cookie"
+#define CURL_BLOCK_OPT_COOKIE_SECRET "cookie-secret"
+#define CURL_BLOCK_OPT_USERNAME  "username"
+#define CURL_BLOCK_OPT_PASSWORD_SECRET   "password-secret"
+#define CURL_BLOCK_OPT_PROXY_USERNAME"proxy-username"
 #define CURL_BLOCK_OPT_PROXY_PASSWORD_SECRET "proxy-password-secret"
 
 struct BDRVCURLState;
@@ -110,8 +112,7 @@ typedef struct CURLSocket {
 QLIST_ENTRY(CURLSocket) next;
 } CURLSocket;
 
-typedef struct CURLState
-{
+typedef struct CURLState {
 struct BDRVCURLState *s;
 CURLAIOCB *acb[CURL_NUM_ACB];
 CURL *curl;
@@ -196,22 +197,22 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int 
action,
 
 DPRINTF("CURL (AIO): Sock action %d on fd %d\n", action, (int)fd);
 switch (action) {
-case CURL_POLL_IN:
-aio_set_fd_handler(s->aio_context, fd, false,
-   curl_multi_read, NULL, NULL, state);
-break;
-case CURL_POLL_OUT:
-aio_set_fd_handler(s->aio_context, fd, false,
-   NULL, curl_multi_do, NULL, state);
-break;
-case CURL_POLL_INOUT:
-aio_set_fd_handler(s->aio_context, fd, false,
-   curl_multi_read, curl_multi_do, NULL, state);
-break;
-case CURL_POLL_REMOVE:
-aio_set_fd_handler(s->aio_context, fd, false,
-   NULL, NULL, NULL, NULL);
-break;
+case CURL_POLL_IN:
+aio_set_fd_handler(s->aio_context, fd, false,
+   curl_multi_read, NULL, NULL, state);
+break;
+case CURL_POLL_OUT:
+aio_set_fd_handler(s->aio_context, fd, false,
+   NULL, curl_multi_do, NULL, state);
+break;
+case CURL_POLL_INOUT:
+aio_set_fd_handler(s->aio_context, fd, false,
+   curl_multi_read, curl_multi_do, NULL, state);
+break;
+case CURL_POLL_REMOVE:
+aio_set_fd_handler(s->aio_context, fd, false,
+   NULL, NULL, NULL, NULL);
+break;
 }
 
 return 0;
@@ -235,7 +236,7 @@ static size_t curl_header_cb(void *ptr, size_t size, size_t 
nmemb, void *opaque)
 /* Called from curl_multi_do_locked, with s->mutex held.  */
 static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
 {
-CURLState *s = ((CURLState*)opaque);
+CURLState *s = ((CURLState *)opaque);
 size_t realsize = size * nmemb;
 int i;
 
@@ -253,11 +254,12 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t 
nmemb, void *opaque)
 memcpy(s->orig_buf + s->buf_off, ptr, realsize);
 s->buf_off += realsize;
 
-for(i=0; iacb[i];
 
-if (!acb)
+if (!acb) {
 continue;
+}
 
 if ((s->buf_off >= acb->end)) {
 size_t request_length = acb->bytes;
@@ -293,17 +295,16 @@ static bool curl_find_buf(BDRVCURLState *s, uint64_t 
start, uint64_t len,
 uint64_t clamped_end = MIN(end, s->len);
 uint64_t clamped_len = clamped_end - start;
 
-for (i=0; istates[i];
 uint64_t buf_end = (state->buf_start + state->buf_off);
 uint64_t buf_fend = (state->buf_start + state->buf_len);
 
-if (!state->orig_buf)
-continue;
-if (!state->buf_off)
+if (!state->orig_buf || !state->buf_off) {
 continue;
+}
 
-

[Qemu-block] [PATCH for-2.11 3/7] block/sheepdog: remove spurious NULL check

[Jeff Cody]

'tag' is already checked in the lines immediately preceding this check,
and set to non-NULL if NULL.  No need to check again, it hasn't changed.

Signed-off-by: Jeff Cody 
---
 block/sheepdog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index abb2e79..bbbfa72 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1632,7 +1632,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, 
int flags,
 if (!tag) {
 tag = "";
 }
-if (tag && strlen(tag) >= SD_MAX_VDI_TAG_LEN) {
+if (strlen(tag) >= SD_MAX_VDI_TAG_LEN) {
 error_setg(errp, "value of parameter 'tag' is too long");
 ret = -EINVAL;
 goto err_no_fd;
-- 
2.9.4

[Qemu-block] [PATCH for-2.11 4/7] block/sheepdog: code beautification

[Jeff Cody]

No functional changes, just whitespace manipulation.

Signed-off-by: Jeff Cody 
---
 block/sheepdog.c | 162 +++
 1 file changed, 81 insertions(+), 81 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index bbbfa72..ad461f1 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -400,7 +400,7 @@ typedef struct BDRVSheepdogReopenState {
 int cache_flags;
 } BDRVSheepdogReopenState;
 
-static const char * sd_strerror(int err)
+static const char *sd_strerror(int err)
 {
 int i;
 
@@ -3078,111 +3078,111 @@ static QemuOptsList sd_create_opts = {
 };
 
 static BlockDriver bdrv_sheepdog = {
-.format_name= "sheepdog",
-.protocol_name  = "sheepdog",
-.instance_size  = sizeof(BDRVSheepdogState),
-.bdrv_parse_filename= sd_parse_filename,
-.bdrv_file_open = sd_open,
-.bdrv_reopen_prepare= sd_reopen_prepare,
-.bdrv_reopen_commit = sd_reopen_commit,
-.bdrv_reopen_abort  = sd_reopen_abort,
-.bdrv_close = sd_close,
-.bdrv_create= sd_create,
-.bdrv_has_zero_init = bdrv_has_zero_init_1,
-.bdrv_getlength = sd_getlength,
+.format_name  = "sheepdog",
+.protocol_name= "sheepdog",
+.instance_size= sizeof(BDRVSheepdogState),
+.bdrv_parse_filename  = sd_parse_filename,
+.bdrv_file_open   = sd_open,
+.bdrv_reopen_prepare  = sd_reopen_prepare,
+.bdrv_reopen_commit   = sd_reopen_commit,
+.bdrv_reopen_abort= sd_reopen_abort,
+.bdrv_close   = sd_close,
+.bdrv_create  = sd_create,
+.bdrv_has_zero_init   = bdrv_has_zero_init_1,
+.bdrv_getlength   = sd_getlength,
 .bdrv_get_allocated_file_size = sd_get_allocated_file_size,
-.bdrv_truncate  = sd_truncate,
+.bdrv_truncate= sd_truncate,
 
-.bdrv_co_readv  = sd_co_readv,
-.bdrv_co_writev = sd_co_writev,
-.bdrv_co_flush_to_disk  = sd_co_flush_to_disk,
-.bdrv_co_pdiscard = sd_co_pdiscard,
-.bdrv_co_get_block_status = sd_co_get_block_status,
+.bdrv_co_readv= sd_co_readv,
+.bdrv_co_writev   = sd_co_writev,
+.bdrv_co_flush_to_disk= sd_co_flush_to_disk,
+.bdrv_co_pdiscard = sd_co_pdiscard,
+.bdrv_co_get_block_status = sd_co_get_block_status,
 
-.bdrv_snapshot_create   = sd_snapshot_create,
-.bdrv_snapshot_goto = sd_snapshot_goto,
-.bdrv_snapshot_delete   = sd_snapshot_delete,
-.bdrv_snapshot_list = sd_snapshot_list,
+.bdrv_snapshot_create = sd_snapshot_create,
+.bdrv_snapshot_goto   = sd_snapshot_goto,
+.bdrv_snapshot_delete = sd_snapshot_delete,
+.bdrv_snapshot_list   = sd_snapshot_list,
 
-.bdrv_save_vmstate  = sd_save_vmstate,
-.bdrv_load_vmstate  = sd_load_vmstate,
+.bdrv_save_vmstate= sd_save_vmstate,
+.bdrv_load_vmstate= sd_load_vmstate,
 
-.bdrv_detach_aio_context = sd_detach_aio_context,
-.bdrv_attach_aio_context = sd_attach_aio_context,
+.bdrv_detach_aio_context  = sd_detach_aio_context,
+.bdrv_attach_aio_context  = sd_attach_aio_context,
 
-.create_opts= &sd_create_opts,
+.create_opts  = &sd_create_opts,
 };
 
 static BlockDriver bdrv_sheepdog_tcp = {
-.format_name= "sheepdog",
-.protocol_name  = "sheepdog+tcp",
-.instance_size  = sizeof(BDRVSheepdogState),
-.bdrv_parse_filename= sd_parse_filename,
+.format_name  = "sheepdog",
+.protocol_name= "sheepdog+tcp",
+.instance_size= sizeof(BDRVSheepdogState),
+.bdrv_parse_filename  = sd_parse_filename,
 .bdrv_file_open = sd_open,
-.bdrv_reopen_prepare= sd_reopen_prepare,
-.bdrv_reopen_commit = sd_reopen_commit,
-.bdrv_reopen_abort  = sd_reopen_abort,
-.bdrv_close = sd_close,
-.bdrv_create= sd_create,
-.bdrv_has_zero_init = bdrv_has_zero_init_1,
-.bdrv_getlength = sd_getlength,
+.bdrv_reopen_prepare  = sd_reopen_prepare,
+.bdrv_reopen_commit   = sd_reopen_commit,
+.bdrv_reopen_abort= sd_reopen_abort,
+.bdrv_close   = sd_close,
+.bdrv_create  = sd_create,
+.bdrv_has_zero_init   = bdrv_has_zero_init_1,
+.bdrv_getlength   = sd_getlength,
 .bdrv_get_allocated_file_size = sd_get_allocated_file_size,
-.bdrv_truncate  = sd_truncate,
+.bdrv_truncate= sd_truncate,
 
-.bdrv_co_readv  = sd_co_readv,
-.bdrv_co_writev = sd_co_writev,
-.bdrv_co_flush_to_disk  = sd_co_flush_to_disk,
-.bdrv_co_pdiscard = sd_co_pdiscard,
-.bdrv_co_get_block_status = sd_co_get_block_status,
+.bdrv_co_readv= sd_co_readv,
+.bdrv_co_writev   

[Qemu-block] [PATCH for-2.11 2/7] block/ssh: make compliant with coding guidelines

[Jeff Cody]

Signed-off-by: Jeff Cody 
---
 block/ssh.c | 32 ++--
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index cbb0e34..97f7673 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -241,7 +241,7 @@ static int parse_uri(const char *filename, QDict *options, 
Error **errp)
 goto err;
 }
 
-if(uri->user && strcmp(uri->user, "") != 0) {
+if (uri->user && strcmp(uri->user, "") != 0) {
 qdict_put_str(options, "user", uri->user);
 }
 
@@ -268,7 +268,7 @@ static int parse_uri(const char *filename, QDict *options, 
Error **errp)
 
  err:
 if (uri) {
-  uri_free(uri);
+uri_free(uri);
 }
 return -EINVAL;
 }
@@ -342,7 +342,7 @@ static int check_host_key_knownhosts(BDRVSSHState *s,
 libssh2_knownhost_readfile(knh, knh_file, LIBSSH2_KNOWNHOST_FILE_OPENSSH);
 
 r = libssh2_knownhost_checkp(knh, host, port, hostkey, len,
- LIBSSH2_KNOWNHOST_TYPE_PLAIN|
+ LIBSSH2_KNOWNHOST_TYPE_PLAIN |
  LIBSSH2_KNOWNHOST_KEYENC_RAW,
  &found);
 switch (r) {
@@ -405,15 +405,18 @@ static int compare_fingerprint(const unsigned char 
*fingerprint, size_t len,
 unsigned c;
 
 while (len > 0) {
-while (*host_key_check == ':')
+while (*host_key_check == ':') {
 host_key_check++;
+}
 if (!qemu_isxdigit(host_key_check[0]) ||
-!qemu_isxdigit(host_key_check[1]))
+!qemu_isxdigit(host_key_check[1])) {
 return 1;
+}
 c = hex2decimal(host_key_check[0]) * 16 +
 hex2decimal(host_key_check[1]);
-if (c - *fingerprint != 0)
+if (c - *fingerprint != 0) {
 return c - *fingerprint;
+}
 fingerprint++;
 len--;
 host_key_check += 2;
@@ -433,8 +436,8 @@ check_host_key_hash(BDRVSSHState *s, const char *hash,
 return -EINVAL;
 }
 
-if(compare_fingerprint((unsigned char *) fingerprint, fingerprint_len,
-   hash) != 0) {
+if (compare_fingerprint((unsigned char *) fingerprint, fingerprint_len,
+hash) != 0) {
 error_setg(errp, "remote host key does not match host_key_check '%s'",
hash);
 return -EPERM;
@@ -507,7 +510,7 @@ static int authenticate(BDRVSSHState *s, const char *user, 
Error **errp)
 goto out;
 }
 
-for(;;) {
+for (;;) {
 r = libssh2_agent_get_identity(agent, &identity, prev_identity);
 if (r == 1) {   /* end of list */
 break;
@@ -863,8 +866,8 @@ static int ssh_create(const char *filename, QemuOpts *opts, 
Error **errp)
 }
 
 r = connect_to_ssh(&s, uri_options,
-   LIBSSH2_FXF_READ|LIBSSH2_FXF_WRITE|
-   LIBSSH2_FXF_CREAT|LIBSSH2_FXF_TRUNC,
+   LIBSSH2_FXF_READ  | LIBSSH2_FXF_WRITE |
+   LIBSSH2_FXF_CREAT | LIBSSH2_FXF_TRUNC,
0644, errp);
 if (r < 0) {
 ret = r;
@@ -872,7 +875,7 @@ static int ssh_create(const char *filename, QemuOpts *opts, 
Error **errp)
 }
 
 if (total_size > 0) {
-libssh2_sftp_seek64(s.sftp_handle, total_size-1);
+libssh2_sftp_seek64(s.sftp_handle, total_size - 1);
 r2 = libssh2_sftp_write(s.sftp_handle, c, 1);
 if (r2 < 0) {
 sftp_error_setg(errp, &s, "truncate failed");
@@ -,7 +1114,7 @@ static int ssh_write(BDRVSSHState *s, BlockDriverState 
*bs,
  * works for me.
  */
 if (r == 0) {
-ssh_seek(s, offset + written, SSH_SEEK_WRITE|SSH_SEEK_FORCE);
+ssh_seek(s, offset + written, SSH_SEEK_WRITE | SSH_SEEK_FORCE);
 co_yield(s, bs);
 goto again;
 }
@@ -1125,8 +1128,9 @@ static int ssh_write(BDRVSSHState *s, BlockDriverState 
*bs,
 end_of_vec = i->iov_base + i->iov_len;
 }
 
-if (offset + written > s->attrs.filesize)
+if (offset + written > s->attrs.filesize) {
 s->attrs.filesize = offset + written;
+}
 }
 
 return 0;
-- 
2.9.4

Re: [Qemu-block] [PATCH v3 5/7] block: add throttle block filter driver

[Manos Pitsidianakis]

On Tue, Aug 08, 2017 at 03:13:36PM +0200, Alberto Garcia wrote:

On Mon 31 Jul 2017 11:54:41 AM CEST, Manos Pitsidianakis wrote:

block/throttle.c uses existing I/O throttle infrastructure inside a
block filter driver. I/O operations are intercepted in the filter's
read/write coroutines, and referred to block/throttle-groups.c

The driver can be used with the syntax
-drive driver=throttle,file.filename=foo.qcow2, \
limits.iops-total=...,throttle-group=bar


Sorry for not having noticed this earlier, but can't you define the
throttling group (and its limits) using -object throttle-group ... as
shown in the previous patch, and simply reference it here? Or would we
have two alternative ways of setting the throttling limits?

What happens if you have many -drive lines each one with a different set
of limits but with the same throttling group?


The limits of the last one to be processed will win. Quoting a reply I 
made to Kevin on the interface test patch:



You're right, I missed this. The test result shows that this command
succeeds. Do we really want to allow other nodes to be affected with a
blockdev-add? Wouldn't it be cleaner to just forbid the combination of
limits and throtte-group?


So basically only anonymous, immutable groups can be created through the 
driver then. All other shared group configurations must be explicitly 
created with an -object / object-add syntax. I think this is a neat 
separation and compromise if we allow anonymous groups. If not, we can 
ignore limits on the throttle driver.


So basically if we have anonymous groups, we accept limits in the driver 
options but only without a group-name. Without anonymous groups, we 
remove limits from the driver options and only use the 
object-add/-object commands to create throttle groups. Does this sound 
like a good idea? It will be more verbose for the human user. One 
advantage: all throttle groups can then be managed through 
qom-set/qom-get since they are owned by the qom tree.


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v3 for-2.10 0/4] improved --version/--help tweaks

[Eric Blake]

On 08/07/2017 07:08 PM, John Snow wrote:
> 
> 
> On 08/03/2017 12:33 PM, Eric Blake wrote:
>> Not sure if this should go through Kevin's block tree, Paolo's
>> miscellaneous patches, or if I should just do a pull request
>> myself (since patch 4 includes a change to qemu-nbd)
>>

> 
> Nothing to keep the commands from going out of order again, it looks
> like -- can this be added as a comment or otherwise scripted as a
> ./hey_it_looks_like_you_are_about_to_release_qemu.sh script that makes
> sure we've dotted the 'i's and crossed the 't's?

We don't add subcommands to qemu-img all that frequently; I think a
comment is okay without having to figure out where to script things.

> 
> Lastly, Didn't we fix the certificate issue? (I thought Jeff had) -- and
> even if not, it's still the correct address to send people to IMO. Let
> people report to us if the SSL certificate appears to be broken.

That can be a followup-patch.

> 
> Eh, regardless of shed coloring:
> 
> Reviewed-by: John Snow 
> 

-- 
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 v4] tests: Avoid non-portable 'echo -ARG'

[Kevin Wolf]

Am 03.07.2017 um 20:09 hat Eric Blake geschrieben:
> POSIX says that backslashes in the arguments to 'echo', as well as
> any use of 'echo -n' and 'echo -e', are non-portable; it recommends
> people should favor 'printf' instead.  This is definitely true where
> we do not control which shell is running (such as in makefile snippets
> or in documentation examples).  But even for scripts where we
> require bash (and therefore, where echo does what we want by default),
> it is still possible to use 'shopt -s xpg_echo' to change bash's
> behavior of echo.  And setting a good example never hurts when we are
> not sure if a snippet will be copied from a bash-only script to a
> general shell script (although I don't change the use of non-portable
> \e for ESC when we know the running shell is bash).
> 
> Replace 'echo -n "..."' with 'printf %s "..."', and 'echo -e "..."'
> with 'printf %b "...\n"', with the optimization that the %s/%b
> argument can be omitted if the string being printed is a strict
> literal with no '%', '$', or '`' (we could technically also make
> this optimization when there are $ or `` substitutions but where
> we can prove their results will not be problematic, but proving
> that such substitutions are safe makes the patch less trivial
> compared to just being consistent).
> 
> In the qemu-iotests check script, fix unusual shell quoting
> that would result in word-splitting if 'date' outputs a space.
> 
> In test 051, take an opportunity to shorten the line.
> 
> In test 068, get rid of a pointless second invocation of bash.
> 
> CC: qemu-triv...@nongnu.org
> Signed-off-by: Eric Blake 

> diff --git a/tests/multiboot/run_test.sh b/tests/multiboot/run_test.sh
> index 78d7edf..c8f3da8 100755
> --- a/tests/multiboot/run_test.sh
> +++ b/tests/multiboot/run_test.sh
> @@ -26,7 +26,7 @@ run_qemu() {
>  local kernel=$1
>  shift
> 
> -echo -e "\n\n=== Running test case: $kernel $@ ===\n" >> test.log
> +printf %b "\n\n=== Running test case: $kernel $@ ===\n\n" >> test.log
> 
>  $QEMU \
>  -kernel $kernel \

Not completely sure why, but this broke the test with whitespace changes
like this:

-=== Running test case: mmap.elf -m 1.1M ===
+=== Running test case: mmap.elf -m1.1M ===

Kevin

[Qemu-block] [PULL 00/18] Block layer patches for 2.10.0-rc2

[Kevin Wolf]

The following changes since commit b4174c4b08a719e7df7e4f35c29f44b7c2517237:

  Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging 
(2017-08-08 10:01:49 +0100)

are available in the git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 113fe792fd4931dd0538f03859278b8719ee4fa2:

  block/nfs: fix mutex assertion in nfs_file_close() (2017-08-08 15:19:16 +0200)


Block layer patches for 2.10.0-rc2


Alberto Garcia (1):
  quorum: Set sectors-count to 0 when reporting a flush error

Cleber Rosa (1):
  qemu-iotests/109: Fix lock race condition

Denis V. Lunev (3):
  block: respect error code from bdrv_getlength in handle_aiocb_write_zeroes
  parallels: respect error code of bdrv_getlength() in allocate_clusters()
  parallels: drop check that bdrv_truncate() is working

Fam Zheng (1):
  vmdk: Fix error handling/reporting of vmdk_check

Jeff Cody (5):
  block/vhdx: check error return of bdrv_getlength()
  block/vhdx: check for offset overflow to bdrv_truncate()
  block/vhdx: check error return of bdrv_flush()
  block/vhdx: check error return of bdrv_truncate()
  block/nfs: fix mutex assertion in nfs_file_close()

Kevin Wolf (6):
  block/null: Remove 'filename' option
  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

Paolo Bonzini (1):
  block: drop bdrv_set_key from BlockDriver

 include/block/block.h  |  3 +-
 include/block/block_int.h  |  1 -
 block.c| 20 +-
 block/file-posix.c |  8 +-
 block/nfs.c| 11 ++--
 block/null.c   | 31 +
 block/parallels.c  | 12 
 block/quorum.c |  3 +-
 block/vhdx-log.c   | 52 +++---
 block/vhdx.c   | 12 +++-
 block/vmdk.c   | 26 +++--
 qemu-io-cmds.c | 19 +++--
 tests/qemu-iotests/109 |  3 +-
 tests/qemu-iotests/109.out | 56 +
 tests/qemu-iotests/136 |  2 +-
 tests/qemu-iotests/187 | 69 ++
 tests/qemu-iotests/187.out | 18 
 tests/qemu-iotests/group   |  1 +
 18 files changed, 299 insertions(+), 48 deletions(-)
 create mode 100755 tests/qemu-iotests/187
 create mode 100644 tests/qemu-iotests/187.out

[Qemu-block] [PULL 02/18] quorum: Set sectors-count to 0 when reporting a flush error

[Kevin Wolf]

From: Alberto Garcia 

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 
Reviewed-by: Eric Blake 
Signed-off-by: Kevin Wolf 
---
 block/quorum.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index 55ba916655..d04da4f430 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -785,8 +785,7 @@ static coroutine_fn int quorum_co_flush(BlockDriverState 
*bs)
 for (i = 0; i < s->num_children; i++) {
 result = bdrv_co_flush(s->children[i]->bs);
 if (result) {
-quorum_report_bad(QUORUM_OP_TYPE_FLUSH, 0,
-  bdrv_getlength(s->children[i]->bs),
+quorum_report_bad(QUORUM_OP_TYPE_FLUSH, 0, 0,
   s->children[i]->bs->node_name, result);
 result_value.l = result;
 quorum_count_vote(&error_votes, &result_value, i);
-- 
2.13.4

[Qemu-block] [PULL 03/18] block/vhdx: check error return of bdrv_getlength()

[Kevin Wolf]

From: Jeff Cody 

Calls to bdrv_getlength() were not checking for error.  In vhdx.c, this
can lead to truncating an image file, so it is a definite bug.  In
vhdx-log.c, the path for improper behavior is less clear, but it is best
to check in any case.

Some minor code movement of the log_guid intialization, as well.

Reported-by: Markus Armbruster 
Reviewed-by: Eric Blake 
Signed-off-by: Jeff Cody 
Signed-off-by: Kevin Wolf 
---
 block/vhdx-log.c | 23 ++-
 block/vhdx.c |  9 -
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 01278f3fc9..2e26fd46a5 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -491,6 +491,7 @@ static int vhdx_log_flush(BlockDriverState *bs, 
BDRVVHDXState *s,
 uint32_t cnt, sectors_read;
 uint64_t new_file_size;
 void *data = NULL;
+int64_t file_length;
 VHDXLogDescEntries *desc_entries = NULL;
 VHDXLogEntryHeader hdr_tmp = { 0 };
 
@@ -510,10 +511,15 @@ static int vhdx_log_flush(BlockDriverState *bs, 
BDRVVHDXState *s,
 if (ret < 0) {
 goto exit;
 }
+file_length = bdrv_getlength(bs->file->bs);
+if (file_length < 0) {
+ret = file_length;
+goto exit;
+}
 /* if the log shows a FlushedFileOffset larger than our current file
  * size, then that means the file has been truncated / corrupted, and
  * we must refused to open it / use it */
-if (hdr_tmp.flushed_file_offset > bdrv_getlength(bs->file->bs)) {
+if (hdr_tmp.flushed_file_offset > file_length) {
 ret = -EINVAL;
 goto exit;
 }
@@ -543,7 +549,7 @@ static int vhdx_log_flush(BlockDriverState *bs, 
BDRVVHDXState *s,
 goto exit;
 }
 }
-if (bdrv_getlength(bs->file->bs) < desc_entries->hdr.last_file_offset) 
{
+if (file_length < desc_entries->hdr.last_file_offset) {
 new_file_size = desc_entries->hdr.last_file_offset;
 if (new_file_size % (1024*1024)) {
 /* round up to nearest 1MB boundary */
@@ -851,6 +857,7 @@ static int vhdx_log_write(BlockDriverState *bs, 
BDRVVHDXState *s,
 uint32_t partial_sectors = 0;
 uint32_t bytes_written = 0;
 uint64_t file_offset;
+int64_t file_length;
 VHDXHeader *header;
 VHDXLogEntryHeader new_hdr;
 VHDXLogDescriptor *new_desc = NULL;
@@ -904,6 +911,12 @@ static int vhdx_log_write(BlockDriverState *bs, 
BDRVVHDXState *s,
 
 sectors += partial_sectors;
 
+file_length = bdrv_getlength(bs->file->bs);
+if (file_length < 0) {
+ret = file_length;
+goto exit;
+}
+
 /* sectors is now how many sectors the data itself takes, not
  * including the header and descriptor metadata */
 
@@ -913,11 +926,11 @@ static int vhdx_log_write(BlockDriverState *bs, 
BDRVVHDXState *s,
 .sequence_number = s->log.sequence,
 .descriptor_count= sectors,
 .reserved= 0,
-.flushed_file_offset = bdrv_getlength(bs->file->bs),
-.last_file_offset= bdrv_getlength(bs->file->bs),
+.flushed_file_offset = file_length,
+.last_file_offset= file_length,
+.log_guid= header->log_guid,
   };
 
-new_hdr.log_guid = header->log_guid;
 
 desc_sectors = vhdx_compute_desc_sectors(new_hdr.descriptor_count);
 
diff --git a/block/vhdx.c b/block/vhdx.c
index a9cecd2773..37224b8858 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1166,7 +1166,14 @@ exit:
 static int vhdx_allocate_block(BlockDriverState *bs, BDRVVHDXState *s,
 uint64_t *new_offset)
 {
-*new_offset = bdrv_getlength(bs->file->bs);
+int64_t current_len;
+
+current_len = bdrv_getlength(bs->file->bs);
+if (current_len < 0) {
+return current_len;
+}
+
+*new_offset = current_len;
 
 /* per the spec, the address for a block is in units of 1MB */
 *new_offset = ROUND_UP(*new_offset, 1024 * 1024);
-- 
2.13.4

[Qemu-block] [PULL 04/18] block/vhdx: check for offset overflow to bdrv_truncate()

[Kevin Wolf]

From: Jeff Cody 

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é 
Signed-off-by: Kevin Wolf 
---
 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 2e26fd46a5..95972230f0 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 37224b8858..7ae4589879 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.13.4

[Qemu-block] [PULL 05/18] block/vhdx: check error return of bdrv_flush()

[Kevin Wolf]

From: Jeff Cody 

Reported-by: Kevin Wolf 
Signed-off-by: Jeff Cody 
Reviewed-by: Eric Blake 
Signed-off-by: Kevin Wolf 
---
 block/vhdx-log.c | 17 ++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 95972230f0..a27dc059cd 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -565,7 +565,10 @@ static int vhdx_log_flush(BlockDriverState *bs, 
BDRVVHDXState *s,
 desc_entries = NULL;
 }
 
-bdrv_flush(bs);
+ret = bdrv_flush(bs);
+if (ret < 0) {
+goto exit;
+}
 /* once the log is fully flushed, indicate that we have an empty log
  * now.  This also sets the log guid to 0, to indicate an empty log */
 vhdx_log_reset(bs, s);
@@ -1039,7 +1042,11 @@ int vhdx_log_write_and_flush(BlockDriverState *bs, 
BDRVVHDXState *s,
 
 /* Make sure data written (new and/or changed blocks) is stable
  * on disk, before creating log entry */
-bdrv_flush(bs);
+ret = bdrv_flush(bs);
+if (ret < 0) {
+goto exit;
+}
+
 ret = vhdx_log_write(bs, s, data, length, offset);
 if (ret < 0) {
 goto exit;
@@ -1047,7 +1054,11 @@ int vhdx_log_write_and_flush(BlockDriverState *bs, 
BDRVVHDXState *s,
 logs.log = s->log;
 
 /* Make sure log is stable on disk */
-bdrv_flush(bs);
+ret = bdrv_flush(bs);
+if (ret < 0) {
+goto exit;
+}
+
 ret = vhdx_log_flush(bs, s, &logs);
 if (ret < 0) {
 goto exit;
-- 
2.13.4

[Qemu-block] [PULL 01/18] qemu-iotests/109: Fix lock race condition

[Kevin Wolf]

From: Cleber Rosa 

A race condition is currently present between the clean up attempt of
the QEMU process and the execution of qemu-img.  The actual (bad)
output is:

 -Warning: Image size mismatch!
 -Images are identical.
 +qemu-img: Could not open '/tests/qemu-iotests/scratch/t.raw': 
Failed to get "consistent read" lock
 +Is another process using the image?

A KILL signal is sent to the QEMU process, but qemu-img may begin to
run before the QEMU process is really gone.  qemu-img will then
attempt to open the TEST_IMG file before it can secure a lock on it.

This attempts a more graceful shutdown, and waits for the QEMU process
to exit.

Signed-off-by: Cleber Rosa 
Reviewed-by: Eric Blake 
Reviewed-by: Jeff Cody 
Reviewed-by: John Snow 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/109 |  3 ++-
 tests/qemu-iotests/109.out | 56 ++
 2 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/109 b/tests/qemu-iotests/109
index 3b496a3918..d70b574d88 100755
--- a/tests/qemu-iotests/109
+++ b/tests/qemu-iotests/109
@@ -67,7 +67,8 @@ function run_qemu()
 _send_qemu_cmd $QEMU_HANDLE '' "BLOCK_JOB_COMPLETED"
 fi
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"query-block-jobs"}' "return"
-_cleanup_qemu
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"quit"}' "return"
+wait=1 _cleanup_qemu
 }
 
 for fmt in qcow qcow2 qed vdi vmdk vpc; do
diff --git a/tests/qemu-iotests/109.out b/tests/qemu-iotests/109.out
index dc02f9eefa..c189e2833d 100644
--- a/tests/qemu-iotests/109.out
+++ b/tests/qemu-iotests/109.out
@@ -12,12 +12,17 @@ Specify the 'raw' format explicitly to remove the 
restrictions.
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": 
"report"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": LEN, "offset": 0, 
"speed": 0, "type": "mirror", "error": "Operation not permitted"}}
 {"return": []}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"SHUTDOWN", "data": {"guest": false}}
 read 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 {"return": {}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_READY", "data": {"device": "src", "len": 1024, "offset": 1024, 
"speed": 0, "type": "mirror"}}
 {"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 1024, 
"offset": 1024, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"SHUTDOWN", "data": {"guest": false}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 1024, "offset": 1024, 
"speed": 0, "type": "mirror"}}
 Warning: Image size mismatch!
 Images are identical.
 
@@ -33,12 +38,17 @@ Specify the 'raw' format explicitly to remove the 
restrictions.
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": 
"report"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": LEN, "offset": 512, 
"speed": 0, "type": "mirror", "error": "Operation not permitted"}}
 {"return": []}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"SHUTDOWN", "data": {"guest": false}}
 read 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 {"return": {}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_READY", "data": {"device": "src", "len": 197120, "offset": 197120, 
"speed": 0, "type": "mirror"}}
 {"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 197120, 
"offset": 197120, "paused": false, "speed": 0, "ready": true, "type": 
"mirror"}]}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"SHUTDOWN", "data": {"guest": false}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 197120, "offset": 
197120, "speed": 0, "type": "mirror"}}
 Warning: Image size mismatch!
 Images are identical.
 
@@ -54,12 +64,17 @@ Specify the 'raw' format explicitly to remove the 
restrictions.
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": 
"report"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": LEN, "offset": 262144, 
"speed": 0, "type": "mirror", "

[Qemu-block] [PULL 11/18] parallels: respect error code of bdrv_getlength() in allocate_clusters()

[Kevin Wolf]

From: "Denis V. Lunev" 

If we can not get the file length, the state of BDS is broken completely.
Return error to the caller.

Signed-off-by: Denis V. Lunev 
CC: Markus Armbruster 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 
Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
---
 block/parallels.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 5bbdfabb7a..6794e53c0b 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -192,7 +192,7 @@ static int64_t allocate_clusters(BlockDriverState *bs, 
int64_t sector_num,
  int nb_sectors, int *pnum)
 {
 BDRVParallelsState *s = bs->opaque;
-int64_t pos, space, idx, to_allocate, i;
+int64_t pos, space, idx, to_allocate, i, len;
 
 pos = block_status(s, sector_num, nb_sectors, pnum);
 if (pos > 0) {
@@ -214,7 +214,11 @@ static int64_t allocate_clusters(BlockDriverState *bs, 
int64_t sector_num,
 assert(idx < s->bat_size && idx + to_allocate <= s->bat_size);
 
 space = to_allocate * s->tracks;
-if (s->data_end + space > bdrv_getlength(bs->file->bs) >> 
BDRV_SECTOR_BITS) {
+len = bdrv_getlength(bs->file->bs);
+if (len < 0) {
+return len;
+}
+if (s->data_end + space > (len >> BDRV_SECTOR_BITS)) {
 int ret;
 space += s->prealloc_size;
 if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
-- 
2.13.4

[Qemu-block] [PULL 07/18] block: drop bdrv_set_key from BlockDriver

[Kevin Wolf]

From: Paolo Bonzini 

This is not used anymore since c01c214b69 ("block: remove all encryption
handling APIs", 2017-07-11).

Signed-off-by: Paolo Bonzini 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Eric Blake 
Signed-off-by: Kevin Wolf 
---
 include/block/block_int.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index d4f4ea7584..7571c0aaaf 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -127,7 +127,6 @@ struct BlockDriver {
   Error **errp);
 void (*bdrv_close)(BlockDriverState *bs);
 int (*bdrv_create)(const char *filename, QemuOpts *opts, Error **errp);
-int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
 int (*bdrv_make_empty)(BlockDriverState *bs);
 
 void (*bdrv_refresh_filename)(BlockDriverState *bs, QDict *options);
-- 
2.13.4

[Qemu-block] [PULL 06/18] block/vhdx: check error return of bdrv_truncate()

[Kevin Wolf]

From: Jeff Cody 

Signed-off-by: Jeff Cody 
Reviewed-by: Eric Blake 
Signed-off-by: Kevin Wolf 
---
 block/vhdx-log.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index a27dc059cd..14b724ef7b 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);
-- 
2.13.4

[Qemu-block] [PULL 08/18] block/null: Remove 'filename' option

[Kevin Wolf]

This option was only added to allow 'null-co://' and 'null-aio://' as
filenames, its value never served any actual purpose and was ignored.
Nevertheless it was accepted as '-drive driver=null,filename=foo'.

The correct way to enable the protocol prefixes (and that without adding
a useless -drive option) is implementing .bdrv_parse_filename. This is
what this patch does.

Technically, this is an incompatible change, but the null block driver
is only used for benchmarking, testing and debugging, and an option
without effect isn't likely to be used by anyone anyway, so no bad
effects are to be expected.

Reported-by: Markus Armbruster 
Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
Reviewed-by: Jeff Cody 
Reviewed-by: Stefan Hajnoczi 
---
 block/null.c   | 31 ++-
 tests/qemu-iotests/136 |  2 +-
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/block/null.c b/block/null.c
index 876f90965b..dd9c13f9ba 100644
--- a/block/null.c
+++ b/block/null.c
@@ -30,11 +30,6 @@ static QemuOptsList runtime_opts = {
 .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
 .desc = {
 {
-.name = "filename",
-.type = QEMU_OPT_STRING,
-.help = "",
-},
-{
 .name = BLOCK_OPT_SIZE,
 .type = QEMU_OPT_SIZE,
 .help = "size of the null block",
@@ -54,6 +49,30 @@ static QemuOptsList runtime_opts = {
 },
 };
 
+static void null_co_parse_filename(const char *filename, QDict *options,
+   Error **errp)
+{
+/* This functions only exists so that a null-co:// filename is accepted
+ * with the null-co driver. */
+if (strcmp(filename, "null-co://")) {
+error_setg(errp, "The only allowed filename for this driver is "
+ "'null-co://'");
+return;
+}
+}
+
+static void null_aio_parse_filename(const char *filename, QDict *options,
+Error **errp)
+{
+/* This functions only exists so that a null-aio:// filename is accepted
+ * with the null-aio driver. */
+if (strcmp(filename, "null-aio://")) {
+error_setg(errp, "The only allowed filename for this driver is "
+ "'null-aio://'");
+return;
+}
+}
+
 static int null_file_open(BlockDriverState *bs, QDict *options, int flags,
   Error **errp)
 {
@@ -242,6 +261,7 @@ static BlockDriver bdrv_null_co = {
 .instance_size  = sizeof(BDRVNullState),
 
 .bdrv_file_open = null_file_open,
+.bdrv_parse_filename= null_co_parse_filename,
 .bdrv_close = null_close,
 .bdrv_getlength = null_getlength,
 
@@ -261,6 +281,7 @@ static BlockDriver bdrv_null_aio = {
 .instance_size  = sizeof(BDRVNullState),
 
 .bdrv_file_open = null_file_open,
+.bdrv_parse_filename= null_aio_parse_filename,
 .bdrv_close = null_close,
 .bdrv_getlength = null_getlength,
 
diff --git a/tests/qemu-iotests/136 b/tests/qemu-iotests/136
index 635b977552..4b994897af 100644
--- a/tests/qemu-iotests/136
+++ b/tests/qemu-iotests/136
@@ -75,7 +75,7 @@ sector = "%d"
 drive_args.append("stats-account-failed=%s" %
   (self.account_failed and "on" or "off"))
 self.create_blkdebug_file()
-self.vm = iotests.VM().add_drive('blkdebug:%s:%s ' %
+self.vm = iotests.VM().add_drive('blkdebug:%s:%s' %
  (blkdebug_file, self.test_img),
  ','.join(drive_args))
 self.vm.launch()
-- 
2.13.4

[Qemu-block] [PULL 09/18] vmdk: Fix error handling/reporting of vmdk_check

[Kevin Wolf]

From: Fam Zheng 

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 
Reviewed-by: Eric Blake 
Signed-off-by: Kevin Wolf 
---
 block/vmdk.c | 26 ++
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 0fc97391a6..c665bcc977 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2236,6 +2236,7 @@ static int vmdk_check(BlockDriverState *bs, 
BdrvCheckResult *result,
 fprintf(stderr,
 "ERROR: could not find extent for sector %" PRId64 "\n",
 sector_num);
+ret = -EINVAL;
 break;
 }
 ret = get_cluster_offset(bs, extent, NULL,
@@ -2247,19 +2248,28 @@ static int vmdk_check(BlockDriverState *bs, 
BdrvCheckResult *result,
 PRId64 "\n", sector_num);
 break;
 }
-if (ret == VMDK_OK &&
-cluster_offset >= bdrv_getlength(extent->file->bs))
-{
-fprintf(stderr,
-"ERROR: cluster offset for sector %"
-PRId64 " points after EOF\n", sector_num);
-break;
+if (ret == VMDK_OK) {
+int64_t extent_len = bdrv_getlength(extent->file->bs);
+if (extent_len < 0) {
+fprintf(stderr,
+"ERROR: could not get extent file length for sector %"
+PRId64 "\n", sector_num);
+ret = extent_len;
+break;
+}
+if (cluster_offset >= extent_len) {
+fprintf(stderr,
+"ERROR: cluster offset for sector %"
+PRId64 " points after EOF\n", sector_num);
+ret = -EINVAL;
+break;
+}
 }
 sector_num += extent->cluster_sectors;
 }
 
 result->corruptions++;
-return 0;
+return ret;
 }
 
 static ImageInfoSpecific *vmdk_get_specific_info(BlockDriverState *bs)
-- 
2.13.4

[Qemu-block] [PULL 10/18] block: respect error code from bdrv_getlength in handle_aiocb_write_zeroes

[Kevin Wolf]

From: "Denis V. Lunev" 

Original idea beyond the code in question was the following: we have failed
to write zeroes with fallocate(FALLOC_FL_ZERO_RANGE) as the simplest
approach and via fallocate(FALLOC_FL_PUNCH_HOLE)/fallocate(0). We have the
only chance now: if the request comes beyond end of the file. Thus we
should calculate file length and respect the error code from that op.

Signed-off-by: Denis V. Lunev 
CC: Markus Armbruster 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 
Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
---
 block/file-posix.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index cfbb236f6f..f4de022ae0 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1339,6 +1339,9 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData 
*aiocb)
 #if defined(CONFIG_FALLOCATE) || defined(CONFIG_XFS)
 BDRVRawState *s = aiocb->bs->opaque;
 #endif
+#ifdef CONFIG_FALLOCATE
+int64_t len;
+#endif
 
 if (aiocb->aio_type & QEMU_AIO_BLKDEV) {
 return handle_aiocb_write_zeroes_block(aiocb);
@@ -1381,7 +1384,10 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData 
*aiocb)
 #endif
 
 #ifdef CONFIG_FALLOCATE
-if (s->has_fallocate && aiocb->aio_offset >= bdrv_getlength(aiocb->bs)) {
+/* Last resort: we are trying to extend the file with zeroed data. This
+ * can be done via fallocate(fd, 0) */
+len = bdrv_getlength(aiocb->bs);
+if (s->has_fallocate && len >= 0 && aiocb->aio_offset >= len) {
 int ret = do_fallocate(s->fd, 0, aiocb->aio_offset, aiocb->aio_nbytes);
 if (ret == 0 || ret != -ENOTSUP) {
 return ret;
-- 
2.13.4

[Qemu-block] [PULL 12/18] parallels: drop check that bdrv_truncate() is working

[Kevin Wolf]

From: "Denis V. Lunev" 

This would be actually strange and error prone. If truncate() nowadays
will fail, there is something fatally wrong. Let's check for that during
the actual work.

The only fallback case is when the file is not zero initialized. In this
case we should switch to preallocation via fallocate().

Signed-off-by: Denis V. Lunev 
CC: Markus Armbruster 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 
Signed-off-by: Kevin Wolf 
---
 block/parallels.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 6794e53c0b..e1e06d23cc 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -703,9 +703,7 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 goto fail_options;
 }
 
-if (!(flags & BDRV_O_RESIZE) || !bdrv_has_zero_init(bs->file->bs) ||
-bdrv_truncate(bs->file, bdrv_getlength(bs->file->bs),
-  PREALLOC_MODE_OFF, NULL) != 0) {
+if (!bdrv_has_zero_init(bs->file->bs)) {
 s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE;
 }
 
-- 
2.13.4

[Qemu-block] [PULL 15/18] block: Set BDRV_O_ALLOW_RDWR during rw reopen

[Kevin Wolf]

Reopening an image should be consistent with opening it, so we should
set BDRV_O_ALLOW_RDWR for any image that is reopened read-write like in
bdrv_open_inherit().

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
Reviewed-by: Jeff Cody 
Reviewed-by: John Snow 
---
 block.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 2711c3dd3b..3615a6809e 100644
--- a/block.c
+++ b/block.c
@@ -2729,8 +2729,11 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 bdrv_join_options(bs, options, old_options);
 QDECREF(old_options);
 
-/* bdrv_open() masks this flag out */
+/* bdrv_open_inherit() sets and clears some additional flags internally */
 flags &= ~BDRV_O_PROTOCOL;
+if (flags & BDRV_O_RDWR) {
+flags |= BDRV_O_ALLOW_RDWR;
+}
 
 QLIST_FOREACH(child, &bs->children, next) {
 QDict *new_child_options;
-- 
2.13.4

[Qemu-block] [PULL 13/18] block: Fix order in bdrv_replace_child()

[Kevin Wolf]

Commit 8ee03995 refactored the code incorrectly and broke the release of
permissions on the old BDS. Instead of changing the permissions to the
new required values after removing the old BDS from the list of
children, it only re-obtains the permissions it already had.

Change the order of operations so that the old BDS is removed again
before calculating the new required permissions.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
Reviewed-by: Jeff Cody 
Reviewed-by: John Snow 
---
 block.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index ce9cce7b3c..ab908cdc50 100644
--- a/block.c
+++ b/block.c
@@ -1933,6 +1933,8 @@ static void bdrv_replace_child(BdrvChild *child, 
BlockDriverState *new_bs)
 BlockDriverState *old_bs = child->bs;
 uint64_t perm, shared_perm;
 
+bdrv_replace_child_noperm(child, new_bs);
+
 if (old_bs) {
 /* Update permissions for old node. This is guaranteed to succeed
  * because we're just taking a parent away, so we're loosening
@@ -1942,8 +1944,6 @@ static void bdrv_replace_child(BdrvChild *child, 
BlockDriverState *new_bs)
 bdrv_set_perm(old_bs, perm, shared_perm);
 }
 
-bdrv_replace_child_noperm(child, new_bs);
-
 if (new_bs) {
 bdrv_get_cumulative_perm(new_bs, &perm, &shared_perm);
 bdrv_set_perm(new_bs, perm, shared_perm);
-- 
2.13.4

[Qemu-block] [PULL 16/18] qemu-io: Allow reopen read-write

[Kevin Wolf]

This allows qemu-iotests to test the switch between read-only and
read-write mode for block devices.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
Reviewed-by: Jeff Cody 
Reviewed-by: John Snow 
---
 qemu-io-cmds.c | 19 +--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 3eb42c6728..2811a89099 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1920,6 +1920,7 @@ static void reopen_help(void)
 " 'reopen -o lazy-refcounts=on' - activates lazy refcount writeback on a qcow2 
image\n"
 "\n"
 " -r, -- Reopen the image read-only\n"
+" -w, -- Reopen the image read-write\n"
 " -c, -- Change the cache mode to the given value\n"
 " -o, -- Changes block driver options (cf. 'open' command)\n"
 "\n");
@@ -1942,7 +1943,7 @@ static const cmdinfo_t reopen_cmd = {
.argmin = 0,
.argmax = -1,
.cfunc  = reopen_f,
-   .args   = "[-r] [-c cache] [-o options]",
+   .args   = "[(-r|-w)] [-c cache] [-o options]",
.oneline= "reopens an image with new options",
.help   = reopen_help,
 };
@@ -1955,11 +1956,12 @@ static int reopen_f(BlockBackend *blk, int argc, char 
**argv)
 int c;
 int flags = bs->open_flags;
 bool writethrough = !blk_enable_write_cache(blk);
+bool has_rw_option = false;
 
 BlockReopenQueue *brq;
 Error *local_err = NULL;
 
-while ((c = getopt(argc, argv, "c:o:r")) != -1) {
+while ((c = getopt(argc, argv, "c:o:rw")) != -1) {
 switch (c) {
 case 'c':
 if (bdrv_parse_cache_mode(optarg, &flags, &writethrough) < 0) {
@@ -1974,7 +1976,20 @@ static int reopen_f(BlockBackend *blk, int argc, char 
**argv)
 }
 break;
 case 'r':
+if (has_rw_option) {
+error_report("Only one -r/-w option may be given");
+return 0;
+}
 flags &= ~BDRV_O_RDWR;
+has_rw_option = true;
+break;
+case 'w':
+if (has_rw_option) {
+error_report("Only one -r/-w option may be given");
+return 0;
+}
+flags |= BDRV_O_RDWR;
+has_rw_option = true;
 break;
 default:
 qemu_opts_reset(&reopen_opts);
-- 
2.13.4

[Qemu-block] [PULL 14/18] block: Allow reopen rw without BDRV_O_ALLOW_RDWR

[Kevin Wolf]

BDRV_O_ALLOW_RDWR is a flag that tells whether qemu can internally
reopen a node read-write temporarily because the user requested
read-write for the top-level image, but qemu decided that read-only is
enough for this node (a backing file).

bdrv_reopen() is different, it is also used for cases where the user
changed their mind and wants to update the options. There is no reason
to forbid making a node read-write in that case.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
Reviewed-by: Jeff Cody 
Reviewed-by: John Snow 
---
 include/block/block.h |  3 ++-
 block.c   | 11 +++
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 34770bb33a..ab80195378 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -436,7 +436,8 @@ int bdrv_is_allocated_above(BlockDriverState *top, 
BlockDriverState *base,
 
 bool bdrv_is_read_only(BlockDriverState *bs);
 bool bdrv_is_writable(BlockDriverState *bs);
-int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
+int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
+   bool ignore_allow_rdw, Error **errp);
 int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
 bool bdrv_is_sg(BlockDriverState *bs);
 bool bdrv_is_inserted(BlockDriverState *bs);
diff --git a/block.c b/block.c
index ab908cdc50..2711c3dd3b 100644
--- a/block.c
+++ b/block.c
@@ -246,7 +246,8 @@ bool bdrv_is_writable(BlockDriverState *bs)
 return !bdrv_is_read_only(bs) && !(bs->open_flags & BDRV_O_INACTIVE);
 }
 
-int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
+int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
+   bool ignore_allow_rdw, Error **errp)
 {
 /* Do not set read_only if copy_on_read is enabled */
 if (bs->copy_on_read && read_only) {
@@ -256,7 +257,9 @@ int bdrv_can_set_read_only(BlockDriverState *bs, bool 
read_only, Error **errp)
 }
 
 /* Do not clear read_only if it is prohibited */
-if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR)) {
+if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR) &&
+!ignore_allow_rdw)
+{
 error_setg(errp, "Node '%s' is read only",
bdrv_get_device_or_node_name(bs));
 return -EPERM;
@@ -269,7 +272,7 @@ int bdrv_set_read_only(BlockDriverState *bs, bool 
read_only, Error **errp)
 {
 int ret = 0;
 
-ret = bdrv_can_set_read_only(bs, read_only, errp);
+ret = bdrv_can_set_read_only(bs, read_only, false, errp);
 if (ret < 0) {
 return ret;
 }
@@ -2907,7 +2910,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
BlockReopenQueue *queue,
  * to r/w. Attempting to set to r/w may fail if either BDRV_O_ALLOW_RDWR is
  * not set, or if the BDS still has copy_on_read enabled */
 read_only = !(reopen_state->flags & BDRV_O_RDWR);
-ret = bdrv_can_set_read_only(reopen_state->bs, read_only, &local_err);
+ret = bdrv_can_set_read_only(reopen_state->bs, read_only, true, 
&local_err);
 if (local_err) {
 error_propagate(errp, local_err);
 goto error;
-- 
2.13.4

[Qemu-block] [PULL 18/18] block/nfs: fix mutex assertion in nfs_file_close()

[Kevin Wolf]

From: 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.

Cc: qemu-sta...@nongnu.org
Signed-off-by: Jeff Cody 
Reviewed-by: Peter Lieven 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: John Snow 
Signed-off-by: Kevin Wolf 
---
 block/nfs.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/block/nfs.c b/block/nfs.c
index d8db419957..bec16b72a6 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));
+g_free(client->path);
+qemu_mutex_destroy(&client->mutex);
+qapi_free_NFSServer(client->server);
+client->server = NULL;
 }
 
 static void nfs_file_close(BlockDriverState *bs)
 {
 NFSClient *client = bs->opaque;
 nfs_client_close(client);
-qemu_mutex_destroy(&client->mutex);
 }
 
 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(&client->mutex);
 opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
 qemu_opts_absorb_qdict(opts, options, &local_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(&client->mutex);
+
 bs->total_sectors = ret;
 ret = 0;
 return ret;
-- 
2.13.4

[Qemu-block] [PULL 17/18] qemu-iotests: Test reopen between read-only and read-write

[Kevin Wolf]

This serves as a regression test for the bugs that were just fixed for
bdrv_reopen() between read-only and read-write mode.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
Reviewed-by: Jeff Cody 
Reviewed-by: John Snow 
---
 tests/qemu-iotests/187 | 69 ++
 tests/qemu-iotests/187.out | 18 
 tests/qemu-iotests/group   |  1 +
 3 files changed, 88 insertions(+)
 create mode 100755 tests/qemu-iotests/187
 create mode 100644 tests/qemu-iotests/187.out

diff --git a/tests/qemu-iotests/187 b/tests/qemu-iotests/187
new file mode 100755
index 00..7bb783363c
--- /dev/null
+++ b/tests/qemu-iotests/187
@@ -0,0 +1,69 @@
+#!/bin/bash
+#
+# Test switching between read-only and read-write
+#
+# Copyright (C) 2017 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=kw...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+rm -f "$TEST_IMG.2"
+rm -f "$TEST_IMG.3"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+size=64M
+_make_test_img $size
+
+echo
+echo "Start from read-only"
+echo
+
+$QEMU_IO -r -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -r -c 'reopen -w' -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -r -c 'reopen -w' -c 'reopen -r' -c 'write 0 64k' $TEST_IMG | 
_filter_qemu_io
+
+echo
+echo "Start from read-write"
+echo
+
+$QEMU_IO -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -c 'reopen -r' -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -c 'reopen -r' -c 'reopen -w' -c 'write 0 64k' $TEST_IMG | 
_filter_qemu_io
+
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/187.out b/tests/qemu-iotests/187.out
new file mode 100644
index 00..68fb944cd5
--- /dev/null
+++ b/tests/qemu-iotests/187.out
@@ -0,0 +1,18 @@
+QA output created by 187
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+
+Start from read-only
+
+Block node is read-only
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Block node is read-only
+
+Start from read-write
+
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+write failed: Operation not permitted
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 823811076d..1848077932 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -182,6 +182,7 @@
 183 rw auto migration
 185 rw auto
 186 rw auto
+187 rw auto
 188 rw auto quick
 189 rw auto
 190 rw auto quick
-- 
2.13.4

Re: [Qemu-block] [PATCH v3 for-2.10 0/4] improved --version/--help tweaks

[Jeff Cody]

On Tue, Aug 08, 2017 at 08:50:55AM -0500, Eric Blake wrote:
> On 08/07/2017 07:08 PM, John Snow wrote:
> > 
> > 
> > On 08/03/2017 12:33 PM, Eric Blake wrote:
> >> Not sure if this should go through Kevin's block tree, Paolo's
> >> miscellaneous patches, or if I should just do a pull request
> >> myself (since patch 4 includes a change to qemu-nbd)
> >>
> 
> > 
> > Nothing to keep the commands from going out of order again, it looks
> > like -- can this be added as a comment or otherwise scripted as a
> > ./hey_it_looks_like_you_are_about_to_release_qemu.sh script that makes
> > sure we've dotted the 'i's and crossed the 't's?
> 
> We don't add subcommands to qemu-img all that frequently; I think a
> comment is okay without having to figure out where to script things.
> 
> > 
> > Lastly, Didn't we fix the certificate issue? (I thought Jeff had) -- and
> > even if not, it's still the correct address to send people to IMO. Let
> > people report to us if the SSL certificate appears to be broken.
> 
> That can be a followup-patch.
> 

Probably no need for one, really.  We force redirect to SSL, so the http://
versions will still end up https://.

And to confirm - the SSL certificates are now all in order.  The issue on
the website I think you were referencing in that commit message is because
we still had two URL resources pulling in non-ssl assets (Google fonts, and
jquery), so browsers blocked those mixed requests.  But that has been
updated.


-Jeff

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

[Eric Blake]

On 08/08/2017 03:28 AM, Kevin Wolf wrote:
> Am 07.08.2017 um 22:30 hat Eric Blake geschrieben:
>> 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 
>> ---

> qcow2 ended up with this prototype:
> 
> int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
>  unsigned int *bytes, uint64_t *cluster_offset)
> 
> Separating a full uin64_t offset by-reference parameter and an 0/-errno
> status return code feels a little cleaner to me.
> 
> And in fact, it's not only cleaner, but bit 63 is QCOW_OFLAG_COMPRESSED,
> so this patch breaks compressed images.

Looks like I'll be sending a v2, then.

-- 
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 03/56] monitor: Rewrite comment describing HMP .args_type

[Paolo Bonzini]

On 08/08/2017 13:20, Dr. David Alan Gilbert wrote:
> * Markus Armbruster (arm...@redhat.com) wrote:
>> 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).
> 
> 'o' is messy.  It using qemu_strtosz_MiB which uses a 'double' intermediate
> so I fear it can round.  It also has a note it can't take all f's due to
> an overflow from the conversion.   Two things not mentioned are that
> it also takes hex (as explicit 0x) and that it also does 'b' as a suffix
> to multiply by 1.  Those two combine in bad ways - i.e. 0x1b is 27MB,
> 1b is 1 byte (same for 'e').  These are probably OK except if you were
> to start replacing 'l' by 'o' because you really wanted 64bit addresses
> say.
> (I also wouldn't bother expanding the size names and powers).
> 
>> + *
>> + * 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).
> 
> I found the previous description clearer.
> 
>> + * 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 

[Qemu-block] [PATCH for 2.10] block/nbd-client: always return EIO on and after the first io channel error

[Vladimir Sementsov-Ogievskiy]

Do not communicate after the first error to avoid communicating throught
broken channel. The only exclusion is try to send NBD_CMD_DISC anyway on
in nbd_client_close.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

Hi all. Here is a patch, fixing a problem noted in
[PATCH 06/17] block/nbd-client: fix nbd_read_reply_entry
and
[PATCH 17/17] block/nbd-client: always return EIO on and after the first io 
channel error
and discussed on list.

If it will be applied to 2.10, then I'll rebase my 'nbd client refactoring and 
fixing'
on it (for 2.11). If not - I'll prefer not rebase the series, so, do not apply 
this
patch for 2.11.

 block/nbd-client.h |  1 +
 block/nbd-client.c | 58 +-
 2 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index df80771357..28db9922c8 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -29,6 +29,7 @@ typedef struct NBDClientSession {
 
 Coroutine *recv_coroutine[MAX_NBD_REQUESTS];
 NBDReply reply;
+bool eio_to_all;
 } NBDClientSession;
 
 NBDClientSession *nbd_get_client_session(BlockDriverState *bs);
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 25dd28406b..1282b2484e 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -49,6 +49,8 @@ static void nbd_teardown_connection(BlockDriverState *bs)
 {
 NBDClientSession *client = nbd_get_client_session(bs);
 
+client->eio_to_all = true;
+
 if (!client->ioc) { /* Already closed */
 return;
 }
@@ -74,12 +76,16 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
 Error *local_err = NULL;
 
 for (;;) {
+if (s->eio_to_all) {
+break;
+}
+
 assert(s->reply.handle == 0);
 ret = nbd_receive_reply(s->ioc, &s->reply, &local_err);
 if (ret < 0) {
 error_report_err(local_err);
 }
-if (ret <= 0) {
+if (ret <= 0 || s->eio_to_all) {
 break;
 }
 
@@ -107,6 +113,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
 qemu_coroutine_yield();
 }
 
+s->eio_to_all = true;
 nbd_recv_coroutines_enter_all(s);
 s->read_reply_co = NULL;
 }
@@ -118,6 +125,10 @@ static int nbd_co_send_request(BlockDriverState *bs,
 NBDClientSession *s = nbd_get_client_session(bs);
 int rc, ret, i;
 
+if (s->eio_to_all) {
+return -EIO;
+}
+
 qemu_co_mutex_lock(&s->send_mutex);
 while (s->in_flight == MAX_NBD_REQUESTS) {
 qemu_co_queue_wait(&s->free_sema, &s->send_mutex);
@@ -135,15 +146,15 @@ static int nbd_co_send_request(BlockDriverState *bs,
 assert(i < MAX_NBD_REQUESTS);
 request->handle = INDEX_TO_HANDLE(s, i);
 
-if (!s->ioc) {
+if (s->eio_to_all) {
 qemu_co_mutex_unlock(&s->send_mutex);
-return -EPIPE;
+return -EIO;
 }
 
 if (qiov) {
 qio_channel_set_cork(s->ioc, true);
 rc = nbd_send_request(s->ioc, request);
-if (rc >= 0) {
+if (rc >= 0 && !s->eio_to_all) {
 ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, false,
   NULL);
 if (ret != request->len) {
@@ -155,7 +166,8 @@ static int nbd_co_send_request(BlockDriverState *bs,
 rc = nbd_send_request(s->ioc, request);
 }
 qemu_co_mutex_unlock(&s->send_mutex);
-return rc;
+
+return s->eio_to_all ? -EIO : rc;
 }
 
 static void nbd_co_receive_reply(NBDClientSession *s,
@@ -169,13 +181,13 @@ static void nbd_co_receive_reply(NBDClientSession *s,
 qemu_coroutine_yield();
 *reply = s->reply;
 if (reply->handle != request->handle ||
-!s->ioc) {
+!s->ioc || s->eio_to_all) {
 reply->error = EIO;
 } else {
 if (qiov && reply->error == 0) {
 ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, true,
   NULL);
-if (ret != request->len) {
+if (ret != request->len || s->eio_to_all) {
 reply->error = EIO;
 }
 }
@@ -225,8 +237,10 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t 
offset,
 } else {
 nbd_co_receive_reply(client, &request, &reply, qiov);
 }
-nbd_coroutine_end(bs, &request);
-return -reply.error;
+if (request.handle != 0) {
+nbd_coroutine_end(bs, &request);
+}
+return client->eio_to_all ? -EIO : -reply.error;
 }
 
 int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t offset,
@@ -254,8 +268,10 @@ int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t 
offset,
 } else {
 nbd_co_receive_reply(client, &request, &reply, NULL);
 }
-nbd_coroutine_end(bs, &request);
-return -reply.error;
+if (request.handle != 0) {
+nbd_coroutine_end(bs, &request);
+}
+return client->eio_to_all ? -EIO : -reply.error;
 }
 
 int nbd_client_co_pwrite_zeroes(BlockDriverState *bs, int64_t offs

Re: [Qemu-block] [PATCH v4] tests: Avoid non-portable 'echo -ARG'

[Eric Blake]

On 08/08/2017 08:54 AM, Kevin Wolf wrote:
> Am 03.07.2017 um 20:09 hat Eric Blake geschrieben:
>> POSIX says that backslashes in the arguments to 'echo', as well as
>> any use of 'echo -n' and 'echo -e', are non-portable; it recommends
>> people should favor 'printf' instead.  This is definitely true where
>> we do not control which shell is running (such as in makefile snippets
>> or in documentation examples).  But even for scripts where we
>> require bash (and therefore, where echo does what we want by default),
>> it is still possible to use 'shopt -s xpg_echo' to change bash's
>> behavior of echo.  And setting a good example never hurts when we are
>> not sure if a snippet will be copied from a bash-only script to a
>> general shell script (although I don't change the use of non-portable
>> \e for ESC when we know the running shell is bash).
>>

>> +++ b/tests/multiboot/run_test.sh
>> @@ -26,7 +26,7 @@ run_qemu() {
>>  local kernel=$1
>>  shift
>>
>> -echo -e "\n\n=== Running test case: $kernel $@ ===\n" >> test.log
>> +printf %b "\n\n=== Running test case: $kernel $@ ===\n\n" >> test.log
>>
>>  $QEMU \
>>  -kernel $kernel \
> 
> Not completely sure why, but this broke the test with whitespace changes
> like this:
> 
> -=== Running test case: mmap.elf -m 1.1M ===
> +=== Running test case: mmap.elf -m1.1M ===

I guess that means I'm not regularly running tests/multiboot?  Is it not
part of 'make check' or qemu-iotests?

Ah, I see the problem, and it's insidious.  We're using "...$@...", but
want to be using "...$*...".  $@ causes multiple arguments to be passed,
but printf %b is not concatenating those arguments; while $* uses only a
single argument.  We didn't notice it with echo -e, because echo inserts
a space between multiple arguments, just as you'd get a space with $*.

Fix coming up.

-- 
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] virtio-blk: handle blk_getlength() errors

[Fam Zheng]

On Tue, 08/08 13:22, Stefan Hajnoczi wrote:
> If blk_getlength() fails in virtio_blk_update_config() consider the disk
> image length to be 0 bytes.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  hw/block/virtio-blk.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index b750bd8b53..a16ac75090 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -730,6 +730,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, 
> uint8_t *config)
>  BlockConf *conf = &s->conf.conf;
>  struct virtio_blk_config blkcfg;
>  uint64_t capacity;
> +int64_t length;
>  int blk_size = conf->logical_block_size;
>  
>  blk_get_geometry(s->blk, &capacity);
> @@ -752,7 +753,8 @@ static void virtio_blk_update_config(VirtIODevice *vdev, 
> uint8_t *config)
>   * divided by 512 - instead it is the amount of blk_size blocks
>   * per track (cylinder).
>   */
> -if (blk_getlength(s->blk) /  conf->heads / conf->secs % blk_size) {
> +length = blk_getlength(s->blk);
> +if (length > 0 && length / conf->heads / conf->secs % blk_size) {
>  blkcfg.geometry.sectors = conf->secs & ~s->sector_mask;
>  } else {
>  blkcfg.geometry.sectors = conf->secs;
> -- 
> 2.13.3
> 
> 

Reviewed-by: Fam Zheng 

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

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:
> 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;

a little bit further down is a:
error_setg(errp, "Invalid addr 0x%016" PRIx64 "/size %" PRId64
 " specified", orig_addr, orig_size);

that PRId64 should be a PRIu64 now

However, other than that;


Reviewed-by: Dr. David Alan Gilbert 

> @@ -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
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[Qemu-block] [PATCH for-2.10] tests/multiboot: Fix whitespace failure

[Eric Blake]

Commit b43671f8 accidentally broke run_test.sh within tests/multiboot;
due to a subtle change in whitespace.

These two commands produce theh same output (at least, for sane $IFS
of space-tab-newline):

echo -e "...$@..."
echo -e "...$*..."

But that's only because echo inserts spaces between multiple arguments
(the $@ case), while the $* form gives a single argument to echo with
the spaces already present.

But when converting to printf %b, there are no automatic spaces between
multiple arguments, so we HAVE to use $*.

It doesn't help that run_test.sh isn't part of 'make check'.

Signed-off-by: Eric Blake 
---
 tests/multiboot/run_test.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/multiboot/run_test.sh b/tests/multiboot/run_test.sh
index c8f3da8f37..0278148b43 100755
--- a/tests/multiboot/run_test.sh
+++ b/tests/multiboot/run_test.sh
@@ -26,7 +26,7 @@ run_qemu() {
 local kernel=$1
 shift

-printf %b "\n\n=== Running test case: $kernel $@ ===\n\n" >> test.log
+printf %b "\n\n=== Running test case: $kernel $* ===\n\n" >> test.log

 $QEMU \
 -kernel $kernel \
-- 
2.13.4

Re: [Qemu-block] [PATCH for 2.10] block/nbd-client: always return EIO on and after the first io channel error

[Vladimir Sementsov-Ogievskiy]

08.08.2017 17:29, Vladimir Sementsov-Ogievskiy wrote:

Do not communicate after the first error to avoid communicating throught
broken channel. The only exclusion is try to send NBD_CMD_DISC anyway on
in nbd_client_close.


worth add: To simplify things, return -EIO in case of disconnect too.

(refers to:

-if (!s->ioc) {
+if (s->eio_to_all) {
 qemu_co_mutex_unlock(&s->send_mutex);
-return -EPIPE;
+return -EIO;
 }

and to:

@@ -49,6 +49,8 @@ static void nbd_teardown_connection(BlockDriverState *bs)
 {
 NBDClientSession *client = nbd_get_client_session(bs);
 
+client->eio_to_all = true;

+

However, I'm unsure about, how s->ioc may be NULL in first chunk, and if 
it possible, why
it is not checked everywhere. read/write after bdrv_close? Should it be 
handled on higher level?)




Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

Hi all. Here is a patch, fixing a problem noted in
[PATCH 06/17] block/nbd-client: fix nbd_read_reply_entry
and
[PATCH 17/17] block/nbd-client: always return EIO on and after the first io 
channel error
and discussed on list.

If it will be applied to 2.10, then I'll rebase my 'nbd client refactoring and 
fixing'
on it (for 2.11). If not - I'll prefer not rebase the series, so, do not apply 
this
patch for 2.11.

  block/nbd-client.h |  1 +
  block/nbd-client.c | 58 +-
  2 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index df80771357..28db9922c8 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -29,6 +29,7 @@ typedef struct NBDClientSession {
  
  Coroutine *recv_coroutine[MAX_NBD_REQUESTS];

  NBDReply reply;
+bool eio_to_all;
  } NBDClientSession;
  
  NBDClientSession *nbd_get_client_session(BlockDriverState *bs);

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 25dd28406b..1282b2484e 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -49,6 +49,8 @@ static void nbd_teardown_connection(BlockDriverState *bs)
  {
  NBDClientSession *client = nbd_get_client_session(bs);
  
+client->eio_to_all = true;

+
  if (!client->ioc) { /* Already closed */
  return;
  }
@@ -74,12 +76,16 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
  Error *local_err = NULL;
  
  for (;;) {

+if (s->eio_to_all) {
+break;
+}
+
  assert(s->reply.handle == 0);
  ret = nbd_receive_reply(s->ioc, &s->reply, &local_err);
  if (ret < 0) {
  error_report_err(local_err);
  }
-if (ret <= 0) {
+if (ret <= 0 || s->eio_to_all) {
  break;
  }
  
@@ -107,6 +113,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)

  qemu_coroutine_yield();
  }
  
+s->eio_to_all = true;

  nbd_recv_coroutines_enter_all(s);
  s->read_reply_co = NULL;
  }
@@ -118,6 +125,10 @@ static int nbd_co_send_request(BlockDriverState *bs,
  NBDClientSession *s = nbd_get_client_session(bs);
  int rc, ret, i;
  
+if (s->eio_to_all) {

+return -EIO;
+}
+
  qemu_co_mutex_lock(&s->send_mutex);
  while (s->in_flight == MAX_NBD_REQUESTS) {
  qemu_co_queue_wait(&s->free_sema, &s->send_mutex);
@@ -135,15 +146,15 @@ static int nbd_co_send_request(BlockDriverState *bs,
  assert(i < MAX_NBD_REQUESTS);
  request->handle = INDEX_TO_HANDLE(s, i);
  
-if (!s->ioc) {

+if (s->eio_to_all) {
  qemu_co_mutex_unlock(&s->send_mutex);
-return -EPIPE;
+return -EIO;
  }
  
  if (qiov) {

  qio_channel_set_cork(s->ioc, true);
  rc = nbd_send_request(s->ioc, request);
-if (rc >= 0) {
+if (rc >= 0 && !s->eio_to_all) {
  ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, false,
NULL);
  if (ret != request->len) {
@@ -155,7 +166,8 @@ static int nbd_co_send_request(BlockDriverState *bs,
  rc = nbd_send_request(s->ioc, request);
  }
  qemu_co_mutex_unlock(&s->send_mutex);
-return rc;
+
+return s->eio_to_all ? -EIO : rc;
  }
  
  static void nbd_co_receive_reply(NBDClientSession *s,

@@ -169,13 +181,13 @@ static void nbd_co_receive_reply(NBDClientSession *s,
  qemu_coroutine_yield();
  *reply = s->reply;
  if (reply->handle != request->handle ||
-!s->ioc) {
+!s->ioc || s->eio_to_all) {
  reply->error = EIO;
  } else {
  if (qiov && reply->error == 0) {
  ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, true,
NULL);
-if (ret != request->len) {
+if (ret != request->len || s->eio_to_all) {
  reply->error = EIO;
  }
  }
@@ -225,8 +237,10 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t 
offset,
  } else {
  nbd_co_receive_reply(clien

Re: [Qemu-block] [PATCH for 2.10] block/nbd-client: always return EIO on and after the first io channel error

[Eric Blake]

On 08/08/2017 09:29 AM, Vladimir Sementsov-Ogievskiy wrote:
> Do not communicate after the first error to avoid communicating throught
> broken channel. The only exclusion is try to send NBD_CMD_DISC anyway on
> in nbd_client_close.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
> 
> Hi all. Here is a patch, fixing a problem noted in
> [PATCH 06/17] block/nbd-client: fix nbd_read_reply_entry
> and
> [PATCH 17/17] block/nbd-client: always return EIO on and after the first io 
> channel error
> and discussed on list.
> 
> If it will be applied to 2.10, then I'll rebase my 'nbd client refactoring 
> and fixing'
> on it (for 2.11). If not - I'll prefer not rebase the series, so, do not 
> apply this
> patch for 2.11.

It may be possible to do something even smaller:

> 
>  block/nbd-client.h |  1 +
>  block/nbd-client.c | 58 
> +-
>  2 files changed, 41 insertions(+), 18 deletions(-)
> 
> diff --git a/block/nbd-client.h b/block/nbd-client.h
> index df80771357..28db9922c8 100644
> --- a/block/nbd-client.h
> +++ b/block/nbd-client.h
> @@ -29,6 +29,7 @@ typedef struct NBDClientSession {
>  
>  Coroutine *recv_coroutine[MAX_NBD_REQUESTS];
>  NBDReply reply;
> +bool eio_to_all;
>  } NBDClientSession;
>  
>  NBDClientSession *nbd_get_client_session(BlockDriverState *bs);
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 25dd28406b..1282b2484e 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -49,6 +49,8 @@ static void nbd_teardown_connection(BlockDriverState *bs)
>  {
>  NBDClientSession *client = nbd_get_client_session(bs);
>  
> +client->eio_to_all = true;
> +
>  if (!client->ioc) { /* Already closed */
>  return;
>  }
> @@ -74,12 +76,16 @@ static coroutine_fn void nbd_read_reply_entry(void 
> *opaque)
>  Error *local_err = NULL;
>  
>  for (;;) {
> +if (s->eio_to_all) {
> +break;
> +}
> +

How is this conditional ever reached? nbd_read_reply_entry() is our
workhorse, that is supposed to run until the client is ready to disconnect.

>  assert(s->reply.handle == 0);
>  ret = nbd_receive_reply(s->ioc, &s->reply, &local_err);
>  if (ret < 0) {
>  error_report_err(local_err);
>  }
> -if (ret <= 0) {
> +if (ret <= 0 || s->eio_to_all) {
>  break;
>  }

This says we're now supposed to break out of the while loop.  So unless
something can set s->eio_to_all during
aio_co_wake(s->recv_coroutine[i]), the next iteration of the loop won't
hit the first conditional.

Meanwhile, even if we skip the first conditional, this second
conditional will still end the loop prior to acting on anything received
from the server (the difference being whether we called
nbd_receive_reply() one additional time, but I don't see that as getting
in the way of a clean exit).

But my question is whether we can just go with a simpler fix: if we ever
break out of the workhorse loop of nbd_read_reply_entry(), then (and
only then) is when we call nbd_recv_coroutines_enter_all() to mop up any
pending transactions before tearing down the coroutines.  Is there
something we can do in nbd_recv_coroutines_enter_all() that will
guarantee that our final entry into each coroutine will fail?

I'm still playing with the idea locally.

-- 
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 03/56] monitor: Rewrite comment describing HMP .args_type

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonz...@redhat.com) wrote:
> On 08/08/2017 13:20, Dr. David Alan Gilbert wrote:
> > * Markus Armbruster (arm...@redhat.com) wrote:
> >> 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).
> > 
> > 'o' is messy.  It using qemu_strtosz_MiB which uses a 'double' intermediate
> > so I fear it can round.  It also has a note it can't take all f's due to
> > an overflow from the conversion.   Two things not mentioned are that
> > it also takes hex (as explicit 0x) and that it also does 'b' as a suffix
> > to multiply by 1.  Those two combine in bad ways - i.e. 0x1b is 27MB,
> > 1b is 1 byte (same for 'e').  These are probably OK except if you were
> > to start replacing 'l' by 'o' because you really wanted 64bit addresses
> > say.
> > (I also wouldn't bother expanding the size names and powers).
> > 
> >> + *
> >> + * 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).
> > 
> > I found the previous description clearer.
> > 
> >> + * TYPEs that put multiple values:
> >> + * 'O'Option string of the form NAME=VALUE,... parsed according to
> >> + *the QemuOptsList given by its name.
> >> + *Example: 'device:

Re: [Qemu-block] [PATCH v4] tests: Avoid non-portable 'echo -ARG'

[Kevin Wolf]

Am 08.08.2017 um 16:29 hat Eric Blake geschrieben:
> On 08/08/2017 08:54 AM, Kevin Wolf wrote:
> > Am 03.07.2017 um 20:09 hat Eric Blake geschrieben:
> >> POSIX says that backslashes in the arguments to 'echo', as well as
> >> any use of 'echo -n' and 'echo -e', are non-portable; it recommends
> >> people should favor 'printf' instead.  This is definitely true where
> >> we do not control which shell is running (such as in makefile snippets
> >> or in documentation examples).  But even for scripts where we
> >> require bash (and therefore, where echo does what we want by default),
> >> it is still possible to use 'shopt -s xpg_echo' to change bash's
> >> behavior of echo.  And setting a good example never hurts when we are
> >> not sure if a snippet will be copied from a bash-only script to a
> >> general shell script (although I don't change the use of non-portable
> >> \e for ESC when we know the running shell is bash).
> >>
> 
> >> +++ b/tests/multiboot/run_test.sh
> >> @@ -26,7 +26,7 @@ run_qemu() {
> >>  local kernel=$1
> >>  shift
> >>
> >> -echo -e "\n\n=== Running test case: $kernel $@ ===\n" >> test.log
> >> +printf %b "\n\n=== Running test case: $kernel $@ ===\n\n" >> test.log
> >>
> >>  $QEMU \
> >>  -kernel $kernel \
> > 
> > Not completely sure why, but this broke the test with whitespace changes
> > like this:
> > 
> > -=== Running test case: mmap.elf -m 1.1M ===
> > +=== Running test case: mmap.elf -m1.1M ===
> 
> I guess that means I'm not regularly running tests/multiboot?  Is it not
> part of 'make check' or qemu-iotests?

The problem is that it needs an i386 compiler to build the test kernels
(and qemu-system-i386 or qemu-system-x86_64 binaries to execute them).

I guess we could check these conditions, though, and skip the test if we
can't produce i386 binaries.

> Ah, I see the problem, and it's insidious.  We're using "...$@...", but
> want to be using "...$*...".  $@ causes multiple arguments to be passed,
> but printf %b is not concatenating those arguments; while $* uses only a
> single argument.  We didn't notice it with echo -e, because echo inserts
> a space between multiple arguments, just as you'd get a space with $*.

The thing that completely confused me here is that printf doesn't just
ignore additional arguments as I would have expected, but just starts
over with the format string, so that it does kind of work with multiple
arguments and fails only subtly.

Kevin


signature.asc
Description: PGP signature

Re: [Qemu-block] [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Markus Armbruster]

Stefan Hajnoczi  writes:

> On Tue, Aug 08, 2017 at 10:06:04AM +0200, Markus Armbruster wrote:
>> Stefan Hajnoczi  writes:
>> 
>> > On Wed, Jul 26, 2017 at 02:24:02PM -0400, Cleber Rosa wrote:
>> >> 
>> >> 
>> >> On 07/26/2017 01:58 PM, Stefan Hajnoczi wrote:
>> >> > On Tue, Jul 25, 2017 at 12:16:13PM -0400, Cleber Rosa wrote:
>> >> >> On 07/25/2017 11:49 AM, Stefan Hajnoczi wrote:
>> >> >>> On Fri, Jul 21, 2017 at 10:21:24AM -0400, Cleber Rosa wrote:
>> >>  On 07/21/2017 10:01 AM, Daniel P. Berrange wrote:
>> >> > On Fri, Jul 21, 2017 at 01:33:25PM +0100, Stefan Hajnoczi wrote:
>> >> >> On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
>> >>  Without the static capabilities defined, the dynamic check would be
>> >>  influenced by the run time environment.  It would really mean 
>> >>  "qemu-io
>> >>  running on this environment (filesystem?) can do native aio".  Again,
>> >>  that's not the best type of information to depend on when writing 
>> >>  tests.
>> >> >>>
>> >> >>> Can you explain this more?
>> >> >>>
>> >> >>> It seems logical to me that if qemu-io in this environment cannot do
>> >> >>> aio=native then we must skip those tests.
>> >> >>>
>> >> >>> Stefan
>> >> >>>
>> >> >>
>> >> >> OK, let's abstract a bit more.  Let's take this part of your statement:
>> >> >>
>> >> >>  "if qemu-io in this environment cannot do aio=native"
>> >> >>
>> >> >> Let's call that a feature check.  Depending on how the *feature check*
>> >> >> is written, a negative result may hide a test failure, because it would
>> >> >> now be skipped.
>> >> > 
>> >> > You are saying a pass->skip transition can hide a failure but ./check
>> >> > tracks skipped tests.  See tests/qemu-iotests/check.log for a
>> >> > pass/fail/skip history.
>> >> > 
>> >> 
>> >> You're not focusing on the problem here.  The problem is that a test
>> >> that *was not* supposed to be skipped, would be skipped.
>> >
>> > As Daniel Berrange mentioned, ./configure has the same problem.  You
>> > cannot just run it blindly because it silently disables features.
>> >
>> > What I'm saying is that in addition to watching ./configure closely, you
>> > also need to look at the skipped tests that ./check reports.  If you do
>> > that then you can be sure the expected set of tests is passing.
>> >
>> >> > It is the job of the CI system to flag up pass/fail/skip transitions.
>> >> > You're no worse off using feature tests.
>> >> > 
>> >> > Stefan
>> >> > 
>> >> 
>> >> What I'm trying to help us achieve here is a reliable and predictable
>> >> way for the same test job execution to be comparable across
>> >> environments.  From the individual developer workstation, CI, QA etc.
>> >
>> > 1. Use ./configure --enable-foo options for all desired features.
>> > 2. Run the ./check command-line and there should be no unexpected skips
>> >like this:
>> >
>> > 087 [not run] missing aio=native support
>> >
>> > To me this seems to address the problem.
>> >
>> > I have mentioned the issues with the build flags solution: it creates a
>> > dependency on the build environment and forces test feature checks to
>> > duplicate build dependency logic.  This is why I think feature tests are
>> > a cleaner solution.
>> 
>> I suspect the actual problem here is that the qemu-iotests harness is
>> not integrated in the build process.  For other tests, we specify the
>> tests to run in a Makefile, and use the same configuration mechanism as
>> for building stuff conditionally.
>
> The ability to run tests against QEMU binaries without a build
> environment is useful though.  It would still be possible to symlink to
> external binaries but then the build feature information could be
> incorrect.

I don't dispute it's useful.  "make check" doesn't do it, though.

I think we can either have a standalone test suite (introspects the
binaries under test to figure out what to test), or an integrated test
suite (tests exactly what is configured).  "make check" is the latter.
qemu-iotests is kind-of-sort-of the former.

Re: [Qemu-block] [PATCH v3 5/7] block: add throttle block filter driver

[Alberto Garcia]

On Tue 08 Aug 2017 03:45:44 PM CEST, Manos Pitsidianakis wrote:
> On Tue, Aug 08, 2017 at 03:13:36PM +0200, Alberto Garcia wrote:
>>On Mon 31 Jul 2017 11:54:41 AM CEST, Manos Pitsidianakis wrote:
>>> block/throttle.c uses existing I/O throttle infrastructure inside a
>>> block filter driver. I/O operations are intercepted in the filter's
>>> read/write coroutines, and referred to block/throttle-groups.c
>>>
>>> The driver can be used with the syntax
>>> -drive driver=throttle,file.filename=foo.qcow2, \
>>> limits.iops-total=...,throttle-group=bar
>>
>>Sorry for not having noticed this earlier, but can't you define the
>>throttling group (and its limits) using -object throttle-group ... as
>>shown in the previous patch, and simply reference it here? Or would we
>>have two alternative ways of setting the throttling limits?
>>
>>What happens if you have many -drive lines each one with a different set
>>of limits but with the same throttling group?
>
> The limits of the last one to be processed will win.

That's what the current throttling API does, and I tend to agree that
it's not completely straightforward (a few people have asked me the same
question since this feature is available).

If we're going to add a new API we could eliminate this ambiguity. After
all the previous -drive throttling.iops-total=... would still be
available, wouldn't it?

> So basically if we have anonymous groups, we accept limits in the
> driver options but only without a group-name.

In the commit message you do however have limits and a group name, is
that a mistake?

-drive driver=throttle,file.filename=foo.qcow2, \
   limits.iops-total=...,throttle-group=bar

Berto

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

[Eric Blake]

On 08/07/2017 08:55 PM, John Snow wrote:
> 
> 
> 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.

And definitely conflicts with my work on byte-based block status.


>>  
>> -uint32_t bdrv_dirty_bitmap_meta_granularity(BdrvDirtyBitmap *bitmap)
>> -{
>> -return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->meta);
>> -}
> 
> Why? Unused? Not cool enough to mention?

Already deleted as unused in my byte-based series.

-- 
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 27/56] block/dirty-bitmap: Clean up signed vs. unsigned dirty counts

[Eric Blake]

On 08/07/2017 09: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 
> ---
>  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(-)

I don't know how much this will conflict with my pending work for
byte-based block status, but I suspect it may be easier for your RFC to
go in after my cleanups (I think you'll still have things to fix).

-- 
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 v3 5/7] block: add throttle block filter driver

[Manos Pitsidianakis]

On Tue, Aug 08, 2017 at 04:53:08PM +0200, Alberto Garcia wrote:

On Tue 08 Aug 2017 03:45:44 PM CEST, Manos Pitsidianakis wrote:

On Tue, Aug 08, 2017 at 03:13:36PM +0200, Alberto Garcia wrote:

On Mon 31 Jul 2017 11:54:41 AM CEST, Manos Pitsidianakis wrote:

block/throttle.c uses existing I/O throttle infrastructure inside a
block filter driver. I/O operations are intercepted in the filter's
read/write coroutines, and referred to block/throttle-groups.c

The driver can be used with the syntax
-drive driver=throttle,file.filename=foo.qcow2, \
limits.iops-total=...,throttle-group=bar


Sorry for not having noticed this earlier, but can't you define the
throttling group (and its limits) using -object throttle-group ... as
shown in the previous patch, and simply reference it here? Or would we
have two alternative ways of setting the throttling limits?

What happens if you have many -drive lines each one with a different set
of limits but with the same throttling group?


The limits of the last one to be processed will win.


That's what the current throttling API does, and I tend to agree that
it's not completely straightforward (a few people have asked me the same
question since this feature is available).

If we're going to add a new API we could eliminate this ambiguity. After
all the previous -drive throttling.iops-total=... would still be
available, wouldn't it?


Indeed, it already is.




So basically if we have anonymous groups, we accept limits in the
driver options but only without a group-name.


In the commit message you do however have limits and a group name, is
that a mistake?

   -drive driver=throttle,file.filename=foo.qcow2, \
  limits.iops-total=...,throttle-group=bar



Sorry this wasn't clear, I'm actually proposing to remove limits from 
the throttle driver options and only create/config throttle groups via 
-object/object-add.


signature.asc
Description: PGP signature

Re: [Qemu-block] [RFC PATCH 09/56] balloon: Make balloon size unsigned in QAPI/QMP

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:
> Sizes should use QAPI type 'size' (uint64_t).  balloon parameter
> @value is 'int' (int64_t).  qmp_balloon() implicitly converts to
> ram_addr_t, i.e. uint64_t.  BALLOON_CHANGE parameter @actual and
> BalloonInfo member @actual are also 'int'.
> virtio_balloon_set_config() and virtio_balloon_stat() implicitly
> convert from ram_addr_t.
> 
> Change all three to 'size', and adjust the code using them.
> 
> balloon 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.
> 
> Doing the same for HMP's balloon deserves its own commit (the next
> one).
> 
> BALLOON_CHANGE and query-balloon now report sizes above 2^63-1
> correctly instead of their (negative) two's complement.
> 
> So does HMP's "info balloon".
> 
> Signed-off-by: Markus Armbruster 
> ---
>  balloon.c| 2 +-
>  hmp.c| 2 +-
>  qapi-schema.json | 4 ++--
>  qapi/event.json  | 2 +-
>  4 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/balloon.c b/balloon.c
> index 1d720ff..2ecca76 100644
> --- a/balloon.c
> +++ b/balloon.c
> @@ -102,7 +102,7 @@ BalloonInfo *qmp_query_balloon(Error **errp)
>  return info;
>  }
>  
> -void qmp_balloon(int64_t target, Error **errp)
> +void qmp_balloon(uint64_t target, Error **errp)
>  {
>  if (!have_balloon(errp)) {
>  return;

Can't you remove the:
  if (target <= 0) {

check?

(The type of the trace_balloon_event probably needs fixing
to be uint64_t rather than the unsigned long)

> diff --git a/hmp.c b/hmp.c
> index 8257dd0..4556045 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=%" PRId64 "\n", info->actual >> 20);
> +monitor_printf(mon, "balloon: actual=%" PRIu64 "\n", info->actual >> 20);
>  
>  qapi_free_BalloonInfo(info);
>  }
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 3ad2bc0..23eb60d 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2003,7 +2003,7 @@
>  # Since: 0.14.0
>  #
>  ##
> -{ 'struct': 'BalloonInfo', 'data': {'actual': 'int' } }
> +{ 'struct': 'BalloonInfo', 'data': {'actual': 'size' } }
>  
>  ##
>  # @query-balloon:
> @@ -2603,7 +2603,7 @@
>  # <- { "return": {} }
>  #
>  ##
> -{ 'command': 'balloon', 'data': {'value': 'int'} }
> +{ 'command': 'balloon', 'data': {'value': 'size'} }
>  
>  ##
>  # @Abort:
> diff --git a/qapi/event.json b/qapi/event.json
> index 6d22b02..9dfc70b 100644
> --- a/qapi/event.json
> +++ b/qapi/event.json
> @@ -488,7 +488,7 @@
>  #
>  ##
>  { 'event': 'BALLOON_CHANGE',
> -  'data': { 'actual': 'int' } }
> +  'data': { 'actual': 'size' } }

I was going to ask whether that was a problem for any external users,
but there again libvirt looks like it reads it into an unsigned long
long.

Dave

>  ##
>  # @GUEST_PANICKED:
> -- 
> 2.7.5
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-block] [PATCH for 2.10] block/nbd-client: always return EIO on and after the first io channel error

[Vladimir Sementsov-Ogievskiy]

08.08.2017 17:44, Eric Blake wrote:

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

Do not communicate after the first error to avoid communicating throught
broken channel. The only exclusion is try to send NBD_CMD_DISC anyway on
in nbd_client_close.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

Hi all. Here is a patch, fixing a problem noted in
[PATCH 06/17] block/nbd-client: fix nbd_read_reply_entry
and
[PATCH 17/17] block/nbd-client: always return EIO on and after the first io 
channel error
and discussed on list.

If it will be applied to 2.10, then I'll rebase my 'nbd client refactoring and 
fixing'
on it (for 2.11). If not - I'll prefer not rebase the series, so, do not apply 
this
patch for 2.11.

It may be possible to do something even smaller:


  block/nbd-client.h |  1 +
  block/nbd-client.c | 58 +-
  2 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index df80771357..28db9922c8 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -29,6 +29,7 @@ typedef struct NBDClientSession {
  
  Coroutine *recv_coroutine[MAX_NBD_REQUESTS];

  NBDReply reply;
+bool eio_to_all;
  } NBDClientSession;
  
  NBDClientSession *nbd_get_client_session(BlockDriverState *bs);

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 25dd28406b..1282b2484e 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -49,6 +49,8 @@ static void nbd_teardown_connection(BlockDriverState *bs)
  {
  NBDClientSession *client = nbd_get_client_session(bs);
  
+client->eio_to_all = true;

+
  if (!client->ioc) { /* Already closed */
  return;
  }
@@ -74,12 +76,16 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
  Error *local_err = NULL;
  
  for (;;) {

+if (s->eio_to_all) {
+break;
+}
+

How is this conditional ever reached? nbd_read_reply_entry() is our
workhorse, that is supposed to run until the client is ready to disconnect.


I forget to set eio_to_all on error in sending request.




  assert(s->reply.handle == 0);
  ret = nbd_receive_reply(s->ioc, &s->reply, &local_err);
  if (ret < 0) {
  error_report_err(local_err);
  }
-if (ret <= 0) {
+if (ret <= 0 || s->eio_to_all) {
  break;
  }

This says we're now supposed to break out of the while loop.  So unless
something can set s->eio_to_all during
aio_co_wake(s->recv_coroutine[i]), the next iteration of the loop won't
hit the first conditional.

Meanwhile, even if we skip the first conditional, this second
conditional will still end the loop prior to acting on anything received
from the server (the difference being whether we called
nbd_receive_reply() one additional time, but I don't see that as getting
in the way of a clean exit).

But my question is whether we can just go with a simpler fix: if we ever
break out of the workhorse loop of nbd_read_reply_entry(), then (and
only then) is when we call nbd_recv_coroutines_enter_all() to mop up any
pending transactions before tearing down the coroutines.  Is there
something we can do in nbd_recv_coroutines_enter_all() that will
guarantee that our final entry into each coroutine will fail?


anyway, you should prevent the following new requests, using some 
additional variable or disconnect..


Also, I think, an error in sending requests should cause finishing of 
this loop in nbd_read_reply_entry, so, an error condition should be 
checked after each yield.





I'm still playing with the idea locally.



--
Best regards,
Vladimir

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

[Eric Blake]

On 08/08/2017 04:13 AM, Daniel P. Berrange wrote:
> On Tue, Aug 08, 2017 at 10:39:29AM +0800, Fam Zheng wrote:
>> 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? :)
>>

> We can't remove it from the bdrv_co_pwritev() function, but we can remove
> it from bdrv_co_pwritev block driver callback AFAICT.

Sounds like a separate cleanup series to remove the length parameter for
both read and write (since we have write_zeroes), for the 2.11
timeframe.  You're right that the block layer does not have to have
quite the same interface as the driver callbacks.

-- 
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 v3 5/7] block: add throttle block filter driver

[Alberto Garcia]

On Tue 08 Aug 2017 04:56:20 PM CEST, Manos Pitsidianakis wrote:
>>> So basically if we have anonymous groups, we accept limits in the
>>> driver options but only without a group-name.
>>
>>In the commit message you do however have limits and a group name, is
>>that a mistake?
>>
>>-drive driver=throttle,file.filename=foo.qcow2, \
>>   limits.iops-total=...,throttle-group=bar
>
> Sorry this wasn't clear, I'm actually proposing to remove limits from
> the throttle driver options and only create/config throttle groups via
> -object/object-add.

Sorry I think it was me who misunderstood :-) Anyway in the new
command-line API I would be more inclined to have limits defined using
"-object throttle-group" and -drive would only reference the group id.

I understand that this implies that it wouldn't be possible to create
anonymous groups (at least not from the command line), is that a
problem?

Berto

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

[Eric Blake]

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

 Throttling "guaranties" that there will not be more than one
 request. But
 what prevent less than one, i.e. zero, like in my reproduction?
>>> Yes, I understand. Can we somehow make sure that at least one iteration
>>> is made? I'd really like to keep the functional test for block job
>>> throttling. I suppose a simple 'sleep 0.1' would do the trick, though
>>> it's not very clean.
>>>
>>> Kevin
>>
>>
>> I've started with 'sleep 0.5', now there are >100 successful
>> iterations... The other way is to check in test that there was 0 or 1
>> requests, but for this it looks better to rewrite it in python.
>>
>>
> 
> is sleep for ms portable?

Sadly, sub-second sleep is a GNU coreutils feature; I suspect the BSD
machines may fail to parse it.  (Of course, we could do some sort of
'sleep $SMALL', where $SMALL is 0.5 if sleep supports it, and 1 otherwise).

-- 
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 10/56] hmp: Make balloon's argument unsigned

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:
> 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);

That looks like a partial reversion of the last patch ?

Dave

>  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, &err);
> -- 
> 2.7.5
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

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

[Vladimir Sementsov-Ogievskiy]

08.08.2017 18:07, Eric Blake wrote:

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


Throttling "guaranties" that there will not be more than one
request. But
what prevent less than one, i.e. zero, like in my reproduction?

Yes, I understand. Can we somehow make sure that at least one iteration
is made? I'd really like to keep the functional test for block job
throttling. I suppose a simple 'sleep 0.1' would do the trick, though
it's not very clean.

Kevin


I've started with 'sleep 0.5', now there are >100 successful
iterations... The other way is to check in test that there was 0 or 1
requests, but for this it looks better to rewrite it in python.



is sleep for ms portable?

Sadly, sub-second sleep is a GNU coreutils feature; I suspect the BSD
machines may fail to parse it.  (Of course, we could do some sort of
'sleep $SMALL', where $SMALL is 0.5 if sleep supports it, and 1 otherwise).


sleep for 1 second may lead to more then one request done before qemu quite.


--
Best regards,
Vladimir

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

[Eric Blake]

On 08/08/2017 05:00 AM, Stefan Hajnoczi wrote:
> On Mon, Aug 07, 2017 at 07:15:29PM +0300, 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.
>>

> Why is this marked for-2.10?  Does it fix a bug?

Theoretically, converting between int64_t and double loses precision on
any values larger than 2^53.  In all practicality, though, if you expect
throttling to be precise through 2^53 (approximately 16 orders of
magnitude), you're crazy.  I also don't see any change to potential
division-by-zero actions (where differences in NaN vs. SIGFPE each have
their own advantages, but changing between them can be construed as a
bug fix).  So I'm also having a hard time seeing this as 2.10 material.

-- 
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] iotests: fix 185

[Kevin Wolf]

Am 08.08.2017 um 17:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 08.08.2017 18:07, Eric Blake wrote:
> > On 08/08/2017 04:04 AM, Vladimir Sementsov-Ogievskiy wrote:
> > 
> > > > > > Throttling "guaranties" that there will not be more than one
> > > > > > request. But
> > > > > > what prevent less than one, i.e. zero, like in my reproduction?
> > > > > Yes, I understand. Can we somehow make sure that at least one 
> > > > > iteration
> > > > > is made? I'd really like to keep the functional test for block job
> > > > > throttling. I suppose a simple 'sleep 0.1' would do the trick, though
> > > > > it's not very clean.
> > > > > 
> > > > > Kevin
> > > > 
> > > > I've started with 'sleep 0.5', now there are >100 successful
> > > > iterations... The other way is to check in test that there was 0 or 1
> > > > requests, but for this it looks better to rewrite it in python.
> > > > 
> > > > 
> > > is sleep for ms portable?
> > Sadly, sub-second sleep is a GNU coreutils feature; I suspect the BSD
> > machines may fail to parse it.  (Of course, we could do some sort of
> > 'sleep $SMALL', where $SMALL is 0.5 if sleep supports it, and 1 otherwise).
> > 
> sleep for 1 second may lead to more then one request done before qemu quite.

_supported_os Linux

So do we really care about portability? And are all the other test cases
working on the BSDs?

Kevin

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

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:
> 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 

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  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
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

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

[Alberto Garcia]

On Tue 08 Aug 2017 05:11:27 PM CEST, Eric Blake wrote:
>> Why is this marked for-2.10?  Does it fix a bug?
>
> Theoretically, converting between int64_t and double loses precision
> on any values larger than 2^53.  In all practicality, though, if you
> expect throttling to be precise through 2^53 (approximately 16 orders
> of magnitude), you're crazy.

You cannot, THROTTLE_VALUE_MAX is set to 10^15, which is smaller than
2^50.

> So I'm also having a hard time seeing this as 2.10 material.

Right, as I said that was a wrong assumption from my side :) No need for
this in 2.10.

Berto

[Qemu-block] [PATCH v2 for 2.10] block/nbd-client: always return EIO on and after the first io channel error

[Vladimir Sementsov-Ogievskiy]

Do not communicate after the first error to avoid communicating throught
broken channel. The only exclusion is try to send NBD_CMD_DISC anyway on
in nbd_client_close.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

Hi all. Here is a patch, fixing a problem noted in
[PATCH 06/17] block/nbd-client: fix nbd_read_reply_entry
and
[PATCH 17/17] block/nbd-client: always return EIO on and after the first io 
channel error
and discussed on list.

If it will be applied to 2.10, then I'll rebase my 'nbd client refactoring and 
fixing'
on it (for 2.11). If not - I'll prefer not rebase the series, so, do not apply 
this
patch for 2.11.

v2: set eio_to_all in nbd_co_send_request and nbd_co_receive_reply in case of 
error

 block/nbd-client.h |  1 +
 block/nbd-client.c | 65 +++---
 2 files changed, 48 insertions(+), 18 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index df80771357..28db9922c8 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -29,6 +29,7 @@ typedef struct NBDClientSession {
 
 Coroutine *recv_coroutine[MAX_NBD_REQUESTS];
 NBDReply reply;
+bool eio_to_all;
 } NBDClientSession;
 
 NBDClientSession *nbd_get_client_session(BlockDriverState *bs);
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 25dd28406b..a72cb7690a 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -49,6 +49,8 @@ static void nbd_teardown_connection(BlockDriverState *bs)
 {
 NBDClientSession *client = nbd_get_client_session(bs);
 
+client->eio_to_all = true;
+
 if (!client->ioc) { /* Already closed */
 return;
 }
@@ -74,12 +76,16 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
 Error *local_err = NULL;
 
 for (;;) {
+if (s->eio_to_all) {
+break;
+}
+
 assert(s->reply.handle == 0);
 ret = nbd_receive_reply(s->ioc, &s->reply, &local_err);
 if (ret < 0) {
 error_report_err(local_err);
 }
-if (ret <= 0) {
+if (ret <= 0 || s->eio_to_all) {
 break;
 }
 
@@ -107,6 +113,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
 qemu_coroutine_yield();
 }
 
+s->eio_to_all = true;
 nbd_recv_coroutines_enter_all(s);
 s->read_reply_co = NULL;
 }
@@ -118,6 +125,10 @@ static int nbd_co_send_request(BlockDriverState *bs,
 NBDClientSession *s = nbd_get_client_session(bs);
 int rc, ret, i;
 
+if (s->eio_to_all) {
+return -EIO;
+}
+
 qemu_co_mutex_lock(&s->send_mutex);
 while (s->in_flight == MAX_NBD_REQUESTS) {
 qemu_co_queue_wait(&s->free_sema, &s->send_mutex);
@@ -135,15 +146,15 @@ static int nbd_co_send_request(BlockDriverState *bs,
 assert(i < MAX_NBD_REQUESTS);
 request->handle = INDEX_TO_HANDLE(s, i);
 
-if (!s->ioc) {
+if (s->eio_to_all) {
 qemu_co_mutex_unlock(&s->send_mutex);
-return -EPIPE;
+return -EIO;
 }
 
 if (qiov) {
 qio_channel_set_cork(s->ioc, true);
 rc = nbd_send_request(s->ioc, request);
-if (rc >= 0) {
+if (rc >= 0 && !s->eio_to_all) {
 ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, false,
   NULL);
 if (ret != request->len) {
@@ -155,7 +166,13 @@ static int nbd_co_send_request(BlockDriverState *bs,
 rc = nbd_send_request(s->ioc, request);
 }
 qemu_co_mutex_unlock(&s->send_mutex);
-return rc;
+
+if (rc < 0 || s->eio_to_all) {
+s->eio_to_all = true;
+return -EIO;
+}
+
+return 0;
 }
 
 static void nbd_co_receive_reply(NBDClientSession *s,
@@ -169,14 +186,16 @@ static void nbd_co_receive_reply(NBDClientSession *s,
 qemu_coroutine_yield();
 *reply = s->reply;
 if (reply->handle != request->handle ||
-!s->ioc) {
+!s->ioc || s->eio_to_all) {
 reply->error = EIO;
+s->eio_to_all = true;
 } else {
 if (qiov && reply->error == 0) {
 ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, true,
   NULL);
-if (ret != request->len) {
+if (ret != request->len || s->eio_to_all) {
 reply->error = EIO;
+s->eio_to_all = true;
 }
 }
 
@@ -225,8 +244,10 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t 
offset,
 } else {
 nbd_co_receive_reply(client, &request, &reply, qiov);
 }
-nbd_coroutine_end(bs, &request);
-return -reply.error;
+if (request.handle != 0) {
+nbd_coroutine_end(bs, &request);
+}
+return client->eio_to_all ? -EIO : -reply.error;
 }
 
 int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t offset,
@@ -254,8 +275,10 @@ int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t 
offset,
 } else {
 nbd_co_receive_reply(client, &request, &reply, NULL);
 }
-nbd_coroutine_end(bs, &request);

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

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:
> 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.

But that also means all these values are assumed to be in MB by default?

Dave

> 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
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

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

[Markus Armbruster]

"Dr. David Alan Gilbert"  writes:

> * Markus Armbruster (arm...@redhat.com) wrote:
>> 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).
>
> 'o' is messy.  It using qemu_strtosz_MiB which uses a 'double' intermediate
> so I fear it can round.

It does, but only when you have more than 53 significant bits.

>  It also has a note it can't take all f's due to
> an overflow from the conversion.

Correct, because values between 0xfc00 and 2^64-1 round up
to 2^64.

If it bothers you, feel free to explore the following: feed the string
both to strtod() and to strtoll().  Whichever eats more characters wins.

This patch is of course just about better documenting what we have.  I
was starting to type something like "repeating the (complex) contract of
qemu_strtosz_MiB() here isn't so hot, let's include it by reference
instead", but then I looked it up.  Pffft.

>Two things not mentioned are that
> it also takes hex (as explicit 0x) and that it also does 'b' as a suffix
> to multiply by 1.  Those two combine in bad ways - i.e. 0x1b is 27MB,
> 1b is 1 byte (same for 'e').  These are probably OK except if you were
> to start replacing 'l' by 'o' because you really wanted 64bit addresses
> say.

I guess the sanest solution is not to recognize suffixes when the number
is hexadecimal.

> (I also wouldn't bother expanding the size names and powers).

I erred on the side of tedious clarity.  Feel free to suggest something
you like better.

>> + *
>> + * TYPEs that put a double value with key NAME:
>> + * 'T'Argument is a time in seconds.  With optional ms, us, ns
>> +

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

[Markus Armbruster]

"Dr. David Alan Gilbert"  writes:

> * Markus Armbruster (arm...@redhat.com) wrote:
>> 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;
>
> a little bit further down is a:
> error_setg(errp, "Invalid addr 0x%016" PRIx64 "/size %" PRId64
>  " specified", orig_addr, orig_size);
>
> that PRId64 should be a PRIu64 now

Will fix.

> However, other than that;
>
>
> Reviewed-by: Dr. David Alan Gilbert 

Thanks!

Re: [Qemu-block] [PULL 00/18] Block layer patches for 2.10.0-rc2

[Peter Maydell]

On 8 August 2017 at 14:58, Kevin Wolf  wrote:
> The following changes since commit b4174c4b08a719e7df7e4f35c29f44b7c2517237:
>
>   Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging 
> (2017-08-08 10:01:49 +0100)
>
> are available in the git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 113fe792fd4931dd0538f03859278b8719ee4fa2:
>
>   block/nfs: fix mutex assertion in nfs_file_close() (2017-08-08 15:19:16 
> +0200)
>
> 
> Block layer patches for 2.10.0-rc2
>
> 

Applied, thanks.

-- PMM

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

[Markus Armbruster]

Juan Quintela  writes:

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

I guess I should've explained this in the cover letter.

Until recent commit 5923f85, integers between INT64_MAX+1 and UINT64_MAX
did not work in QMP.  QEMU sent and accepted integers between INT64_MIN
and -1 instead.

The fix maintains strict compatibility for QMP input: negative integers
are accepted as before for backward compatibility.  Perhaps we can get
rid of this wart some day.

It does not maintain strict compatibility for QMP output: we now output
the correct integer.  We figure that's tolerable, because the obvious
way to parse the old output is strtoull(), and that does the right thing
for the new output when it does the right thing for the old output.

Fixing a QAPI type from 'int' to 'size' has the same compatibility
impact.

Questions?

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

[Markus Armbruster]

John Snow  writes:

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

I didn't feel like fixing an unused function, so I dropped it.  Can
mention this in the commit message.

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

[Markus Armbruster]

John Snow  writes:

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

Juan asked the same question on PATCH 15, see my reply there.

> Reviewed-by: John Snow 

Thanks!

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

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:
> "Dr. David Alan Gilbert"  writes:
> 
> > * Markus Armbruster (arm...@redhat.com) wrote:
> >> 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).
> >
> > 'o' is messy.  It using qemu_strtosz_MiB which uses a 'double' intermediate
> > so I fear it can round.
> 
> It does, but only when you have more than 53 significant bits.
> 
> >  It also has a note it can't take all f's due to
> > an overflow from the conversion.
> 
> Correct, because values between 0xfc00 and 2^64-1 round up
> to 2^64.

Right, so these bother me not for normal sizes, but if we were to start
to use them for hex values with meanings, like addresses for example.
(Although I guess that's unlikely with the default assumption of MB)

> If it bothers you, feel free to explore the following: feed the string
> both to strtod() and to strtoll().  Whichever eats more characters wins.

Is the reason we're using strtod because we actively want users to be
able to say 3.5G ?  I guess that's a reason to keep it.

> This patch is of course just about better documenting what we have.  I
> was starting to type something like "repeating the (complex) contract of
> qemu_strtosz_MiB() here isn't so hot, let's include it by reference
> instead", but then I looked it up.  Pffft.
> 
> >Two things not mentioned are that
> > it also takes hex (as explicit 0x) and that it also does 'b' as a suffix
> > to multiply by 1

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

[Eric Blake]

On 08/08/2017 10:16 AM, Kevin Wolf wrote:

 is sleep for ms portable?
>>> Sadly, sub-second sleep is a GNU coreutils feature; I suspect the BSD
>>> machines may fail to parse it.  (Of course, we could do some sort of
>>> 'sleep $SMALL', where $SMALL is 0.5 if sleep supports it, and 1 otherwise).
>>>
>> sleep for 1 second may lead to more then one request done before qemu quite.
> 
> _supported_os Linux
> 
> So do we really care about portability? And are all the other test cases
> working on the BSDs?

You've got valid points there - I suspect that a run of qemu-iotests on
BSD would turn up quite a few failures, some of which would be easy to
address; but it's not on my personal priority list.  I'm fine with an
approach of committing something that works where it was first tested,
then adding followup patches to improve portability, especially as long
as we are not currently getting anyone complaining about qemu-iotests
failures on BSD.

-- 
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 1/4] IDE: Do not flush empty CDROM drives

[John Snow]

The block backend changed in a way that flushing empty CDROM drives
is now an error. Amend IDE to avoid doing so until the root problem
can be addressed for 2.11.

Reported-by: Kieron Shorrock 
Signed-off-by: John Snow 
---
 hw/ide/core.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 0b48b64..6cbca43 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1053,17 +1053,21 @@ static void ide_flush_cb(void *opaque, int ret)
 ide_set_irq(s->bus);
 }
 
-static void ide_flush_cache(IDEState *s)
+static bool ide_flush_cache(IDEState *s)
 {
 if (s->blk == NULL) {
 ide_flush_cb(s, 0);
-return;
+return false;
+} else if (!blk_bs(s->blk)) {
+/* Nothing to flush */
+return true;
 }
 
 s->status |= BUSY_STAT;
 ide_set_retry(s);
 block_acct_start(blk_get_stats(s->blk), &s->acct, 0, BLOCK_ACCT_FLUSH);
 s->pio_aiocb = blk_aio_flush(s->blk, ide_flush_cb, s);
+return false;
 }
 
 static void ide_cfata_metadata_inquiry(IDEState *s)
@@ -1508,8 +1512,7 @@ static bool cmd_write_dma(IDEState *s, uint8_t cmd)
 
 static bool cmd_flush_cache(IDEState *s, uint8_t cmd)
 {
-ide_flush_cache(s);
-return false;
+return ide_flush_cache(s);
 }
 
 static bool cmd_seek(IDEState *s, uint8_t cmd)
-- 
2.9.4