[Qemu-block] [PATCH for 2.10 v2 04/20] nbd: fix memory leak in nbd_opt_go()

[Philippe Mathieu-Daudé]

nbd/client.c:385:12: warning: Potential leak of memory pointed to by 'buf'

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Eric Blake 
---
 nbd/client.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index 509ed5e4ba..0a17de80b5 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -376,9 +376,11 @@ static int nbd_opt_go(QIOChannel *ioc, const char 
*wantname,
 if (info->request_sizes) {
 stw_be_p(buf + 4 + len + 2, NBD_INFO_BLOCK_SIZE);
 }
-if (nbd_send_option_request(ioc, NBD_OPT_GO,
-4 + len + 2 + 2 * info->request_sizes, buf,
-errp) < 0) {
+error = nbd_send_option_request(ioc, NBD_OPT_GO,
+4 + len + 2 + 2 * info->request_sizes,
+buf, errp);
+g_free(buf);
+if (error < 0) {
 return -1;
 }
 
-- 
2.13.3

Re: [Qemu-block] [Qemu-devel] [PATCH v3 04/12] tests: Pass literal format strings directly to qmp_FOO()

[John Snow]

On 07/25/2017 05:15 PM, Eric Blake wrote:

From: Markus Armbruster 

The qmp_FOO() take a printf-like format string.  In a few places, we
assign a string literal to a variable and pass that instead of simply
passing the literal.  Clean that up.

Bonus: gets rid of non-literal format strings.  A step towards
compile-time format string checking without triggering
-Wformat-nonliteral.

Signed-off-by: Markus Armbruster 
Message-Id: <1500645206-13559-4-git-send-email-arm...@redhat.com>
Signed-off-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 


Reviewed-by: John Snow 

Re: [Qemu-block] [Qemu-devel] [PATCH 2/2] iotests: Redirect stderr to stdout in 186

[Jeff Cody]

On Tue, Jul 25, 2017 at 05:56:44PM +0200, Max Reitz wrote:
> Without redirecting qemu's stderr to stdout, _filter_qemu will not apply
> to warnings.  This results in $QEMU_PROG not being replaced by QEMU_PROG
> which is not great if your qemu executable is not called
> qemu-system-x86_64 (e.g. qemu-system-i386).
> 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/186 |  2 +-
>  tests/qemu-iotests/186.out | 12 ++--
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/qemu-iotests/186 b/tests/qemu-iotests/186
> index ab83ee4..2b9f618 100755
> --- a/tests/qemu-iotests/186
> +++ b/tests/qemu-iotests/186
> @@ -56,7 +56,7 @@ function do_run_qemu()
>  done
>  fi
>  echo quit
> -) | $QEMU -S -nodefaults -display none -device virtio-scsi-pci -monitor 
> stdio "$@"
> +) | $QEMU -S -nodefaults -display none -device virtio-scsi-pci -monitor 
> stdio "$@" 2>&1
>  echo
>  }
>  
> diff --git a/tests/qemu-iotests/186.out b/tests/qemu-iotests/186.out
> index b8bf9a2..c8377fe 100644
> --- a/tests/qemu-iotests/186.out
> +++ b/tests/qemu-iotests/186.out
> @@ -442,28 +442,28 @@ ide0-cd0 (NODE_NAME): null-co:// (null-co, read-only)
>  Cache mode:   writeback
>  (qemu) quit
>  
> -qemu-system-x86_64: -drive if=scsi,driver=null-co: warning: bus=0,unit=0 is 
> deprecated with this machine type
>  Testing: -drive if=scsi,driver=null-co
>  QEMU X.Y.Z monitor - type 'help' for more information
> -(qemu) info block
> +(qemu) QEMU_PROG: -drive if=scsi,driver=null-co: warning: bus=0,unit=0 is 
> deprecated with this machine type
> +info block
>  scsi0-hd0 (NODE_NAME): null-co:// (null-co)
>  Attached to:  /machine/unattached/device[27]/scsi.0/legacy[0]
>  Cache mode:   writeback
>  (qemu) quit
>  
> -qemu-system-x86_64: -drive if=scsi,media=cdrom: warning: bus=0,unit=0 is 
> deprecated with this machine type
>  Testing: -drive if=scsi,media=cdrom
>  QEMU X.Y.Z monitor - type 'help' for more information
> -(qemu) info block
> +(qemu) QEMU_PROG: -drive if=scsi,media=cdrom: warning: bus=0,unit=0 is 
> deprecated with this machine type
> +info block
>  scsi0-cd0: [not inserted]
>  Attached to:  /machine/unattached/device[27]/scsi.0/legacy[0]
>  Removable device: not locked, tray closed
>  (qemu) quit
>  
> -qemu-system-x86_64: -drive if=scsi,driver=null-co,media=cdrom: warning: 
> bus=0,unit=0 is deprecated with this machine type
>  Testing: -drive if=scsi,driver=null-co,media=cdrom
>  QEMU X.Y.Z monitor - type 'help' for more information
> -(qemu) info block
> +(qemu) QEMU_PROG: -drive if=scsi,driver=null-co,media=cdrom: warning: 
> bus=0,unit=0 is deprecated with this machine type
> +info block
>  scsi0-cd0 (NODE_NAME): null-co:// (null-co, read-only)
>  Attached to:  /machine/unattached/device[27]/scsi.0/legacy[0]
>  Removable device: not locked, tray closed
> -- 
> 2.9.4
> 
> 

Reviewed-by: Jeff Cody 

Re: [Qemu-block] [Qemu-devel] [PATCH 1/2] iotests: Fix test 156

[Jeff Cody]

On Tue, Jul 25, 2017 at 05:56:43PM +0200, Max Reitz wrote:
> On one hand, the _make_test_img invocation for creating the target image
> was missing a -u because its backing file is not supposed to exist at
> that point.
> 
> On the other hand, nobody noticed probably because the backing file is
> created later on and _cleanup failed to remove it: The quotation marks
> were misplaced so bash tried to deleted a file literally called
> "$TEST_IMG{,.target}..." instead of resolving the globs.  Thus, the
> files stayed around after the first run and qemu-img create did not
> complain about a missing backing file on any run but the first.
> 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/156 | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/156 b/tests/qemu-iotests/156
> index 2c4a06e..e75dc4d 100755
> --- a/tests/qemu-iotests/156
> +++ b/tests/qemu-iotests/156
> @@ -38,7 +38,7 @@ status=1# failure is the default!
>  _cleanup()
>  {
>  _cleanup_qemu
> -rm -f "$TEST_IMG{,.target}{,.backing,.overlay}"
> +rm -f "$TEST_IMG"{,.target}{,.backing,.overlay}
>  }
>  trap "_cleanup; exit \$status" 0 1 2 3 15
>  
> @@ -83,7 +83,7 @@ _send_qemu_cmd $QEMU_HANDLE \
>  'return'
>  
>  # Create target image
> -TEST_IMG="$TEST_IMG.target.overlay" _make_test_img -b "$TEST_IMG.target" 1M
> +TEST_IMG="$TEST_IMG.target.overlay" _make_test_img -u -b "$TEST_IMG.target" 
> 1M
>  
>  # Mirror snapshot
>  _send_qemu_cmd $QEMU_HANDLE \
> -- 
> 2.9.4
> 
> 

Reviewed-by: Jeff Cody 

Re: [Qemu-block] [Qemu-devel] [PATCH 1/2] dirty-bitmap: Report BlockDirtyInfo.count in bytes, as documented

[John Snow]

On 07/25/2017 05:37 PM, Eric Blake wrote:

On 07/25/2017 04:28 PM, John Snow wrote:



On 07/21/2017 02:32 PM, Eric Blake wrote:

We've been documenting the value in bytes since its introduction
in commit b9a9b3a4 (v1.3), where it was actually reported in bytes.

Commit e4654d2 (v2.0) then removed things from block/qapi.c, in
preparation for a rewrite to a list of dirty sectors in the next
commit 21b5683 in block.c, but the new code mistakenly started
reporting in sectors.

Fixes: https://bugzilla.redhat.com/1441460

CC: qemu-sta...@nongnu.org
Signed-off-by: Eric Blake 
Reviewed-by: John Snow 



I must have forgotten about this -- Didn't this get reviewed as part of
your byte series? Is this a resend for a stable branch?


Yes, it was reviewed during my part 2-of-4 series on byte-base block
status (the dirty bitmap series).  Series 1 made it into 2.10, but
series 2-4 did not; ergo, I filtered out the true bug-fixes that were
still worthy post-freeze.



Thanks for the clarification. Short term memory is a little strapped 
right now due to my move.


ACK

Re: [Qemu-block] [PATCH] qemu-iotests: add a "how to" to ./README

[Jeff Cody]

On Fri, Jul 21, 2017 at 10:34:16AM +0100, Stefan Hajnoczi wrote:
> There is not much getting started documentation for qemu-iotests.  This
> patch explains how to create a new test and covers the overall testing
> approach.
> 
> Cc: Ishani Chugh 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  tests/qemu-iotests/README | 83 
> +++
>  1 file changed, 83 insertions(+)
> 
> diff --git a/tests/qemu-iotests/README b/tests/qemu-iotests/README
> index 6079b40..8259b9f 100644
> --- a/tests/qemu-iotests/README
> +++ b/tests/qemu-iotests/README
> @@ -14,8 +14,91 @@ Just run ./check to run all tests for the raw image 
> format, or ./check
>  -qcow2 to test the qcow2 image format.  The output of ./check -h explains
>  additional options to test further image formats or I/O methods.
>  
> +* Testing approach
> +
> +Each test is an executable file (usually a bash script) that is run by the
> +./check test harness.  Standard out and standard error are captured to an
> +output file.  If the output file differs from the "golden master" output file
> +for the test then it fails.
> +
> +Tests are simply a sequence of commands that produce output and the test 
> itself
> +does not judge whether it passed or failed.  If you find yourself writing
> +checks to determine success or failure then you should rethink the test and
> +rely on output diffing instead.
> +
> +** Filtering volatile output
> +
> +When output contains absolute file paths, timestamps, process IDs, hostnames,
> +or other volatile strings, the diff against golden master output will fail.
> +Such output must be filtered to replace volatile strings with fixed
> +placeholders.
> +
> +For example, the path to the temporary working directory changes between test
> +runs so it must be filtered:
> +
> +  sed -e "s#$TEST_DIR/#TEST_DIR/#g"
> +
> +Commonly needed filters are available in ./common.filter.
> +
> +** Python tests
> +
> +Most tests are implemented in bash but it is difficult to interact with the 
> QMP
> +monitor.  A Python module called 'iotests' is available for tests that 
> require
> +JSON and interacting with QEMU.
> +

There are some that prefer the bash tests still, and we do have a 'standard'
way to do so... so perhaps add as well:


** Bash tests

If you wish to create a test in Bash that interacts with the QMP monitor,
'common.qemu' provides functions for interacting with multiple QEMU
processes.


> +* How to create a test
> +
> +1. Choose an unused test number
> +
> +Tests are identified by a unique number.  Look for the highest test case 
> number
> +by looking at the test files.  Then search the qemu-de...@nongnu.org mailing
> +list to check if anyone has already sent patches using the next available
> +number.  You may need to increment the number a few times to reach an unused
> +number.
> +
> +2. Create the test file
> +
> +Copy an existing test (one that most closely resembles what you wish to test)
> +to the new test number:
> +
> +  cp 001 
> +
> +3. Assign groups to the test
> +
> +Add your test to the ./group file.  This file is the index of tests and 
> assigns
> +them to functional groups like "rw" for read-write tests.  Most tests belong 
> to
> +the "rw" and "auto" groups.  "auto" means the test runs when ./check is 
> invoked
> +without a -g argument.
> +
> +Consider adding your test to the "quick" group if it executes quickly (<1s).
> +This group is run by "make check-block" and is often included as part of 
> build
> +tests in continuous integration systems.
> +
> +4. Write the test
> +
> +Edit the test script.  Look at existing tests for examples.
> +
> +5. Generate the golden master file
> +
> +Run your test with "./check ".  You may need to pass additional
> +options to use an image format or protocol.
> +
> +The test will fail because there is no golden master yet.  Inspect the output
> +that your test generated with "cat .out.bad".
> +
> +Verify that the output is as expected and contains no volatile strings like
> +timestamps.  You may need to add filters to your test to remove volatile
> +strings.
> +
> +Once you are happy with the test output it can be used as the golden master
> +with "mv .out.bad .out".  Rerun the test to verify
> +that it passes.
> +
> +Congratulations, you've created a new test!
> +
>  * Feedback and patches
>  
>  Please send improvements to the test suite, general feedback or just
>  reports of failing tests cases to qemu-de...@nongnu.org with a CC:
>  to qemu-block@nongnu.org.
> +
> -- 
> 2.9.4
> 
> 

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

[Cleber Rosa]

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.

Let me reinforce my point, and you can address it directly:  feature
checks like you proposed can easily produce false negatives.  Not
something hypothetical or far fetched, I gave very reasonable examples
on this same thread.

Do you think that's OK because the skip count will get an increment?
That's exactly one of the main concerns raised in the original thread (
and break room conversations) that motivated this experiment.

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

Please let me know If you really believe this should *not* be done here
(upstream QEMU).

Regards!

-- 
Cleber Rosa
[ Sr Software Engineer - Virtualization Team - Red Hat ]
[ Avocado Test Framework - avocado-framework.github.io ]
[  7ABB 96EB 8B46 B94D 5E0F  E9BB 657E 8D33 A5F2 09F3  ]



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [RFC] block-insert-node and block-job-delete

[Manos Pitsidianakis]

On Wed, Jul 26, 2017 at 04:12:21PM +0100, Stefan Hajnoczi wrote:

On Wed, Jul 26, 2017 at 05:19:24PM +0300, Manos Pitsidianakis wrote:

This proposal follows a discussion we had with Kevin and Stefan on filter
node management.

With block filter drivers arises a need to configure filter nodes on runtime
with QMP on live graphs. A problem with doing live graph modifications is
that some block jobs modify the graph when they are done and don't operate
under any synchronisation, resulting in a race condition if we try to insert
a filter node in the place of an existing edge.


But maybe you are thinking about higher level race conditions between
QMP commands.  Can you give an example of the race?


Exactly, synchronisation between the QMP/user actions. Here's an 
example, correct me if I'm wrong:


User issues a block-commit to this tree to commit A onto B:
   BB -> A -> B

This inserts the dummy mirror node at the top since this is a mirror job 
(active commit)

   BB -> dummy -> A -> B

User wants to add a filter node F below the BB. First we create the 
node:

   BB -> dummy -> A -> B
  F /

Commit finishes before we do block-insert-node, dummy and A is removed 
from the BB's chain, mirror_exit calls bdrv_replace_node()


   BB > B
   F -> /

Now inserting F under BB will error since dummy does not exist any more.

The proposal doesn't guard against the following:
User issues a block-commit to this tree to commit B onto C:
   BB -> A -> B -> C

The dummy node (commit's top bs is A):
   BB -> A -> dummy -> B -> C

blockdev-add of a filter we wish to eventually be between A and C:
   BB -> A -> dummy -> B -> C
F/

if commit finishes before we issue block-insert-node (commit_complete()
calls bdrv_set_backing_hd() which only touches A)
   BB -> A --> C
  F -> dummy -> B /
   resulting in error when issuing block-insert-node,

else if we set the job to manual-delete and insert F:
   BB -> A -> F -> dummy -> B -> C
deleting the job will result in F being lost since the job's top bs 
wasn't updated.




signature.asc
Description: PGP signature

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

[Stefan Hajnoczi]

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.

It is the job of the CI system to flag up pass/fail/skip transitions.
You're no worse off using feature tests.

Stefan


signature.asc
Description: PGP signature

[Qemu-block] [PATCH v2 7/7] block: add throttle block filter driver interface tests

[Manos Pitsidianakis]

Signed-off-by: Manos Pitsidianakis 
---
 tests/qemu-iotests/184 | 237 +
 tests/qemu-iotests/184.out | 319 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 557 insertions(+)
 create mode 100755 tests/qemu-iotests/184
 create mode 100644 tests/qemu-iotests/184.out

diff --git a/tests/qemu-iotests/184 b/tests/qemu-iotests/184
new file mode 100755
index 00..c3f3f2b7ca
--- /dev/null
+++ b/tests/qemu-iotests/184
@@ -0,0 +1,237 @@
+#!/bin/bash
+#
+# Test I/O throttle block filter driver interface
+#
+# Copyright (C) 2017 Manos Pitsidianakis
+#
+# 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="Manos Pitsidianakis"
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1   # failure is the default!
+
+_cleanup()
+{
+_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt raw
+_supported_proto file
+_supported_os Linux
+
+function do_run_qemu()
+{
+echo Testing: "$@" | _filter_imgfmt
+$QEMU -nographic -qmp-pretty stdio -serial none "$@"
+echo
+}
+
+function run_qemu()
+{
+do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | _filter_qmp\
+  | _filter_qemu_io | _filter_generated_node_ids
+}
+
+_make_test_img 64M
+test_throttle=$($QEMU_IMG --help|grep throttle)
+[ "$test_throttle" = "" ] && _supported_fmt throttle
+
+echo
+echo "== checking interface =="
+
+run_qemu <

[Qemu-block] [PATCH v2 6/7] block: add BlockDevOptionsThrottle to QAPI

[Manos Pitsidianakis]

This is needed to configure throttle filter driver nodes with QAPI.

Signed-off-by: Manos Pitsidianakis 
---
 qapi/block-core.json | 20 +++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0bdc69aa5f..f5ce67c4fb 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -,6 +,7 @@
 # Drivers that are supported in block device operations.
 #
 # @vxhs: Since 2.10
+# @throttle: Since 2.11
 #
 # Since: 2.9
 ##
@@ -2231,7 +2232,7 @@
 'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
 'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
 'quorum', 'raw', 'rbd', 'replication', 'sheepdog', 'ssh',
-'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] }
+'throttle', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] }
 
 ##
 # @BlockdevOptionsFile:
@@ -3095,6 +3096,22 @@
 '*tls-creds': 'str' } }
 
 ##
+# @BlockdevOptionsThrottle:
+#
+# Driver specific block device options for the throttle driver
+#
+# @throttle-group:   the name of the throttle-group object to use. It will be
+#created if it doesn't already exist
+# @file: reference to or definition of the data source block device
+# @limits:   ThrottleLimits options
+# Since: 2.11
+##
+{ 'struct': 'BlockdevOptionsThrottle',
+  'data': { '*throttle-group': 'str',
+'file' : 'BlockdevRef',
+'*limits' : 'ThrottleLimits'
+ } }
+##
 # @BlockdevOptions:
 #
 # Options for creating a block device.  Many options are available for all
@@ -3155,6 +3172,7 @@
   'replication':'BlockdevOptionsReplication',
   'sheepdog':   'BlockdevOptionsSheepdog',
   'ssh':'BlockdevOptionsSsh',
+  'throttle':   'BlockdevOptionsThrottle',
   'vdi':'BlockdevOptionsGenericFormat',
   'vhdx':   'BlockdevOptionsGenericFormat',
   'vmdk':   'BlockdevOptionsGenericCOWFormat',
-- 
2.11.0

[Qemu-block] [PATCH v2 1/7] block: move ThrottleGroup membership to ThrottleGroupMember

[Manos Pitsidianakis]

This commit eliminates the 1:1 relationship between BlockBackend and
throttle group state.  Users will be able to create multiple throttle
nodes, each with its own throttle group state, in the future.  The
throttle group state cannot be per-BlockBackend anymore, it must be
per-throttle node. This is done by gathering ThrottleGroup membership
details from BlockBackendPublic into ThrottleGroupMember and refactoring
existing code to use the structure.

Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Manos Pitsidianakis 
---
 block/block-backend.c   |  66 +
 block/qapi.c|   8 +-
 block/throttle-groups.c | 288 
 blockdev.c  |   4 +-
 include/block/throttle-groups.h |  39 +-
 include/sysemu/block-backend.h  |  20 +--
 tests/test-throttle.c   |  53 
 7 files changed, 252 insertions(+), 226 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 968438c149..bde6948d0e 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -215,9 +215,9 @@ BlockBackend *blk_new(uint64_t perm, uint64_t shared_perm)
 blk->shared_perm = shared_perm;
 blk_set_enable_write_cache(blk, true);
 
-qemu_co_mutex_init(&blk->public.throttled_reqs_lock);
-qemu_co_queue_init(&blk->public.throttled_reqs[0]);
-qemu_co_queue_init(&blk->public.throttled_reqs[1]);
+qemu_co_mutex_init(&blk->public.throttle_group_member.throttled_reqs_lock);
+qemu_co_queue_init(&blk->public.throttle_group_member.throttled_reqs[0]);
+qemu_co_queue_init(&blk->public.throttle_group_member.throttled_reqs[1]);
 block_acct_init(&blk->stats);
 
 notifier_list_init(&blk->remove_bs_notifiers);
@@ -285,7 +285,7 @@ static void blk_delete(BlockBackend *blk)
 assert(!blk->refcnt);
 assert(!blk->name);
 assert(!blk->dev);
-if (blk->public.throttle_state) {
+if (blk->public.throttle_group_member.throttle_state) {
 blk_io_limits_disable(blk);
 }
 if (blk->root) {
@@ -596,9 +596,12 @@ BlockBackend *blk_by_public(BlockBackendPublic *public)
  */
 void blk_remove_bs(BlockBackend *blk)
 {
+ThrottleTimers *tt;
+
 notifier_list_notify(&blk->remove_bs_notifiers, blk);
-if (blk->public.throttle_state) {
-throttle_timers_detach_aio_context(&blk->public.throttle_timers);
+if (blk->public.throttle_group_member.throttle_state) {
+tt = &blk->public.throttle_group_member.throttle_timers;
+throttle_timers_detach_aio_context(tt);
 }
 
 blk_update_root_state(blk);
@@ -620,9 +623,10 @@ int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, 
Error **errp)
 bdrv_ref(bs);
 
 notifier_list_notify(&blk->insert_bs_notifiers, blk);
-if (blk->public.throttle_state) {
+if (blk->public.throttle_group_member.throttle_state) {
 throttle_timers_attach_aio_context(
-&blk->public.throttle_timers, bdrv_get_aio_context(bs));
+&blk->public.throttle_group_member.throttle_timers,
+bdrv_get_aio_context(bs));
 }
 
 return 0;
@@ -984,8 +988,9 @@ int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t 
offset,
 bdrv_inc_in_flight(bs);
 
 /* throttling disk I/O */
-if (blk->public.throttle_state) {
-throttle_group_co_io_limits_intercept(blk, bytes, false);
+if (blk->public.throttle_group_member.throttle_state) {
+
throttle_group_co_io_limits_intercept(&blk->public.throttle_group_member,
+bytes, false);
 }
 
 ret = bdrv_co_preadv(blk->root, offset, bytes, qiov, flags);
@@ -1008,10 +1013,10 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, 
int64_t offset,
 }
 
 bdrv_inc_in_flight(bs);
-
 /* throttling disk I/O */
-if (blk->public.throttle_state) {
-throttle_group_co_io_limits_intercept(blk, bytes, true);
+if (blk->public.throttle_group_member.throttle_state) {
+
throttle_group_co_io_limits_intercept(&blk->public.throttle_group_member,
+bytes, true);
 }
 
 if (!blk->enable_write_cache) {
@@ -1680,15 +1685,17 @@ static AioContext *blk_aiocb_get_aio_context(BlockAIOCB 
*acb)
 void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
 {
 BlockDriverState *bs = blk_bs(blk);
+ThrottleTimers *tt;
 
 if (bs) {
-if (blk->public.throttle_state) {
-throttle_timers_detach_aio_context(&blk->public.throttle_timers);
+if (blk->public.throttle_group_member.throttle_state) {
+tt = &blk->public.throttle_group_member.throttle_timers;
+throttle_timers_detach_aio_context(tt);
 }
 bdrv_set_aio_context(bs, new_context);
-if (blk->public.throttle_state) {
-throttle_timers_attach_aio_context(&blk->public.throttle_timers,
-   new_context);
+if (blk->public.throttle_group_member.throttle_state) {
+tt = &blk->public.thr

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

[Manos Pitsidianakis]

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

The configuration flags and their semantics are identical to the
hardcoded throttling ones.

A node can be created referring to an existing group, and will overwrite
its limits if any are specified, otherwise they are retained.

Signed-off-by: Manos Pitsidianakis 
---
 block/Makefile.objs |   1 +
 block/throttle.c| 395 
 include/qemu/throttle-options.h |   1 +
 3 files changed, 397 insertions(+)
 create mode 100644 block/throttle.c

diff --git a/block/Makefile.objs b/block/Makefile.objs
index 2aaede4ae1..6eaf78a046 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -25,6 +25,7 @@ block-obj-y += accounting.o dirty-bitmap.o
 block-obj-y += write-threshold.o
 block-obj-y += backup.o
 block-obj-$(CONFIG_REPLICATION) += replication.o
+block-obj-y += throttle.o
 
 block-obj-y += crypto.o
 
diff --git a/block/throttle.c b/block/throttle.c
new file mode 100644
index 00..f3395462fb
--- /dev/null
+++ b/block/throttle.c
@@ -0,0 +1,395 @@
+/*
+ * QEMU block throttling filter driver infrastructure
+ *
+ * Copyright (c) 2017 Manos Pitsidianakis
+ *
+ * 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 or
+ * (at your option) version 3 of the License.
+ *
+ * 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 .
+ */
+
+#include "qemu/osdep.h"
+#include "block/throttle-groups.h"
+#include "qemu/throttle-options.h"
+#include "qapi/error.h"
+
+#undef THROTTLE_OPT_PREFIX
+#define THROTTLE_OPT_PREFIX "limits."
+static QemuOptsList throttle_opts = {
+.name = "throttle",
+.head = QTAILQ_HEAD_INITIALIZER(throttle_opts.head),
+.desc = {
+THROTTLE_OPTS,
+{
+.name = QEMU_OPT_THROTTLE_GROUP_NAME,
+.type = QEMU_OPT_STRING,
+.help = "throttle group name",
+},
+{ /* end of list */ }
+},
+};
+
+/* Extract ThrottleConfig options. Assumes cfg is initialized and will be
+ * checked for validity.
+ */
+static int throttle_extract_options(QemuOpts *opts, ThrottleConfig *cfg,
+ Error **errp)
+{
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_TOTAL)) {
+cfg->buckets[THROTTLE_BPS_TOTAL].avg =
+qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_TOTAL,
+0);
+}
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_READ)) {
+cfg->buckets[THROTTLE_BPS_READ].avg  =
+qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_READ,
+0);
+}
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_WRITE)) {
+cfg->buckets[THROTTLE_BPS_WRITE].avg =
+qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_WRITE,
+0);
+}
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_TOTAL)) {
+cfg->buckets[THROTTLE_OPS_TOTAL].avg =
+qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_TOTAL,
+0);
+}
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_READ)) {
+cfg->buckets[THROTTLE_OPS_READ].avg =
+qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_READ,
+0);
+}
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_WRITE)) {
+cfg->buckets[THROTTLE_OPS_WRITE].avg =
+qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_WRITE,
+0);
+}
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_TOTAL_MAX)) {
+cfg->buckets[THROTTLE_BPS_TOTAL].max =
+qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX
+QEMU_OPT_BPS_TOTAL_MAX, 0);
+}
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_READ_MAX)) {
+cfg->buckets[THROTTLE_BPS_READ].max  =
+qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX
+QEMU_OPT_BPS_READ_MAX, 0);
+}
+if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_WRITE_MAX)) {
+cfg->buckets[THROTTLE_BPS_WRITE].max =
+

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

[Manos Pitsidianakis]

ThrottleGroup is converted to an object. This will allow the future
throttle block filter drive easy creation and configuration of throttle
groups in QMP and cli.

A new QAPI struct, ThrottleLimits, is introduced to provide a shared
struct for all throttle configuration needs in QMP.

ThrottleGroups can be created via CLI as
-object throttle-group,id=foo,x-iops-total=100,x-..
where x-* are individual limit properties. Since we can't add non-scalar
properties in -object this interface must be used instead. However,
setting these properties must be disabled after initialization because
certain combinations of limits are forbidden and thus configuration
changes should be done in one transaction. The individual properties
will go away when support for non-scalar values in CLI is implemented
and thus are marked as experimental.

ThrottleGroup also has a `limits` property that uses the ThrottleLimits
struct.  It can be used to create ThrottleGroups or set the
configuration in existing groups as follows:

{ "execute": "object-add",
  "arguments": {
"qom-type": "throttle-group",
"id": "foo",
"props" : {
  "limits": {
  "iops-total": 100
  }
}
  }
}
{ "execute" : "qom-set",
"arguments" : {
"path" : "foo",
"property" : "limits",
"value" : {
"iops-total" : 99
}
}
}

This also means a group's configuration can be fetched with qom-get.

ThrottleGroups can be anonymous which means they can't get accessed by
other users ie they will always be units instead of group (Because they
have one ThrottleGroupMember).

Signed-off-by: Manos Pitsidianakis 
---
Notes:
Note: I tested Markus Armbruster's patch in 
<87wp7fghi9@dusky.pond.sub.org>
on master and I can use this syntax successfuly:
-object '{ "qom-type" : "throttle-group", "id" : "foo", "props" : { 
"limits" \
: { "iops-total" : 1000 } } }'
If this gets merged using -object will be a little more verbose but at 
least we
won't have seperate properties, which is a good thing, so the x-* should be
dropped.

 block/throttle-groups.c | 402 
 include/block/throttle-groups.h |   3 +
 include/qemu/throttle-options.h |  59 --
 include/qemu/throttle.h |   3 +
 qapi/block-core.json|  48 +
 tests/test-throttle.c   |   1 +
 util/throttle.c | 151 +++
 7 files changed, 617 insertions(+), 50 deletions(-)

diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index f711a3dc62..b9c5036e44 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -25,9 +25,17 @@
 #include "qemu/osdep.h"
 #include "sysemu/block-backend.h"
 #include "block/throttle-groups.h"
+#include "qemu/throttle-options.h"
 #include "qemu/queue.h"
 #include "qemu/thread.h"
 #include "sysemu/qtest.h"
+#include "qapi/error.h"
+#include "qapi-visit.h"
+#include "qom/object.h"
+#include "qom/object_interfaces.h"
+
+static void throttle_group_obj_init(Object *obj);
+static void throttle_group_obj_complete(UserCreatable *obj, Error **errp);
 
 /* The ThrottleGroup structure (with its ThrottleState) is shared
  * among different ThrottleGroupMembers and it's independent from
@@ -54,6 +62,10 @@
  * that ThrottleGroupMember has throttled requests in the queue.
  */
 typedef struct ThrottleGroup {
+Object parent_obj;
+
+/* refuse individual property change if initialization is complete */
+bool is_initialized;
 char *name; /* This is constant during the lifetime of the group */
 
 QemuMutex lock; /* This lock protects the following four fields */
@@ -63,8 +75,7 @@ typedef struct ThrottleGroup {
 bool any_timer_armed[2];
 QEMUClockType clock_type;
 
-/* These two are protected by the global throttle_groups_lock */
-unsigned refcount;
+/* This is protected by the global throttle_groups_lock */
 QTAILQ_ENTRY(ThrottleGroup) list;
 } ThrottleGroup;
 
@@ -77,7 +88,8 @@ static QTAILQ_HEAD(, ThrottleGroup) throttle_groups =
  * If no ThrottleGroup is found with the given name a new one is
  * created.
  *
- * @name: the name of the ThrottleGroup
+ * @name: the name of the ThrottleGroup, NULL means a new anonymous group will
+ *be created.
  * @ret:  the ThrottleState member of the ThrottleGroup
  */
 ThrottleState *throttle_group_incref(const char *name)
@@ -87,32 +99,30 @@ ThrottleState *throttle_group_incref(const char *name)
 
 qemu_mutex_lock(&throttle_groups_lock);
 
-/* Look for an existing group with that name */
-QTAILQ_FOREACH(iter, &throttle_groups, list) {
-if (!strcmp(name, iter->name)) {
-tg = iter;
-break;
+if (name) {
+/* Look for an existing group with that name */
+QTAILQ_FOREACH(iter, &throttle_groups, list) {
+if (!g_strcmp0(name, iter->name)) {
+tg = iter;
+break;
+}
 }
 }
 
 /* Create a 

[Qemu-block] [PATCH v2 0/7] add throttle block driver filter

[Manos Pitsidianakis]

This series adds a throttle block driver filter. Currently throttling is done
at the BlockBackend level. Using block driver interfaces we can move the
throttling to any point in the BDS graph using a throttle node which uses the
existing throttling code. This allows for potentially more complex
configurations (throttling at any point in the graph, chained filters)

There's also the question of how filter drivers affect the rest of the block
layer, especially live block jobs. This won't be tackled in this series though.

v2:
  change QOM throttle group object name
  print valid ranges for uint on error
  move frees in throttle_group_obj_finalize()
  split throttle_group_{set,get}()
  add throttle_recurse_is_first_non_filter()

Manos Pitsidianakis (7):
  block: move ThrottleGroup membership to ThrottleGroupMember
  block: add aio_context field in ThrottleGroupMember
  block: tidy ThrottleGroupMember initializations
  block: convert ThrottleGroup to object with QOM
  block: add throttle block filter driver
  block: add BlockDevOptionsThrottle to QAPI
  block: add throttle block filter driver interface tests

 block/Makefile.objs |   1 +
 block/block-backend.c   |  61 ++--
 block/qapi.c|   8 +-
 block/throttle-groups.c | 715 ++--
 block/throttle.c| 395 ++
 blockdev.c  |   4 +-
 include/block/throttle-groups.h |  47 ++-
 include/qemu/throttle-options.h |  60 ++--
 include/qemu/throttle.h |   3 +
 include/sysemu/block-backend.h  |  20 +-
 qapi/block-core.json|  68 +++-
 tests/qemu-iotests/184  | 237 +
 tests/qemu-iotests/184.out  | 319 ++
 tests/qemu-iotests/group|   1 +
 tests/test-throttle.c   | 111 ---
 util/throttle.c | 151 +
 16 files changed, 1891 insertions(+), 310 deletions(-)
 create mode 100644 block/throttle.c
 create mode 100755 tests/qemu-iotests/184
 create mode 100644 tests/qemu-iotests/184.out

-- 
2.11.0

[Qemu-block] [PATCH v2 3/7] block: tidy ThrottleGroupMember initializations

[Manos Pitsidianakis]

Move the CoMutex and CoQueue inits inside throttle_group_register_tgm()
which is called whenever a ThrottleGroupMember is initialized. There's
no need for them to be separate.

Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Manos Pitsidianakis 
---
 block/block-backend.c   | 3 ---
 block/throttle-groups.c | 3 +++
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 1fcf6055ff..74c1e7e058 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -215,9 +215,6 @@ BlockBackend *blk_new(uint64_t perm, uint64_t shared_perm)
 blk->shared_perm = shared_perm;
 blk_set_enable_write_cache(blk, true);
 
-qemu_co_mutex_init(&blk->public.throttle_group_member.throttled_reqs_lock);
-qemu_co_queue_init(&blk->public.throttle_group_member.throttled_reqs[0]);
-qemu_co_queue_init(&blk->public.throttle_group_member.throttled_reqs[1]);
 block_acct_init(&blk->stats);
 
 notifier_list_init(&blk->remove_bs_notifiers);
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index a979e86243..f711a3dc62 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -508,6 +508,9 @@ void throttle_group_register_tgm(ThrottleGroupMember *tgm,
  read_timer_cb,
  write_timer_cb,
  tgm);
+qemu_co_mutex_init(&tgm->throttled_reqs_lock);
+qemu_co_queue_init(&tgm->throttled_reqs[0]);
+qemu_co_queue_init(&tgm->throttled_reqs[1]);
 
 qemu_mutex_unlock(&tg->lock);
 }
-- 
2.11.0

[Qemu-block] [PATCH v2 2/7] block: add aio_context field in ThrottleGroupMember

[Manos Pitsidianakis]

timer_cb() needs to know about the current Aio context of the throttle
request that is woken up. In order to make ThrottleGroupMember backend
agnostic, this information is stored in an aio_context field instead of
accessing it from BlockBackend.

Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Manos Pitsidianakis 
---
 block/block-backend.c   | 14 -
 block/throttle-groups.c | 38 -
 include/block/throttle-groups.h |  7 -
 tests/test-throttle.c   | 63 +
 4 files changed, 69 insertions(+), 53 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index bde6948d0e..1fcf6055ff 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1685,17 +1685,15 @@ static AioContext *blk_aiocb_get_aio_context(BlockAIOCB 
*acb)
 void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
 {
 BlockDriverState *bs = blk_bs(blk);
-ThrottleTimers *tt;
+ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
 
 if (bs) {
-if (blk->public.throttle_group_member.throttle_state) {
-tt = &blk->public.throttle_group_member.throttle_timers;
-throttle_timers_detach_aio_context(tt);
+if (tgm->throttle_state) {
+throttle_group_detach_aio_context(tgm);
 }
 bdrv_set_aio_context(bs, new_context);
-if (blk->public.throttle_group_member.throttle_state) {
-tt = &blk->public.throttle_group_member.throttle_timers;
-throttle_timers_attach_aio_context(tt, new_context);
+if (tgm->throttle_state) {
+throttle_group_attach_aio_context(tgm, new_context);
 }
 }
 }
@@ -1929,7 +1927,7 @@ void blk_io_limits_disable(BlockBackend *blk)
 void blk_io_limits_enable(BlockBackend *blk, const char *group)
 {
 assert(!blk->public.throttle_group_member.throttle_state);
-throttle_group_register_tgm(&blk->public.throttle_group_member, group);
+throttle_group_register_tgm(&blk->public.throttle_group_member, group, 
blk_get_aio_context(blk));
 }
 
 void blk_io_limits_update_group(BlockBackend *blk, const char *group)
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index c8ed16ddf8..a979e86243 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -391,9 +391,6 @@ static void coroutine_fn 
throttle_group_restart_queue_entry(void *opaque)
 
 static void throttle_group_restart_queue(ThrottleGroupMember *tgm, bool 
is_write)
 {
-BlockBackendPublic *blkp = container_of(tgm, BlockBackendPublic,
-throttle_group_member);
-BlockBackend *blk = blk_by_public(blkp);
 Coroutine *co;
 RestartData rd = {
 .tgm = tgm,
@@ -401,7 +398,7 @@ static void 
throttle_group_restart_queue(ThrottleGroupMember *tgm, bool is_write
 };
 
 co = qemu_coroutine_create(throttle_group_restart_queue_entry, &rd);
-aio_co_enter(blk_get_aio_context(blk), co);
+aio_co_enter(tgm->aio_context, co);
 }
 
 void throttle_group_restart_tgm(ThrottleGroupMember *tgm)
@@ -449,13 +446,11 @@ void throttle_group_get_config(ThrottleGroupMember *tgm, 
ThrottleConfig *cfg)
 /* ThrottleTimers callback. This wakes up a request that was waiting
  * because it had been throttled.
  *
- * @blk:   the BlockBackend whose request had been throttled
+ * @tgm:   the ThrottleGroupMember whose request had been throttled
  * @is_write:  the type of operation (read/write)
  */
-static void timer_cb(BlockBackend *blk, bool is_write)
+static void timer_cb(ThrottleGroupMember *tgm, bool is_write)
 {
-BlockBackendPublic *blkp = blk_get_public(blk);
-ThrottleGroupMember *tgm = &blkp->throttle_group_member;
 ThrottleState *ts = tgm->throttle_state;
 ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
 
@@ -484,18 +479,18 @@ static void write_timer_cb(void *opaque)
  *
  * @tgm:   the ThrottleGroupMember to insert
  * @groupname: the name of the group
+ * @ctx:   the AioContext to use
  */
 void throttle_group_register_tgm(ThrottleGroupMember *tgm,
- const char *groupname)
+ const char *groupname,
+ AioContext *ctx)
 {
 int i;
-BlockBackendPublic *blkp = container_of(tgm, BlockBackendPublic,
-throttle_group_member);
-BlockBackend *blk = blk_by_public(blkp);
 ThrottleState *ts = throttle_group_incref(groupname);
 ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
 
 tgm->throttle_state = ts;
+tgm->aio_context = ctx;
 
 qemu_mutex_lock(&tg->lock);
 /* If the ThrottleGroup is new set this ThrottleGroupMember as the token */
@@ -508,11 +503,11 @@ void throttle_group_register_tgm(ThrottleGroupMember *tgm,
 QLIST_INSERT_HEAD(&tg->head, tgm, round_robin);
 
 throttle_timers_init(&tgm->throttle_timers,
- blk_get_aio_context(blk),
+ tgm->aio

Re: [Qemu-block] [RFC] block-insert-node and block-job-delete

[Stefan Hajnoczi]

On Wed, Jul 26, 2017 at 05:19:24PM +0300, Manos Pitsidianakis wrote:
> This proposal follows a discussion we had with Kevin and Stefan on filter
> node management.
> 
> With block filter drivers arises a need to configure filter nodes on runtime
> with QMP on live graphs. A problem with doing live graph modifications is
> that some block jobs modify the graph when they are done and don't operate
> under any synchronisation, resulting in a race condition if we try to insert
> a filter node in the place of an existing edge.

Block jobs *do* operate under synchronization.  They only manipulate the
graph from the main loop (under the QEMU global mutex just like a
monitor command).

But maybe you are thinking about higher level race conditions between
QMP commands.  Can you give an example of the race?

CCing John Snow (interested in commands for 'reaping' completed block
jobs) and Jeff Cody (block jobs maintainer).


signature.asc
Description: PGP signature

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

[Eric Blake]

On 07/26/2017 09:10 AM, Peter Maydell wrote:
> On 26 July 2017 at 15:02, Eric Blake  wrote:
>> 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.
>>

>>
>> +/* Bug reporting information for --help arguments, About dialogs, etc */
>> +#define QEMU_BUGREPORTS \
>> +"See  for bug reports.\n" \
>> +"More information on the qemu project at "
> 
> QEMU should be all upper case.

Will fix.  While at it, it looks weird that one sentence ends in '.' but
not the other; I omitted it on the second on the chance that it might
interfere with terminals that are able to auto-click the link, but in
testing, at least Gnome's terminal manages just fine thanks to the <>
bracketing.

> 
>> +
> 
> I wonder if we may regret the embedded newline when
> we come to handling UI about dialogs. I guess we
> can cross that bridge when we come to it (they
> may need to special case it anyway if they want
> to make the links clicky).

ui/cocoa.m appears to be the only UI expression version information in a
GUI (which I have no way to test); the GTK window that I get for
'./x86_64-softmmu/qemu-system-x86_64' didn't have anything in its
menubar.  Perhaps it should, but I'm not a gui guy, so I'll leave that
patch for someone with more experience.

Also, I hit this warning:

$ ./x86_64-softmmu/qemu-system-x86_64

(qemu-system-x86_64:19518): Gtk-WARNING **: Allocating size to
GtkScrollbar 0x556c628be340 without calling
gtk_widget_get_preferred_width/height(). How does the code know the size
to allocate?

(qemu-system-x86_64:19518): Gtk-WARNING **: gtk_widget_size_allocate():
attempt to allocate widget with width -426186808 and height 400

I have no idea where it's coming from (that is, whether it is a bug in
the gtk stack of Fedora 26, or whether it is the fault of qemu itself
misusing the preferred dance of gtk calls), but again that's something
that I would rather leave to a programmer more familiar with gui issues
(you can tell I usually test './x86_64-softmmu/qemu-system-x86_64
-nodefaults --nographic -qmp stdio')

-- 
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] [RFC] block-insert-node and block-job-delete

[Manos Pitsidianakis]

This proposal follows a discussion we had with Kevin and Stefan on 
filter node management.


With block filter drivers arises a need to configure filter nodes on 
runtime with QMP on live graphs. A problem with doing live graph 
modifications is that some block jobs modify the graph when they are 
done and don't operate under any synchronisation, resulting in a race 
condition if we try to insert a filter node in the place of an existing 
edge.


The race can be overcome if we introduce an optional manual-delete flag 
in the creation of a block job to indicate that they will not be deleted 
automatically, but rather wait until the user explicitly calls 
block-job-delete to remove the block job and apply the graph 
modifications. This makes filter insertion require that there are no 
active block jobs with manual-delete set to false. The graph operations 
will be synchronous unlike block-job-complete; the 
BlockJobDeferToMainLoopFn completion callback will be executed from QMP 
instead of using block_job_defer_to_main_loop.


block-job-delete will be defined as 
{ 'command': 'block-job-delete', 'data': { 'device': 'str' } }


With this change we can define block-insert-node. New nodes will be 
created with blockdev-add with the appropriate children nodes. On 
calling block-insert-node we specify the node to add, and the edge we 
wish to replace:

{ 'command': 'block-insert-node',
   'data' : { '*parent' :  'str', 'child' : 'str', 'node' : 'str', 
   '*device' : 'str' } }


This will be similar to x-blockdev-change, but instead of bdrv_add_child 
the parent driver must implement bdrv_reopen_* to change the child. If 
instead of parent we specify device, the node will be inserted as the 
root bs of the specified BlockBackend device. If 'child' is not in the 
bs->children of 'node', we should abort. I'm not certain if there's need 
to implement an option between bs->file/bs->backing.


In the following example we insert a throttle filter node (T) between A 
and B:


   A
   |
   B

{ "execute": "blockdev-add",
 "arguments": {
   "driver": "throttle",
   "node-name": "T",
   "throttling-group": "foobar",
   "limits" : {
 "iops-total" : 1000,
   },
   "file": "B"
 }
}

 A  T
 \ /
  B

{ "execute" : "block-insert-node",
"arguments" : {
 "parent" : "A",
 "child" : "B",
 "node" : "T"
 }
}
 A
 |
 T
 |
 B

If bdrv_reopen is ever introduced to QMP this command, except for the BB 
root case, might be obsolete.


signature.asc
Description: PGP signature

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

[Peter Maydell]

On 26 July 2017 at 15:02, Eric Blake  wrote:
> 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" \
> +"More information on the qemu project at "

QEMU should be all upper case.

> +

I wonder if we may regret the embedded newline when
we come to handling UI about dialogs. I guess we
can cross that bridge when we come to it (they
may need to special case it anyway if they want
to make the links clicky).

thanks
-- PMM

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

[Eric Blake]

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" \
+"More information on the qemu project at "
+
 /* main function, renamed */
 #if defined(CONFIG_COCOA)
 int qemu_main(int argc, char **argv, char **envp);
diff --git a/vl.c b/vl.c
index fb6b2efafa..b824f81f64 100644
--- a/vl.c
+++ b/vl.c
@@ -1942,7 +1942,9 @@ static void help(int exitcode)
"ctrl-alt-n  switch to virtual console 'n'\n"
"ctrl-alttoggle mouse and keyboard grab\n"
"\n"
-   "When using -nographic, press 'ctrl-a h' to get some help.\n");
+   "When using -nographic, press 'ctrl-a h' to get some help.\n"
+   "\n"
+   QEMU_BUGREPORTS "\n");

 exit(exitcode);
 }
diff --git a/bsd-user/main.c b/bsd-user/main.c
index 501e16f675..4db10cb376 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -686,6 +686,8 @@ static void usage(void)
"-E var1=val2 -E var2=val2 -U LD_PRELOAD -U LD_DEBUG\n"
"Note that if you provide several changes to single variable\n"
"last change will stay in effect.\n"
+   "\n"
+   QEMU_BUGREPORTS "\n"
,
TARGET_NAME,
interp_prefix,
diff --git a/linux-user/main.c b/linux-user/main.c
index 2b38d39d87..7d6e481277 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -4136,7 +4136,9 @@ static void usage(int exitcode)
"-E var1=val2,var2=val2 -U LD_PRELOAD,LD_DEBUG\n"
"QEMU_SET_ENV=var1=val2,var2=val2 
QEMU_UNSET_ENV=LD_PRELOAD,LD_DEBUG\n"
"Note that if you provide several changes to a single variable\n"
-   "the last change will stay in effect.\n");
+   "the last change will stay in effect.\n"
+   "\n"
+   QEMU_BUGREPORTS "\n");

 exit(exitcode);
 }
diff --git a/qemu-img.c b/qemu-img.c
index f4d5f0d77d..758719e083 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");
+printf("\n\n" QEMU_BUGREPORTS "\n");
 exit(EXIT_SUCCESS);
 }

diff --git a/qemu-io.c b/qemu-io.c
index ec175630a6..b93553a603 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -262,8 +262,9 @@ static void usage(const char *name)
 "  -h, --help   display this help and exit\n"
 "  -V, --versionoutput version information and exit\n"
 "\n"
-"See '%s -c help' for information on available commands."
-"\n",
+"See '%s -c help' for information on available commands.\n"
+"\n"
+QEMU_BUGREPORTS "\n",
 name, name);
 }

diff --git a/qemu-nbd.c b/qemu-nbd.c
index b8666bb575..052eb4d067 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"
-"Report bugs to \n"
+QEMU_BUGREPORTS "\n"
 , name, NBD_DEFAULT_PORT, "DEVICE");
 }

diff --git a/qga/main.c b/qga/main.c
index b64c7ac2a2..56d5633c13 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"
-"Report bugs to \n"
+QEMU_BUGREPORTS "\n"
 , cmd, QGA_VIRTIO_PATH_DEFAULT, QGA_SERIAL_PATH_DEFAULT,
 dfl_pathnames.pidfile,

[Qemu-block] [PATCH 1/3] qemu-io: Give more --version information

[Eric Blake]

Include the package version information (useful for detecting
builds from git or downstream backports), and the copyright notice.

Signed-off-by: Eric Blake 
---
 qemu-io.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qemu-io.c b/qemu-io.c
index 4cfa41c8f9..ec175630a6 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -26,6 +26,7 @@
 #include "block/block_int.h"
 #include "trace/control.h"
 #include "crypto/init.h"
+#include "qemu-version.h"

 #define CMD_NOFILE_OK   0x01

@@ -522,7 +523,8 @@ int main(int argc, char **argv)
 trace_file = trace_opt_parse(optarg);
 break;
 case 'V':
-printf("%s version %s\n", progname, QEMU_VERSION);
+printf("%s version " QEMU_VERSION QEMU_PKGVERSION "\n"
+   QEMU_COPYRIGHT "\n", progname);
 exit(0);
 case 'h':
 usage(progname);
-- 
2.13.3

Re: [Qemu-block] [Qemu-devel] [PATCH v3] docs: add qemu-block-drivers(7) man page

[Daniel P. Berrange]

On Wed, Jul 26, 2017 at 02:03:37PM +0100, Stefan Hajnoczi wrote:
> Block driver documentation is available in qemu-doc.html.  It would be
> convenient to have documentation for formats, protocols, and filter
> drivers in a man page.
> 
> Extract the relevant part of qemu-doc.html into a new file called
> docs/qemu-block-drivers.texi.  This file can also be built as a
> stand-alone document (man, html, etc).

Is there perhaps benefit it taking it further and creating one
man page per block driver. Many of the block drivers are optional
and can be turned off, so this would let us leave out the docs
for disabled block drivers too.

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 v2] docs: add qemu-block-drivers(7) man page

[Stefan Hajnoczi]

On Mon, Jul 24, 2017 at 11:46:14AM +0200, Kevin Wolf wrote:
> Am 27.06.2017 um 17:41 hat Stefan Hajnoczi geschrieben:
> > Extract the relevant part of qemu-doc.html into a new file called
> > docs/qemu-block-drivers.texi.  This file can also be built as a
> > stand-alone document (man, html, etc).
> > 
> > Signed-off-by: Stefan Hajnoczi 
> > Reviewed-by: John Snow 
> 
> Looks good, but doesn't apply any more.
> 
> A rebased version can still be included in QEMU 2.10, documentation
> improvements are fine during the freeze.

Thanks, I have sent v3 to resolve the conflict.

Stefan


signature.asc
Description: PGP signature

[Qemu-block] [PATCH v3] docs: add qemu-block-drivers(7) man page

[Stefan Hajnoczi]

Block driver documentation is available in qemu-doc.html.  It would be
convenient to have documentation for formats, protocols, and filter
drivers in a man page.

Extract the relevant part of qemu-doc.html into a new file called
docs/qemu-block-drivers.texi.  This file can also be built as a
stand-alone document (man, html, etc).

Signed-off-by: Stefan Hajnoczi 
---
v3:
 * Rebased due to LUKS documentation conflict [Kevin]

 Makefile |  23 +-
 docs/qemu-block-drivers.texi | 799 +++
 qemu-doc.texi| 780 +-
 3 files changed, 818 insertions(+), 784 deletions(-)
 create mode 100644 docs/qemu-block-drivers.texi

diff --git a/Makefile b/Makefile
index ef721480eb..67edeffbfe 100644
--- a/Makefile
+++ b/Makefile
@@ -209,6 +209,9 @@ ifdef BUILD_DOCS
 DOCS=qemu-doc.html qemu-doc.txt qemu.1 qemu-img.1 qemu-nbd.8 qemu-ga.8
 DOCS+=docs/interop/qemu-qmp-ref.html docs/interop/qemu-qmp-ref.txt 
docs/interop/qemu-qmp-ref.7
 DOCS+=docs/interop/qemu-ga-ref.html docs/interop/qemu-ga-ref.txt 
docs/interop/qemu-ga-ref.7
+DOCS+=docs/qemu-block-drivers.html
+DOCS+=docs/qemu-block-drivers.txt
+DOCS+=docs/qemu-block-drivers.7
 ifdef CONFIG_VIRTFS
 DOCS+=fsdev/virtfs-proxy-helper.1
 endif
@@ -530,6 +533,8 @@ distclean: clean
rm -f docs/interop/qemu-qmp-ref.txt docs/interop/qemu-ga-ref.txt
rm -f docs/interop/qemu-qmp-ref.pdf docs/interop/qemu-ga-ref.pdf
rm -f docs/interop/qemu-qmp-ref.html docs/interop/qemu-ga-ref.html
+   rm -f docs/qemu-block-drivers.7 docs/qemu-block-drivers.txt
+   rm -f docs/qemu-block-drivers.pdf docs/qemu-block-drivers.html
for d in $(TARGET_DIRS); do \
rm -rf $$d || exit 1 ; \
 done
@@ -570,11 +575,14 @@ install-doc: $(DOCS)
$(INSTALL_DATA) qemu-doc.txt "$(DESTDIR)$(qemu_docdir)"
$(INSTALL_DATA) docs/interop/qemu-qmp-ref.html 
"$(DESTDIR)$(qemu_docdir)"
$(INSTALL_DATA) docs/interop/qemu-qmp-ref.txt "$(DESTDIR)$(qemu_docdir)"
+   $(INSTALL_DATA) docs/qemu-block-drivers.html "$(DESTDIR)$(qemu_docdir)"
+   $(INSTALL_DATA) docs/qemu-block-drivers.txt "$(DESTDIR)$(qemu_docdir)"
 ifdef CONFIG_POSIX
$(INSTALL_DIR) "$(DESTDIR)$(mandir)/man1"
$(INSTALL_DATA) qemu.1 "$(DESTDIR)$(mandir)/man1"
$(INSTALL_DIR) "$(DESTDIR)$(mandir)/man7"
$(INSTALL_DATA) docs/interop/qemu-qmp-ref.7 "$(DESTDIR)$(mandir)/man7"
+   $(INSTALL_DATA) docs/qemu-block-drivers.7 "$(DESTDIR)$(mandir)/man7"
 ifneq ($(TOOLS),)
$(INSTALL_DATA) qemu-img.1 "$(DESTDIR)$(mandir)/man1"
$(INSTALL_DIR) "$(DESTDIR)$(mandir)/man8"
@@ -721,15 +729,15 @@ fsdev/virtfs-proxy-helper.1: 
fsdev/virtfs-proxy-helper.texi
 qemu-nbd.8: qemu-nbd.texi qemu-option-trace.texi
 qemu-ga.8: qemu-ga.texi
 
-html: qemu-doc.html docs/interop/qemu-qmp-ref.html 
docs/interop/qemu-ga-ref.html
-info: qemu-doc.info docs/interop/qemu-qmp-ref.info 
docs/interop/qemu-ga-ref.info
-pdf: qemu-doc.pdf docs/interop/qemu-qmp-ref.pdf docs/interop/qemu-ga-ref.pdf
-txt: qemu-doc.txt docs/interop/qemu-qmp-ref.txt docs/interop/qemu-ga-ref.txt
+html: qemu-doc.html docs/interop/qemu-qmp-ref.html 
docs/interop/qemu-ga-ref.html docs/qemu-block-drivers.html
+info: qemu-doc.info docs/interop/qemu-qmp-ref.info 
docs/interop/qemu-ga-ref.info docs/qemu-block-drivers.info
+pdf: qemu-doc.pdf docs/interop/qemu-qmp-ref.pdf docs/interop/qemu-ga-ref.pdf 
docs/qemu-block-drivers.pdf
+txt: qemu-doc.txt docs/interop/qemu-qmp-ref.txt docs/interop/qemu-ga-ref.txt 
docs/qemu-block-drivers.txt
 
 qemu-doc.html qemu-doc.info qemu-doc.pdf qemu-doc.txt: \
qemu-img.texi qemu-nbd.texi qemu-options.texi qemu-option-trace.texi \
qemu-monitor.texi qemu-img-cmds.texi qemu-ga.texi \
-   qemu-monitor-info.texi
+   qemu-monitor-info.texi docs/qemu-block-drivers.texi
 
 docs/interop/qemu-ga-ref.dvi docs/interop/qemu-ga-ref.html \
 docs/interop/qemu-ga-ref.info docs/interop/qemu-ga-ref.pdf \
@@ -741,6 +749,11 @@ docs/interop/qemu-qmp-ref.dvi 
docs/interop/qemu-qmp-ref.html \
 docs/interop/qemu-qmp-ref.txt docs/interop/qemu-qmp-ref.7: \
docs/interop/qemu-qmp-ref.texi docs/interop/qemu-qmp-qapi.texi
 
+docs/qemu-block-drivers.dvi docs/qemu-block-drivers.html \
+docs/qemu-block-drivers.info docs/qemu-block-drivers.pdf \
+docs/qemu-block-drivers.txt docs/qemu-block-drivers.7: \
+   docs/qemu-block-drivers.texi
+
 
 ifdef CONFIG_WIN32
 
diff --git a/docs/qemu-block-drivers.texi b/docs/qemu-block-drivers.texi
new file mode 100644
index 00..43ec3faf15
--- /dev/null
+++ b/docs/qemu-block-drivers.texi
@@ -0,0 +1,799 @@
+@c man begin SYNOPSIS
+QEMU block driver reference manual
+@c man end
+
+@c man begin DESCRIPTION
+
+@node disk_images_formats
+
+@subsection Disk image file formats
+
+QEMU supports many image file formats that can be used with VMs as well as with
+any of the tools (like @code{qemu-img}). This includes the preferred for

[Qemu-block] [PATCH 5/6] hw/block: Use errp directly rather than local_err

[Mao Zhongyi]

Pass the error message to errp directly rather than the local
variable local_err and propagate it to errp via error_propagate().

Cc: John Snow 
Cc: Kevin Wolf 
Cc: Max Reitz 
Cc: Keith Busch 
Cc: Stefan Hajnoczi 
Cc: "Michael S. Tsirkin" 
Cc: Paolo Bonzini 
Cc: Gerd Hoffmann 
Cc: Markus Armbruster 

Signed-off-by: Mao Zhongyi 
---
 hw/block/fdc.c| 17 ++---
 hw/block/nvme.c   |  8 +++-
 hw/block/virtio-blk.c | 17 +
 hw/ide/qdev.c | 12 
 hw/scsi/scsi-disk.c   | 13 -
 hw/usb/dev-storage.c  |  9 +++--
 6 files changed, 25 insertions(+), 51 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 02f4a46..e6398c3 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -473,16 +473,13 @@ static void fd_revalidate(FDrive *drv)
 static void fd_change_cb(void *opaque, bool load, Error **errp)
 {
 FDrive *drive = opaque;
-Error *local_err = NULL;
 
 if (!load) {
 blk_set_perm(drive->blk, 0, BLK_PERM_ALL, &error_abort);
 } else {
-blkconf_apply_backend_options(drive->conf,
-  blk_is_read_only(drive->blk), false,
-  &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+if (blkconf_apply_backend_options(drive->conf,
+  blk_is_read_only(drive->blk),
+  false, errp) < 0) {
 return;
 }
 }
@@ -522,7 +519,6 @@ static void floppy_drive_realize(DeviceState *qdev, Error 
**errp)
 FloppyDrive *dev = FLOPPY_DRIVE(qdev);
 FloppyBus *bus = FLOPPY_BUS(qdev->parent_bus);
 FDrive *drive;
-Error *local_err = NULL;
 int ret;
 
 if (dev->unit == -1) {
@@ -568,10 +564,9 @@ static void floppy_drive_realize(DeviceState *qdev, Error 
**errp)
 dev->conf.rerror = BLOCKDEV_ON_ERROR_AUTO;
 dev->conf.werror = BLOCKDEV_ON_ERROR_AUTO;
 
-blkconf_apply_backend_options(&dev->conf, blk_is_read_only(dev->conf.blk),
-  false, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+if (blkconf_apply_backend_options(&dev->conf,
+  blk_is_read_only(dev->conf.blk),
+  false, errp) < 0) {
 return;
 }
 
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 43c60ab..2a9d03b 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -928,7 +928,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
 int i;
 int64_t bs_size;
 uint8_t *pci_conf;
-Error *local_err = NULL;
 
 if (!n->conf.blk) {
 error_setg(errp, "drive property not set");
@@ -947,10 +946,9 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
 return;
 }
 blkconf_blocksizes(&n->conf);
-blkconf_apply_backend_options(&n->conf, blk_is_read_only(n->conf.blk),
-  false, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+if (blkconf_apply_backend_options(&n->conf,
+  blk_is_read_only(n->conf.blk),
+  false, errp) < 0) {
 return;
 }
 
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index b750bd8..5a4e9d2 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -911,7 +911,6 @@ static void virtio_blk_device_realize(DeviceState *dev, 
Error **errp)
 VirtIODevice *vdev = VIRTIO_DEVICE(dev);
 VirtIOBlock *s = VIRTIO_BLK(dev);
 VirtIOBlkConf *conf = &s->conf;
-Error *err = NULL;
 unsigned i;
 
 if (!conf->conf.blk) {
@@ -928,17 +927,13 @@ static void virtio_blk_device_realize(DeviceState *dev, 
Error **errp)
 }
 
 blkconf_serial(&conf->conf, &conf->serial);
-blkconf_apply_backend_options(&conf->conf,
-  blk_is_read_only(conf->conf.blk), true,
-  &err);
-if (err) {
-error_propagate(errp, err);
+if (blkconf_apply_backend_options(&conf->conf,
+  blk_is_read_only(conf->conf.blk),
+  true, errp) < 0) {
 return;
 }
 s->original_wce = blk_enable_write_cache(conf->conf.blk);
-blkconf_geometry(&conf->conf, NULL, 65535, 255, 255, &err);
-if (err) {
-error_propagate(errp, err);
+if (blkconf_geometry(&conf->conf, NULL, 65535, 255, 255, errp) < 0) {
 return;
 }
 blkconf_blocksizes(&conf->conf);
@@ -953,9 +948,7 @@ static void virtio_blk_device_realize(DeviceState *dev, 
Error **errp)
 for (i = 0; i < conf->num_queues; i++) {
 virtio_add_queue(vdev, 128, virtio_blk_handle_output);
 }
-virtio_blk_data_plane_create(vdev, conf, &s->dataplane, &err);
-if (err != NULL) {
-error_propagate(errp, err);
+if (virtio_blk_data_plane_crea

[Qemu-block] [PATCH 4/6] hw/block: Fix the return type

[Mao Zhongyi]

When the function no success value to transmit, it usually make the
function return void. It has turned out not to be a success, because
it means that the extra local_err variable and error_propagate() will
be needed. It leads to cumbersome code, therefore, transmit success/
failure in the return value is worth.

So fix the return type of blkconf_apply_backend_options(),
blkconf_geometry() and virtio_blk_data_plane_create() to avoid it.

Cc: John Snow 
Cc: Kevin Wolf 
Cc: Max Reitz 
Cc: Stefan Hajnoczi 

Signed-off-by: Mao Zhongyi 
---
 hw/block/block.c| 21 -
 hw/block/dataplane/virtio-blk.c | 16 +---
 hw/block/dataplane/virtio-blk.h |  6 +++---
 include/hw/block/block.h| 10 +-
 4 files changed, 29 insertions(+), 24 deletions(-)

diff --git a/hw/block/block.c b/hw/block/block.c
index 27878d0..717bd0e 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -51,8 +51,8 @@ void blkconf_blocksizes(BlockConf *conf)
 }
 }
 
-void blkconf_apply_backend_options(BlockConf *conf, bool readonly,
-   bool resizable, Error **errp)
+int blkconf_apply_backend_options(BlockConf *conf, bool readonly,
+  bool resizable, Error **errp)
 {
 BlockBackend *blk = conf->blk;
 BlockdevOnError rerror, werror;
@@ -76,7 +76,7 @@ void blkconf_apply_backend_options(BlockConf *conf, bool 
readonly,
 
 ret = blk_set_perm(blk, perm, shared_perm, errp);
 if (ret < 0) {
-return;
+return -1;
 }
 
 switch (conf->wce) {
@@ -99,11 +99,13 @@ void blkconf_apply_backend_options(BlockConf *conf, bool 
readonly,
 
 blk_set_enable_write_cache(blk, wce);
 blk_set_on_error(blk, rerror, werror);
+
+return 0;
 }
 
-void blkconf_geometry(BlockConf *conf, int *ptrans,
-  unsigned cyls_max, unsigned heads_max, unsigned secs_max,
-  Error **errp)
+int blkconf_geometry(BlockConf *conf, int *ptrans,
+ unsigned cyls_max, unsigned heads_max, unsigned secs_max,
+ Error **errp)
 {
 DriveInfo *dinfo;
 
@@ -129,15 +131,16 @@ void blkconf_geometry(BlockConf *conf, int *ptrans,
 if (conf->cyls || conf->heads || conf->secs) {
 if (conf->cyls < 1 || conf->cyls > cyls_max) {
 error_setg(errp, "cyls must be between 1 and %u", cyls_max);
-return;
+return -1;
 }
 if (conf->heads < 1 || conf->heads > heads_max) {
 error_setg(errp, "heads must be between 1 and %u", heads_max);
-return;
+return -1;
 }
 if (conf->secs < 1 || conf->secs > secs_max) {
 error_setg(errp, "secs must be between 1 and %u", secs_max);
-return;
+return -1;
 }
 }
+return 0;
 }
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 5556f0e..619bc5e 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -76,9 +76,9 @@ static void notify_guest_bh(void *opaque)
 }
 
 /* Context: QEMU global mutex held */
-void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
-  VirtIOBlockDataPlane **dataplane,
-  Error **errp)
+int virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
+ VirtIOBlockDataPlane **dataplane,
+ Error **errp)
 {
 VirtIOBlockDataPlane *s;
 BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
@@ -91,11 +91,11 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, 
VirtIOBlkConf *conf,
 error_setg(errp,
"device is incompatible with iothread "
"(transport does not support notifiers)");
-return;
+return -1;
 }
 if (!virtio_device_ioeventfd_enabled(vdev)) {
 error_setg(errp, "ioeventfd is required for iothread");
-return;
+return -1;
 }
 
 /* If dataplane is (re-)enabled while the guest is running there could
@@ -103,12 +103,12 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, 
VirtIOBlkConf *conf,
  */
 if (blk_op_is_blocked(conf->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
 error_prepend(errp, "cannot start virtio-blk dataplane: ");
-return;
+return -1;
 }
 }
 /* Don't try if transport does not support notifiers. */
 if (!virtio_device_ioeventfd_enabled(vdev)) {
-return;
+return -1;
 }
 
 s = g_new0(VirtIOBlockDataPlane, 1);
@@ -126,6 +126,8 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, 
VirtIOBlkConf *conf,
 s->batch_notify_vqs = bitmap_new(conf->num_queues);
 
 *dataplane = s;
+
+return 0;
 }
 
 /* Context: QEMU global mutex held */
diff --git a/hw/block/dataplane/

[Qemu-block] [PATCH 3/6] hw/block/nvme: Convert to realize

[Mao Zhongyi]

Convert nvme_init() to realize and rename it to nvme_realize().

Cc: Keith Busch 
Cc: Kevin Wolf 
Cc: Max Reitz 
Cc: Markus Armbruster 

Signed-off-by: Mao Zhongyi 
---
 hw/block/nvme.c | 18 ++
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 6071dc1..43c60ab 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -920,7 +920,7 @@ static const MemoryRegionOps nvme_cmb_ops = {
 },
 };
 
-static int nvme_init(PCIDevice *pci_dev)
+static void nvme_realize(PCIDevice *pci_dev, Error **errp)
 {
 NvmeCtrl *n = NVME(pci_dev);
 NvmeIdCtrl *id = &n->id_ctrl;
@@ -931,24 +931,27 @@ static int nvme_init(PCIDevice *pci_dev)
 Error *local_err = NULL;
 
 if (!n->conf.blk) {
-return -1;
+error_setg(errp, "drive property not set");
+return;
 }
 
 bs_size = blk_getlength(n->conf.blk);
 if (bs_size < 0) {
-return -1;
+error_setg(errp, "could not get backing file size");
+return;
 }
 
 blkconf_serial(&n->conf, &n->serial);
 if (!n->serial) {
-return -1;
+error_setg(errp, "serial property not set");
+return;
 }
 blkconf_blocksizes(&n->conf);
 blkconf_apply_backend_options(&n->conf, blk_is_read_only(n->conf.blk),
   false, &local_err);
 if (local_err) {
-error_report_err(local_err);
-return -1;
+error_propagate(errp, local_err);
+return;
 }
 
 pci_conf = pci_dev->config;
@@ -1046,7 +1049,6 @@ static int nvme_init(PCIDevice *pci_dev)
 cpu_to_le64(n->ns_size >>
 id_ns->lbaf[NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas)].ds);
 }
-return 0;
 }
 
 static void nvme_exit(PCIDevice *pci_dev)
@@ -1081,7 +1083,7 @@ static void nvme_class_init(ObjectClass *oc, void *data)
 DeviceClass *dc = DEVICE_CLASS(oc);
 PCIDeviceClass *pc = PCI_DEVICE_CLASS(oc);
 
-pc->init = nvme_init;
+pc->realize = nvme_realize;
 pc->exit = nvme_exit;
 pc->class_id = PCI_CLASS_STORAGE_EXPRESS;
 pc->vendor_id = PCI_VENDOR_ID_INTEL;
-- 
2.9.4

[Qemu-block] [PATCH 0/6] Convert to realize and improve error handling

[Mao Zhongyi]

This series mainly implements the conversions of ide, floppy and nvme
device to realize. Add some error handling messages and remove the local
variable local_err, use errp to propagate the error directly. Also
fix the unusual function name.

Cc: John Snow 
Cc: Kevin Wolf 
Cc: Max Reitz 
Cc: Keith Busch 
Cc: Stefan Hajnoczi 
Cc: "Michael S. Tsirkin" 
Cc: Paolo Bonzini 
Cc: Gerd Hoffmann 
Cc: Markus Armbruster 

Mao Zhongyi (6):
  hw/ide: Convert DeviceClass init to realize
  hw/block/fdc: Convert to realize
  hw/block/nvme: Convert to realize
  hw/block: Fix the return type
  hw/block: Use errp directly rather than local_err
  dev-storage: Fix the unusual function name

 hw/block/block.c| 21 +
 hw/block/dataplane/virtio-blk.c | 16 ---
 hw/block/dataplane/virtio-blk.h |  6 +--
 hw/block/fdc.c  | 48 +
 hw/block/nvme.c | 24 +--
 hw/block/virtio-blk.c   | 17 +++-
 hw/ide/core.c   |  7 +--
 hw/ide/qdev.c   | 94 +++--
 hw/scsi/scsi-disk.c | 13 ++
 hw/usb/dev-storage.c| 29 ++---
 include/hw/block/block.h| 10 ++---
 include/hw/ide/internal.h   |  5 ++-
 12 files changed, 135 insertions(+), 155 deletions(-)

-- 
2.9.4

[Qemu-block] [PATCH 2/6] hw/block/fdc: Convert to realize

[Mao Zhongyi]

Convert floppy_drive_init() to realize and rename it to
floppy_drive_realize().

Cc: John Snow 
Cc: Kevin Wolf 
Cc: Max Reitz 
Cc: Markus Armbruster 

Signed-off-by: Mao Zhongyi 
---
 hw/block/fdc.c | 33 -
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 4011290..02f4a46 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -517,7 +517,7 @@ static Property floppy_drive_properties[] = {
 DEFINE_PROP_END_OF_LIST(),
 };
 
-static int floppy_drive_init(DeviceState *qdev)
+static void floppy_drive_realize(DeviceState *qdev, Error **errp)
 {
 FloppyDrive *dev = FLOPPY_DRIVE(qdev);
 FloppyBus *bus = FLOPPY_BUS(qdev->parent_bus);
@@ -535,15 +535,15 @@ static int floppy_drive_init(DeviceState *qdev)
 }
 
 if (dev->unit >= MAX_FD) {
-error_report("Can't create floppy unit %d, bus supports only %d units",
- dev->unit, MAX_FD);
-return -1;
+error_setg(errp, "Can't create floppy unit %d, bus supports "
+   "only %d units", dev->unit, MAX_FD);
+return;
 }
 
 drive = get_drv(bus->fdc, dev->unit);
 if (drive->blk) {
-error_report("Floppy unit %d is in use", dev->unit);
-return -1;
+error_setg(errp, "Floppy unit %d is in use", dev->unit);
+return;
 }
 
 if (!dev->conf.blk) {
@@ -557,8 +557,9 @@ static int floppy_drive_init(DeviceState *qdev)
 if (dev->conf.logical_block_size != 512 ||
 dev->conf.physical_block_size != 512)
 {
-error_report("Physical and logical block size must be 512 for floppy");
-return -1;
+error_setg(errp, "Physical and logical block size must "
+   "be 512 for floppy");
+return;
 }
 
 /* rerror/werror aren't supported by fdc and therefore not even registered
@@ -570,20 +571,20 @@ static int floppy_drive_init(DeviceState *qdev)
 blkconf_apply_backend_options(&dev->conf, blk_is_read_only(dev->conf.blk),
   false, &local_err);
 if (local_err) {
-error_report_err(local_err);
-return -1;
+error_propagate(errp, local_err);
+return;
 }
 
 /* 'enospc' is the default for -drive, 'report' is what blk_new() gives us
  * for empty drives. */
 if (blk_get_on_error(dev->conf.blk, 0) != BLOCKDEV_ON_ERROR_ENOSPC &&
 blk_get_on_error(dev->conf.blk, 0) != BLOCKDEV_ON_ERROR_REPORT) {
-error_report("fdc doesn't support drive option werror");
-return -1;
+error_setg(errp, "fdc doesn't support drive option werror");
+return;
 }
 if (blk_get_on_error(dev->conf.blk, 1) != BLOCKDEV_ON_ERROR_REPORT) {
-error_report("fdc doesn't support drive option rerror");
-return -1;
+error_setg(errp, "fdc doesn't support drive option rerror");
+return;
 }
 
 drive->conf = &dev->conf;
@@ -599,14 +600,12 @@ static int floppy_drive_init(DeviceState *qdev)
 dev->type = drive->drive;
 
 fd_revalidate(drive);
-
-return 0;
 }
 
 static void floppy_drive_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *k = DEVICE_CLASS(klass);
-k->init = floppy_drive_init;
+k->realize = floppy_drive_realize;
 set_bit(DEVICE_CATEGORY_STORAGE, k->categories);
 k->bus_type = TYPE_FLOPPY_BUS;
 k->props = floppy_drive_properties;
-- 
2.9.4

[Qemu-block] [PATCH 1/6] hw/ide: Convert DeviceClass init to realize

[Mao Zhongyi]

Replace init with realize in IDEDeviceClass, which has errp
as a parameter. So all the implementations now use error_setg
instead of error_report for reporting error.

Cc: John Snow 
Cc: Markus Armbruster 

Signed-off-by: Mao Zhongyi 
---
 hw/ide/core.c |  7 ++--
 hw/ide/qdev.c | 86 +++
 include/hw/ide/internal.h |  5 +--
 3 files changed, 49 insertions(+), 49 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 0b48b64..7c86fc7 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -34,6 +34,7 @@
 #include "hw/block/block.h"
 #include "sysemu/block-backend.h"
 #include "qemu/cutils.h"
+#include "qemu/error-report.h"
 
 #include "hw/ide/internal.h"
 
@@ -2398,7 +2399,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, 
IDEDriveKind kind,
const char *version, const char *serial, const char *model,
uint64_t wwn,
uint32_t cylinders, uint32_t heads, uint32_t secs,
-   int chs_trans)
+   int chs_trans, Error **errp)
 {
 uint64_t nb_sectors;
 
@@ -2423,11 +2424,11 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, 
IDEDriveKind kind,
 blk_set_guest_block_size(blk, 2048);
 } else {
 if (!blk_is_inserted(s->blk)) {
-error_report("Device needs media, but drive is empty");
+error_setg(errp, "Device needs media, but drive is empty");
 return -1;
 }
 if (blk_is_read_only(blk)) {
-error_report("Can't use a read-only drive");
+error_setg(errp, "Can't use a read-only drive");
 return -1;
 }
 blk_set_dev_ops(blk, &ide_hd_block_ops, s);
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index cc2f5bd..d60ac25 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -80,7 +80,7 @@ static char *idebus_get_fw_dev_path(DeviceState *dev)
 return g_strdup(path);
 }
 
-static int ide_qdev_init(DeviceState *qdev)
+static void ide_qdev_realize(DeviceState *qdev, Error **errp)
 {
 IDEDevice *dev = IDE_DEVICE(qdev);
 IDEDeviceClass *dc = IDE_DEVICE_GET_CLASS(dev);
@@ -91,34 +91,31 @@ static int ide_qdev_init(DeviceState *qdev)
 }
 
 if (dev->unit >= bus->max_units) {
-error_report("Can't create IDE unit %d, bus supports only %d units",
+error_setg(errp, "Can't create IDE unit %d, bus supports only %d 
units",
  dev->unit, bus->max_units);
-goto err;
+return;
 }
 
 switch (dev->unit) {
 case 0:
 if (bus->master) {
-error_report("IDE unit %d is in use", dev->unit);
-goto err;
+error_setg(errp, "IDE unit %d is in use", dev->unit);
+return;
 }
 bus->master = dev;
 break;
 case 1:
 if (bus->slave) {
-error_report("IDE unit %d is in use", dev->unit);
-goto err;
+error_setg(errp, "IDE unit %d is in use", dev->unit);
+return;
 }
 bus->slave = dev;
 break;
 default:
-error_report("Invalid IDE unit %d", dev->unit);
-goto err;
+error_setg(errp, "Invalid IDE unit %d", dev->unit);
+return;
 }
-return dc->init(dev);
-
-err:
-return -1;
+dc->realize(dev, errp);
 }
 
 IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive)
@@ -159,7 +156,7 @@ typedef struct IDEDrive {
 IDEDevice dev;
 } IDEDrive;
 
-static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
+static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
 {
 IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
 IDEState *s = bus->ifs + dev->unit;
@@ -168,8 +165,8 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
 
 if (!dev->conf.blk) {
 if (kind != IDE_CD) {
-error_report("No drive specified");
-return -1;
+error_setg(errp, "No drive specified");
+return;
 } else {
 /* Anonymous BlockBackend for an empty drive */
 dev->conf.blk = blk_new(0, BLK_PERM_ALL);
@@ -182,36 +179,36 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind 
kind)
 dev->conf.discard_granularity = 512;
 } else if (dev->conf.discard_granularity &&
dev->conf.discard_granularity != 512) {
-error_report("discard_granularity must be 512 for ide");
-return -1;
+error_setg(errp, "discard_granularity must be 512 for ide");
+return;
 }
 
 blkconf_blocksizes(&dev->conf);
 if (dev->conf.logical_block_size != 512) {
-error_report("logical_block_size must be 512 for IDE");
-return -1;
+error_setg(errp, "logical_block_size must be 512 for IDE");
+return;
 }
 
 blkconf_serial(&dev->conf, &dev->serial);
 if (kind != IDE_CD) {
 blkconf_geometry(&dev->conf, &dev->chs_trans, 65535, 

[Qemu-block] [PATCH 6/6] dev-storage: Fix the unusual function name

[Mao Zhongyi]

The function name of usb_msd_{realize,unrealize}_*,
usb_msd_class_initfn_* are unusual. Rename it to
usb_msd_*_{realize,unrealize}, usb_msd_class_*_initfn.

Cc: Gerd Hoffmann 

Signed-off-by: Mao Zhongyi 
---
 hw/usb/dev-storage.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index bd88d6e..599c536 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -596,7 +596,7 @@ static void usb_msd_unrealize_storage(USBDevice *dev, Error 
**errp)
 object_unref(OBJECT(&s->bus));
 }
 
-static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
+static void usb_msd_storage_realize(USBDevice *dev, Error **errp)
 {
 MSDState *s = USB_STORAGE_DEV(dev);
 BlockBackend *blk = s->conf.blk;
@@ -643,14 +643,14 @@ static void usb_msd_realize_storage(USBDevice *dev, Error 
**errp)
 s->scsi_dev = scsi_dev;
 }
 
-static void usb_msd_unrealize_bot(USBDevice *dev, Error **errp)
+static void usb_msd_bot_unrealize(USBDevice *dev, Error **errp)
 {
 MSDState *s = USB_STORAGE_DEV(dev);
 
 object_unref(OBJECT(&s->bus));
 }
 
-static void usb_msd_realize_bot(USBDevice *dev, Error **errp)
+static void usb_msd_bot_realize(USBDevice *dev, Error **errp)
 {
 MSDState *s = USB_STORAGE_DEV(dev);
 DeviceState *d = DEVICE(dev);
@@ -764,12 +764,12 @@ static void usb_msd_class_initfn_common(ObjectClass 
*klass, void *data)
 dc->vmsd = &vmstate_usb_msd;
 }
 
-static void usb_msd_class_initfn_storage(ObjectClass *klass, void *data)
+static void usb_msd_class_storage_initfn(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
 USBDeviceClass *uc = USB_DEVICE_CLASS(klass);
 
-uc->realize = usb_msd_realize_storage;
+uc->realize = usb_msd_storage_realize;
 uc->unrealize = usb_msd_unrealize_storage;
 dc->props = msd_properties;
 }
@@ -828,26 +828,26 @@ static void usb_msd_instance_init(Object *obj)
 object_property_set_int(obj, -1, "bootindex", NULL);
 }
 
-static void usb_msd_class_initfn_bot(ObjectClass *klass, void *data)
+static void usb_msd_class_bot_initfn(ObjectClass *klass, void *data)
 {
 USBDeviceClass *uc = USB_DEVICE_CLASS(klass);
 
-uc->realize = usb_msd_realize_bot;
-uc->unrealize = usb_msd_unrealize_bot;
+uc->realize = usb_msd_bot_realize;
+uc->unrealize = usb_msd_bot_unrealize;
 uc->attached_settable = true;
 }
 
 static const TypeInfo msd_info = {
 .name  = "usb-storage",
 .parent= TYPE_USB_STORAGE,
-.class_init= usb_msd_class_initfn_storage,
+.class_init= usb_msd_class_storage_initfn,
 .instance_init = usb_msd_instance_init,
 };
 
 static const TypeInfo bot_info = {
 .name  = "usb-bot",
 .parent= TYPE_USB_STORAGE,
-.class_init= usb_msd_class_initfn_bot,
+.class_init= usb_msd_class_bot_initfn,
 };
 
 static void usb_msd_register_types(void)
-- 
2.9.4

Re: [Qemu-block] [Qemu-devel] [PATCH] qemu-iotests: add a "how to" to ./README

[Peter Maydell]

On 26 July 2017 at 12:33, Stefan Hajnoczi  wrote:
> On Tue, Jul 25, 2017 at 04:34:19PM +0100, Peter Maydell wrote:
>> On 25 July 2017 at 16:20, Stefan Hajnoczi  wrote:
>> > On Mon, Jul 24, 2017 at 11:20:44AM +0100, Peter Maydell wrote:
>> >> Should ./check be run from the source tree, or the build tree? The
>> >> existing README text doesn't say and I don't think your additions
>> >> do either.
>> >
>> > It doesn't matter, both should work.  The script detects both
>> > possibilities and rejigs itself to compensate.
>>
>> Does that mean
>>  if you run it from the source tree in a tree configured
>>  for out of tree build it will:
>>  (a) pollute your source tree with test output and binaries
>>  (b) give you a helpful message about what to do
>>  (c) magically find the build tree somehow
>>  (d) not need to write any binaries/test output/temp files at all
>>
>> ?
>
> I think it would fail since the binaries would be missing in the source
> tree.

That sounds like "the README should say you need to run it
from the build tree", then ?

thanks
-- PMM

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

[Stefan Hajnoczi]

On Tue, Jul 25, 2017 at 07:21:45PM +0300, Manos Pitsidianakis wrote:
> On Tue, Jul 25, 2017 at 05:09:41PM +0100, Stefan Hajnoczi wrote:
> > On Tue, Jul 25, 2017 at 01:29:08PM +0300, Manos Pitsidianakis wrote:
> > > On Mon, Jul 24, 2017 at 04:12:47PM +0100, Stefan Hajnoczi wrote:
> > > > On Fri, Jul 14, 2017 at 12:45:18PM +0300, Manos Pitsidianakis wrote:
> > > > > ThrottleGroup is converted to an object. This will allow the future
> > > > > throttle block filter drive easy creation and configuration of 
> > > > > throttle
> > > > > groups in QMP and cli.
> > > > >
> > > > > A new QAPI struct, ThrottleLimits, is introduced to provide a shared
> > > > > struct for all throttle configuration needs in QMP.
> > > > >
> > > > > ThrottleGroups can be created via CLI as
> > > > > -object throttling-group,id=foo,x-iops-total=100,x-..
> > > >
> > > > Please make the QOM name and struct name consistent.  Either
> > > > ThrottleGroup/throttle-group or ThrottlingGroup/throttling-group but not
> > > > ThrottleGroup/throttling-group.
> > > 
> > > I did this on purpose because current throttling has ThrottleGroup
> > > internally and throttling.group in the command line. Should we keep this
> > > only in legacy and make it throttle-group everywhere?
> > 
> > I don't see a compatibility issue because -drive throttling.group= is a
> > -drive property while THROTTLE_GROUP ("throttling-group") is a QOM class
> > name.  They are unrelated and the QOM convention is for the
> > typedef/struct name (ThrottleGroup) to be consistent with the QOM class
> > name.
> > 
> > Therefore it should be safe to use "throttle-group" as the QOM class
> > name instead of "throttling-group".
> 
> I meant for consistency not compatibility. Otherwise it probably would be
> better to keep throttle-group/ThrottleGroup in the new interfaces.

You could call it ThrottlingGroup ("throttling-group") for consistency.

> > 
> > > > > +visit_type_int64(v, name, &value, &local_err);
> > > > > +if (local_err) {
> > > > > +goto fail;
> > > > > +}
> > > > > +if (value < 0) {
> > > > > +error_setg(&local_err, "Property value must be in range "
> > > > > +   "[0, %"PRId64"]", INT64_MAX);
> > > >
> > > > Please print the maximum value relevant to the actual field type instead
> > > > of INT64_MAX.
> > > 
> > > This checks the limits of the int64 field you give to QOM. I think you 
> > > mean
> > > in the value assignment to each field that follows? In any case, since
> > > unsigned is the only smaller field we could convert it to 
> > > uint64_t/uint32_t
> > > internally.
> > 
> > I'm saying that UNSIGNED fields are silently truncated if the value is
> > larger than UINT_MAX, and also that the error message is misleading
> > since UNSIGNED fields cannot take values in the whole range we print.
> 
> Yes, wouldn't it be better to convert the unsigned field burst_length to
> uint64_t and take care of the overflow case? The field describes seconds,
> but there's no reason to keep it that small.

A burst_length of UINT_MAX seconds is over 136 years.  I guess unsigned
int was chosen because larger values will never be used :).

That said, making burst_length uint64_t is fine from a compatibility
point of view.


signature.asc
Description: PGP signature

Re: [Qemu-block] [Qemu-devel] RFC: unified --help information

[Peter Maydell]

On 26 July 2017 at 12:22, Eric Blake  wrote:
> So something like these two lines (via a macro) in all of the
> executables, ideally at the tail end of --help output?
>
> See  for bug reports.
> More information on the qemu project at 

Text looks good to me. A minor nit: better not use https://
URLs til we fix the website (which currently makes firefox
warn about an invalid security certificate...)

Would be good to put this in GUI about dialog boxes
and the like too.

thanks
-- PMM

Re: [Qemu-block] [Qemu-devel] [PATCH] qemu-iotests: add a "how to" to ./README

[Stefan Hajnoczi]

On Tue, Jul 25, 2017 at 04:34:19PM +0100, Peter Maydell wrote:
> On 25 July 2017 at 16:20, Stefan Hajnoczi  wrote:
> > On Mon, Jul 24, 2017 at 11:20:44AM +0100, Peter Maydell wrote:
> >> Should ./check be run from the source tree, or the build tree? The
> >> existing README text doesn't say and I don't think your additions
> >> do either.
> >
> > It doesn't matter, both should work.  The script detects both
> > possibilities and rejigs itself to compensate.
> 
> Does that mean
>  if you run it from the source tree in a tree configured
>  for out of tree build it will:
>  (a) pollute your source tree with test output and binaries
>  (b) give you a helpful message about what to do
>  (c) magically find the build tree somehow
>  (d) not need to write any binaries/test output/temp files at all
> 
> ?

I think it would fail since the binaries would be missing in the source
tree.

./check handles either source or out-of-tree mode:

if [ -L "$0" ]
then
# called from the build tree
source_iotests=$(dirname "$(readlink "$0")")
if [ -z "$source_iotests" ]
then
_init_error "failed to obtain source tree name from check symlink"
fi
source_iotests=$(cd "$source_iotests"; pwd) || _init_error "failed to enter 
source tree"
build_iotests=$PWD
else
# called from the source tree
source_iotests=$PWD
# this may be an in-tree build (note that in the following code we may not
# assume that it truly is and have to test whether the build results
# actually exist)
build_iotests=$PWD
fi

build_root="$build_iotests/../.."


signature.asc
Description: PGP signature

Re: [Qemu-block] RFC: unified --help information

[Eric Blake]

On 07/26/2017 06:11 AM, Paolo Bonzini wrote:

>>
>> qemu-nbd mentions: Report bugs to 
>> qemu-ga mentions: Report bugs to 
>>
>> while qemu-system-*, qemu-img, and qemu-io mention nothing at all.  Is
>> it worth unifying these to all mention qemu-de...@nongnu.org as the
>> primary bug-report site, and/or to add mention of  as a
>> site for further information?
> 
> I think we should point to a more specific bugreport URL.
> 
> Currently it's http://wiki.qemu.org/Contribute/ReportABug.  If we want
> an URL on qemu.org, we can either add a redirect from qemu.org to the
> wiki, or move the contents to qemu.org (it's static enough that it
> shouldn't be a big issue).  I've sent out a patch for the latter.

So something like these two lines (via a macro) in all of the
executables, ideally at the tail end of --help output?

See  for bug reports.
More information on the qemu project at 

(assuming your recent patch goes in).  I'm also open to bikeshedding on
better wording, or requests on which content is most useful vs. too verbose.

-- 
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: unified --help information

[Paolo Bonzini]

On 26/07/2017 12:53, Eric Blake wrote:
> 
> GNU coreutils online help: 
> Full documentation at: 
> or available locally via: info '(coreutils) ls invocation'
> 
> or 'm4 --help | tail -n3':
> Report bugs to: bug...@gnu.org
> GNU M4 home page: 
> General help using GNU software: 
> 
> In our executables, we are rather inconsistent with --help
> 
> qemu-nbd mentions: Report bugs to 
> qemu-ga mentions: Report bugs to 
> 
> while qemu-system-*, qemu-img, and qemu-io mention nothing at all.  Is
> it worth unifying these to all mention qemu-de...@nongnu.org as the
> primary bug-report site, and/or to add mention of  as a
> site for further information?

I think we should point to a more specific bugreport URL.

Currently it's http://wiki.qemu.org/Contribute/ReportABug.  If we want
an URL on qemu.org, we can either add a redirect from qemu.org to the
wiki, or move the contents to qemu.org (it's static enough that it
shouldn't be a big issue).  I've sent out a patch for the latter.

> Likewise, with --version, qemu-io and qemu-ga lack the git suffix
> present in qemu-system-* and just added in qemu-nbd.
> 
> I can submit patches to make things consistent, and think it is 2.10
> material, but first want to get consensus on what the behavior should be.

I agree it's better to be consistent.

Paolo



signature.asc
Description: OpenPGP digital signature

[Qemu-block] RFC: unified --help information

[Eric Blake]

I noticed that some utilities provide a handy link to the website (which
in turn connects the users to bug database, mailing list, online docs,
...); for example, look at 'ls --help | tail -n3':

GNU coreutils online help: 
Full documentation at: 
or available locally via: info '(coreutils) ls invocation'

or 'm4 --help | tail -n3':
Report bugs to: bug...@gnu.org
GNU M4 home page: 
General help using GNU software: 

In our executables, we are rather inconsistent with --help

qemu-nbd mentions: Report bugs to 
qemu-ga mentions: Report bugs to 

while qemu-system-*, qemu-img, and qemu-io mention nothing at all.  Is
it worth unifying these to all mention qemu-de...@nongnu.org as the
primary bug-report site, and/or to add mention of  as a
site for further information?

Likewise, with --version, qemu-io and qemu-ga lack the git suffix
present in qemu-system-* and just added in qemu-nbd.

I can submit patches to make things consistent, and think it is 2.10
material, but first want to get consensus on what the behavior should be.

-- 
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 3/3] qemu-iotests: require CONFIG_LINUX_AIO for test 087

[Daniel P. Berrange]

On Wed, Jul 26, 2017 at 04:55:39PM +0800, Jing Liu wrote:
> 
> 
> On 2017/7/25 下午11:48, Daniel P. Berrange wrote:
> > On Tue, Jul 25, 2017 at 04:45:46PM +0100, Stefan Hajnoczi wrote:
> > > On Mon, Jul 24, 2017 at 02:44:13PM +0800, Jing Liu wrote:
> > > > On 2017/7/21 上午11:47, Cleber Rosa wrote:
> > > > > One of the "sub-"tests of test 087 requires CONFIG_LINUX_AIO.
> > > > > 
> > > > > As a PoC/RFC, this goes the easy route and skips the test as a whole
> > > > > when that feature is missing.  Other approaches include splitting
> > > > > the test and adding extra filtering.
> > > > > 
> > > > > Signed-off-by: Cleber Rosa 
> > > > > ---
> > > > >tests/qemu-iotests/087 | 1 +
> > > > >1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
> > > > > index f8e4903..a2fb7de 100755
> > > > > --- a/tests/qemu-iotests/087
> > > > > +++ b/tests/qemu-iotests/087
> > > > > @@ -34,6 +34,7 @@ status=1# failure is the default!
> > > > >_supported_fmt qcow2
> > > > >_supported_proto file
> > > > >_supported_os Linux
> > > > > +_require_feature CONFIG_LINUX_AIO
> > > > I tested that CONFIG_NETTLE_KDF is also a necessary for 087.
> > > > 
> > > > +_require_feature CONFIG_NETTLE_KDF
> > > Are you sure?  Looks like either nettle or gcrypt is needed:
> > Correct, it works with either.
> Ah, because I just found out nettle which related to KDF.
> why can not find out gcrypt.h in qemu?
> How to compile config_gcrypt_kdf into qemu?

QEMU picks either nettle or libgcrypt automatically, dependant on which
of these gnutls is linked to.

You can force override this via  --disable-nettle --enable-gcrypt
(or vica-verca), for sake of testing, but you shouldn't do that
for production builds.


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 V5 10/10] block/qcow2: add compress info to image specific info

[Peter Lieven]

Am 25.07.2017 um 23:55 schrieb Eric Blake:
> On 07/25/2017 09:41 AM, Peter Lieven wrote:
>> Signed-off-by: Peter Lieven 
>> ---
>>  block/qcow2.c| 7 +++
>>  qapi/block-core.json | 6 +-
>>  2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> +++ b/qapi/block-core.json
>> @@ -68,6 +68,9 @@
>>  # @encrypt: details about encryption parameters; only set if image
>>  #   is encrypted (since 2.10)
>>  #
>> +# @compress: details about parameters for compressed clusters; only set if
>> +#the compress format header extension is present (since 2.11)
> Thinking aloud:
>
> I still think this is better as "only set if the image uses compressed
> headers" (similar to encrypt).  That is, I would like this output to be
> present even when opening legacy deflate/0/12 images.
>
> But thinking more, what I am REALLY asking for is "if any cluster is
> compressed, then this information is present; if nothing is compressed,
> the choice of compression header doesn't matter".  But we don't have a
> quick-and-dirty way to tell if an image has any compressed clusters,
> short of examining every L2 map (including maps inside internal snapshots).
>
> Hmm - we already have 'Dirty bit' and 'Corrupt bit' as
> incompatible_features that can quickly identify certain image
> properties; do we want another bit set to 1 for images known to use
> compressed clusters?  Or even make it an auto-clear bit (or even a pair
> of auto-clear bits: one to designate that this image was written by a
> new-enough qemu that promises to mark the image if any compressed
> clusters exist, and the other is set only if such clusters do exist;
> that way, if both bits are clear, we know we have to walk L2 tables to
> look for compressed clusters, but if the first bit is set, then the
> second bit is reliable)?
>
> So maybe this means we are still debating spec ideas.  Kevin, do you
> want to chime in?
>

I would also vote for adding an information to the image if it contains
compressed clusters. 2 bits sounds reasonable. One to promise we set
the compressed bit if we write any compressed cluster and one to tell
that there are actually compressed cluster. If we set the "promise" bit
at create time we can simply hook up in qcow2_co_pwritev_comperssed
and set the bit there as soon as we write any compressed cluster.

The compression info I would display unconditionally. Because the compress
settings are made at create time and are there even if we don't have compressed
clusters yet.

Peter

Re: [Qemu-block] [PATCH 3/3] qemu-iotests: require CONFIG_LINUX_AIO for test 087

[Jing Liu]

On 2017/7/25 下午11:48, Daniel P. Berrange wrote:

On Tue, Jul 25, 2017 at 04:45:46PM +0100, Stefan Hajnoczi wrote:

On Mon, Jul 24, 2017 at 02:44:13PM +0800, Jing Liu wrote:

On 2017/7/21 上午11:47, Cleber Rosa wrote:

One of the "sub-"tests of test 087 requires CONFIG_LINUX_AIO.

As a PoC/RFC, this goes the easy route and skips the test as a whole
when that feature is missing.  Other approaches include splitting
the test and adding extra filtering.

Signed-off-by: Cleber Rosa 
---
   tests/qemu-iotests/087 | 1 +
   1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index f8e4903..a2fb7de 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -34,6 +34,7 @@ status=1  # failure is the default!
   _supported_fmt qcow2
   _supported_proto file
   _supported_os Linux
+_require_feature CONFIG_LINUX_AIO

I tested that CONFIG_NETTLE_KDF is also a necessary for 087.

+_require_feature CONFIG_NETTLE_KDF

Are you sure?  Looks like either nettle or gcrypt is needed:

Correct, it works with either.

Ah, because I just found out nettle which related to KDF.
why can not find out gcrypt.h in qemu?
How to compile config_gcrypt_kdf into qemu?



crypto/Makefile.objs:crypto-obj-$(CONFIG_NETTLE_KDF) += pbkdf-nettle.o
crypto/Makefile.objs:crypto-obj-$(if 
$(CONFIG_NETTLE_KDF),n,$(CONFIG_GCRYPT_KDF)) += pbkdf-gcrypt.o

But this shows why the compile-time testing of features is ugly:

1. It duplicates build dependency logic into the test cases.

2. The test cases don't care about nettle vs gcrypt and they shouldn't
have to know about it.  They just care whether LUKS is available or
not.

Yeah that knowledge is messy and fragile wrt future changes.


Regards,
Daniel