Re: [Qemu-block] [Qemu-devel] [PATCH v3 06/15] monitor: Rename HMP command type and tables

[Markus Armbruster]

Kevin Wolf  writes:

> This renames the type for HMP monitor commands and the tables holding
> the commands to make clear that they are related to HMP and to allow
> making them public later:
>
> * mon_cmd_t -> HMPCommand (fixing use of a reserved name, too)
> * mon_cmds -> hmp_cmds
> * info_cmds -> hmp_info_cmds
>
> Signed-off-by: Kevin Wolf 

Reviewed-by: Markus Armbruster 

Re: [Qemu-block] [Qemu-devel] [PATCH v3 05/15] monitor: Remove Monitor.cmd_table indirection

[Markus Armbruster]

Kevin Wolf  writes:

> Monitor.cmd_table is initialised to point to mon_cmds and never changed
> afterwards. We can remove the indirection and just reference mon_cmds
> directly instead.
>
> Signed-off-by: Kevin Wolf 

Reviewed-by: Markus Armbruster 

Re: [Qemu-block] [QEMU] [PATCH v2 0/8] Add Qemu to SeaBIOS LCHS interface

[Gerd Hoffmann]

  Hi,

> Can there be a guest that will fail the MBR in such a way? Yes.
> Look at the following MBR partition table of a Windows XP guest in our 
> production
> environment:
> 
> Disk size in sectors: 16777216
> 
> Binary (only one partition 16 bytes): 80 01 01 00 07 fe ff ff 3f 00 00 00 d5 
> ea ff 00
> Start: (0, 1, 1, 63)
> End: (1023, 254, 63, 16771859)
> 
> As can be easily seen, any MBR guessing algorithm should guess:
> 
>   255 heads (since a value of 254 appears), 63 spt (since a value of 63 
> appears)
> 
> Turns out that this image does not work with 255, 63 but actually requires
> 
>   16 heads, 63 spt
> 
> to boot.
> 
> So relying on MBR partitions alone is not always enough and sometimes manual 
> intervention
> is required.

Ok, given that seabios has no setup any manual configuration needs to be done 
via qemu.

But why do we need a new interface for that?  IDE can pass the geometry
to the guest.  virtio-blk has support too (VIRTIO_BLK_F_GEOMETRY).
Likewise scsi (MODE_PAGE_HD_GEOMETRY).  So this should be doable without
any qemu changes.

cheers,
  Gerd

Re: [Qemu-block] [Qemu-devel] testsuite spurious failure due to leftover files

[Markus Armbruster]

Eric Blake  writes:

> iotests ./check -qcow2 failed for me today:
>
> 191  fail   [10:46:04] [10:46:13]   0s (last: 7s)output
> mismatch (see 191.out.bad)
> --- /home/eblake/qemu/tests/qemu-iotests/191.out  2019-05-24
> 09:27:46.449348997 -0500
> +++ /home/eblake/qemu/tests/qemu-iotests/191.out.bad  2019-06-13
> 10:46:13.906004319 -0500
> @@ -8,6 +8,8 @@
>  Formatting 'TEST_DIR/t.IMGFMT.ovl2', fmt=IMGFMT size=67108864
> backing_file=TEST_DIR/t.IMGFMT.mid
>  wrote 65536/65536 bytes at offset 1048576
>  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +mkfifo: cannot create fifo
> '/home/eblake/qemu/tests/qemu-iotests/scratch/qmp-out-17782_0': File exists
> +mkfifo: cannot create fifo
> '/home/eblake/qemu/tests/qemu-iotests/scratch/qmp-in-17782_0': File exists
>  {
>  "return": {
>  }
>
> If someone wants a good project to take on - it would be nice to revive
> Jeff Cody's initial work on refactoring the iotests to operate with one
> directory per test, allowing things to be run in parallel, and allowing
> cleanup to be much easier (nuke the temp directories, rather than trying
> to track down which test left a possibly-colliding file name behind)

The non-iotests could use the same treatment.

Too big for , I'm afraid.

Re: [Qemu-block] [Qemu-devel] [PATCH] iotests: Hide timestamps for skipped tests

[Philippe Mathieu-Daudé]

On 6/13/19 8:37 PM, Max Reitz wrote:
> Currently, the "thistime" variable is not reinitialized on every loop
> iteration.  This leads to tests that do not yield a run time (because
> they failed or were skipped) printing the run time of the previous test
> that did.  Fix that by reinitializing "thistime" for every test.
> 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/check | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index 44ebf24080..f925606cc5 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -773,6 +773,7 @@ do
>  printdiff=false # show diff to reference output?
>  status=""   # test result summary
>  results=""  # test result details
> +thistime="" # time the test took
>  
>  if [ -n "$TESTS_REMAINING_LOG" ] ; then
>  sed -e "s/$seq//" -e 's/  / /' -e 's/^ *//' $TESTS_REMAINING_LOG > 
> $TESTS_REMAINING_LOG.tmp
> 

Reviewed-by: Philippe Mathieu-Daudé 

Re: [Qemu-block] [PATCH v9] ssh: switch from libssh2 to libssh

[Eric Blake]

On 6/13/19 2:31 PM, Max Reitz wrote:

>> @@ -657,71 +644,147 @@ static int connect_to_ssh(BDRVSSHState *s, 
>> BlockdevOptionsSsh *opts,
> 
> [...]
> 
>> +/*
>> + * Try to disable the Nagle algorithm on TCP sockets to reduce latency,
>> + * but do not fail if it cannot be disabled.
>> + */
>> +r = socket_set_nodelay(new_sock);
>> +if (r < 0) {
>> +warn_report("setting NODELAY for the ssh server %s failed: %m",
> 
> TIL about %m.

printf("%m") is not portable to non-glibc. Spell it out as
"%s",strerror(errno).  (It _is_ portable for syslog(), but warn_report()
is not wired to syslog() semantics).

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH] iotests: Hide timestamps for skipped tests

[Eric Blake]

On 6/13/19 1:37 PM, Max Reitz wrote:
> Currently, the "thistime" variable is not reinitialized on every loop
> iteration.  This leads to tests that do not yield a run time (because
> they failed or were skipped) printing the run time of the previous test
> that did.  Fix that by reinitializing "thistime" for every test.
> 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/check | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Eric Blake 

> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index 44ebf24080..f925606cc5 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -773,6 +773,7 @@ do
>  printdiff=false # show diff to reference output?
>  status=""   # test result summary
>  results=""  # test result details
> +thistime="" # time the test took
>  
>  if [ -n "$TESTS_REMAINING_LOG" ] ; then
>  sed -e "s/$seq//" -e 's/  / /' -e 's/^ *//' $TESTS_REMAINING_LOG > 
> $TESTS_REMAINING_LOG.tmp
> 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v4 0/7] qemu-img: Add salvaging mode to convert

[Max Reitz]

On 07.05.19 22:35, Max Reitz wrote:
> Hi,
> 
> This series adds a --salvage option to qemu-img convert.  With this,
> qemu-img will not abort when it encounters an I/O error.  Instead, it
> tries to narrow it down and will treat the affected sectors as being
> completely 0 (and print a warning).
> 
> Testing this is not so easy, because while real I/O errors during read
> operations should be treated as described above, errors encountered
> during bdrv_block_status() should just be ignored and the affected
> sectors should be considered allocated.  But blkdebug does not yet have
> a way to intercept this, and:
> 
> (1) Just adding a new block-status event would be silly, because I don't
> want an event, I want it to fail on a certain kind of operation, on
> a certain sector range, independently of any events, so why can't we
> just do that?  See patch 4.
> 
> (2) If we just make blkdebug intercept .bdrv_co_block_status() like all
> other kinds of operations, at least iotest 041 fails, which does
> exactly that silly thing: It uses the read_aio event to wait for any
> read.  But it turns out that there may be a bdrv_*block_status()
> call in between, so suddenly the wrong operation yields an error.
> As I said, the real fault here is that it does not really make sense
> to pray that the operation you want to fail is the one that is
> immediately executed after some event that you hope will trigger
> that operation.
> See patch 3.
> 
> So patch 3 allows blkdebug users to select which kind of I/O operation
> they actually want to make fail, and patch 4 allows them to not use any
> event, but to have a rule active all the time.
> 
> Together, we can then enable error injection for block-status in patch 5
> and make use of event=none iotype=block-status in patch 6.

Applied the series to my block branch, and fixed _filter_offsets in
patch 6 as suggested by Vladimir.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] qemu-img: Fix options leakage in img_rebase()

[Max Reitz]

On 28.05.19 21:53, Max Reitz wrote:
> img_rebase() can leak a QDict in two occasions.  Fix it.
> 
> Coverity: CID 1401416
> Fixes: d16699b64671466b42079c45b89127aeea1ca565
> Fixes: 330c72957196e0ae382abcaa97ebf4eb9bc8574f
> Signed-off-by: Max Reitz 
> ---
>  qemu-img.c | 3 +++
>  1 file changed, 3 insertions(+)

Applied to my block branch.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 0/8] block: Ignore loosening perm restrictions failures

[Max Reitz]

Ping

just the final three patches left to review


On 22.05.19 19:03, Max Reitz wrote:
> Hi,
> 
> This series is mainly a fix for
> https://bugzilla.redhat.com/show_bug.cgi?id=1703793.  The problem
> described there is that mirroring to a gluster volume, then switching
> off the volume makes qemu crash.  There are two problems here:
> 
> (1) file-posix reopens the FD all the time because it thinks the FD it
> has is RDONLY.  It actually isn’t after the first reopen, we just
> forgot to change the internal flags.  That’s what patch 1 is for.
> 
> (2) Even then, when mirror completes, it drops its write permission on
> the FD.  This requires a reopen, which will fail if the volume is
> down.  Mirror doesn’t expect that.  Nobody ever expects that
> dropping permissions can fail, and rightfully so because that’s what
> I think we have generally agreed on.
> Therefore, the block layer should hide this error.  This is what the
> last two patches are for.
> 
> The penultimate patch adds two assertions: bdrv_replace_child() (for the
> old BDS) and bdrv_inactivate_recurse() assume they only ever drop
> assertions.  This is now substantiated by these new assertions.
> It turns out that this assumption was just plain wrong.  Patches 3 to 5
> make it right.
> 
> 
> v3:
> - Received no reply to my “Hm, warnings break 'make check', so maybe we
>   should just keep quiet if loosening restrictions fails?” question, so
>   I assume silence means agreement.  Changed patch 7 accordingly.
> 
> - Added a test: The fact how make check kind-of-but-not-really broke
>   showed a nice reproducer: Launching qemu with some file, then deleting
>   that file, then quitting qemu.
> 
> - Rebase “conflict” in patch 6: The forward declaration of
>   bdrv_get_cumulative_perm() is already in qemu thanks to commit
>   481e0eeef4f.
> 
> 
> git-backport-diff against v2:
> 
> Key:
> [] : patches are identical
> [] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, 
> respectively
> 
> 001/8:[] [--] 'file-posix: Update open_flags in raw_set_perm()'
> 002/8:[] [--] 'block: Add bdrv_child_refresh_perms()'
> 003/8:[] [--] 'block/mirror: Fix child permissions'
> 004/8:[] [--] 'block/commit: Drop bdrv_child_try_set_perm()'
> 005/8:[0018] [FC] 'block: Fix order in bdrv_replace_child()'
> Again confuses my v2 patch with 8aecf1d1bd250a, should be:
>   [] : patches are identical
> 006/8:[0002] [FC] 'block: Add *tighten_restrictions to *check*_perm()'
> 007/8:[0018] [FC] 'block: Ignore loosening perm restrictions failures'
> 008/8:[down] 'iotests: Test failure to loosen restrictions'
> 
> 
> Max Reitz (8):
>   file-posix: Update open_flags in raw_set_perm()
>   block: Add bdrv_child_refresh_perms()
>   block/mirror: Fix child permissions
>   block/commit: Drop bdrv_child_try_set_perm()
>   block: Fix order in bdrv_replace_child()
>   block: Add *tighten_restrictions to *check*_perm()
>   block: Ignore loosening perm restrictions failures
>   iotests: Test failure to loosen restrictions
> 
>  include/block/block_int.h  |  15 
>  block.c| 153 +++--
>  block/commit.c |   2 -
>  block/file-posix.c |   4 +
>  block/mirror.c |  32 +---
>  tests/qemu-iotests/182 |  21 +
>  tests/qemu-iotests/182.out |   6 ++
>  7 files changed, 198 insertions(+), 35 deletions(-)
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 0/2] blockdev: Overlays are not snapshots

[Max Reitz]

On 03.06.19 22:22, Max Reitz wrote:
> QEMU’s always been confused over what a snapshot is: Is it the overlay?
> Is it the backing image?
> 
> Confusion is rarely a good thing.  I can’t think of any objective reason
> why the overlay would be a snapshot.  A snapshot is something that does
> not change over time; the overlay does.
> 
> (I suppose historically the reason is that “Taking an overlay” makes no
> sense, so the operations are called “Taking a snapshot”.  Somehow, this
> meaning carried over to the new file that is created during that
> operation; if “Creating a snapshot” creates a file, that file must be
> the snapshot, right?  Well, no, it isn’t.)
> 
> Let’s fix this as best as we can.  Better Nate than lever.

Applied to my block branch.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v6] ssh: switch from libssh2 to libssh

[Stefan Weil]

On 12.06.19 15:27, Philippe Mathieu-Daudé wrote:
> Cc'ing Alex (Docker, Travis) and Stefan (MinGW)
[...]
> Note, libssh is not available on MinGW.

Nor is it available for Mingw64:

https://cygwin.com/cgi-bin2/package-grep.cgi?grep=mingw64-x86_64-libssh=x86_64

That makes building for Windows more difficult because there is an
additional dependency which must be built from source.

Regards
Stefan

Re: [Qemu-block] [PATCH v9] ssh: switch from libssh2 to libssh

[Max Reitz]

On 13.06.19 15:20, Pino Toscano wrote:
> Rewrite the implementation of the ssh block driver to use libssh instead
> of libssh2.  The libssh library has various advantages over libssh2:
> - easier API for authentication (for example for using ssh-agent)
> - easier API for known_hosts handling
> - supports newer types of keys in known_hosts
> 
> Use APIs/features available in libssh 0.8 conditionally, to support
> older versions (which are not recommended though).
> 
> Adjust the tests according to the different error message, and to the
> newer host keys (ed25519) that are used by default with OpenSSH >= 6.7
> and libssh >= 0.7.0.
> 
> Adjust the various Docker/Travis scripts to use libssh when available
> instead of libssh2. The mingw/mxe testing is dropped for now, as there
> are no packages for it.
> 
> Signed-off-by: Pino Toscano 
> Tested-by: Philippe Mathieu-Daudé 
> Acked-by: Alex Bennée 
> ---

Can confirm that this runs much faster than the last version I tested.
197 and 215 are still whacky (like 100 s instead of 50), but that’s fine
with me. :-)

> Changes from v8:
> - use a newer key type in iotest 207
> - improve the commit message
> 
> Changes from v7:
> - #if HAVE_LIBSSH_0_8 -> #ifdef HAVE_LIBSSH_0_8
> - ptrdiff_t -> size_t
> 
> Changes from v6:
> - fixed few checkpatch style issues
> - detect libssh 0.8 via symbol detection
> - adjust travis/docker test material
> - remove dead "default" case in a switch
> - use variables for storing MIN() results
> - adapt a documentation bit
> 
> Changes from v5:
> - adapt to newer tracing APIs
> - disable ssh compression (mimic what libssh2 does by default)
> - use build time checks for libssh 0.8, and use newer APIs directly
> 
> Changes from v4:
> - fix wrong usages of error_setg/session_error_setg/sftp_error_setg
> - fix few return code checks
> - remove now-unused parameters in few internal functions
> - allow authentication with "none" method
> - switch to unsigned int for the port number
> - enable TCP_NODELAY on the socket
> - fix one reference error message in iotest 207
> 
> Changes from v3:
> - fix socket cleanup in connect_to_ssh()
> - add comments about the socket cleanup
> - improve the error reporting (closer to what was with libssh2)
> - improve EOF detection on sftp_read()
> 
> Changes from v2:
> - used again an own fd
> - fixed co_yield() implementation
> 
> Changes from v1:
> - fixed jumbo packets writing
> - fixed missing 'err' assignment
> - fixed commit message
> 
>  .travis.yml   |   4 +-
>  block/Makefile.objs   |   6 +-
>  block/ssh.c   | 622 +-
>  block/trace-events|  14 +-
>  configure |  65 +-
>  docs/qemu-block-drivers.texi  |   2 +-
>  .../dockerfiles/debian-win32-cross.docker |   1 -
>  .../dockerfiles/debian-win64-cross.docker |   1 -
>  tests/docker/dockerfiles/fedora.docker|   4 +-
>  tests/docker/dockerfiles/ubuntu.docker|   2 +-
>  tests/docker/dockerfiles/ubuntu1804.docker|   2 +-
>  tests/qemu-iotests/207|   4 +-
>  tests/qemu-iotests/207.out|   2 +-
>  13 files changed, 376 insertions(+), 353 deletions(-)

Surprisingly little has changed since v4.  Good, good, that makes my
reviewing job much easier because I can thus rely on past me. :-)

> diff --git a/block/ssh.c b/block/ssh.c
> index 6da7b9cbfe..fb458d4548 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c

[...]

> @@ -282,82 +274,85 @@ static void ssh_parse_filename(const char *filename, 
> QDict *options,
>  parse_uri(filename, options, errp);
>  }
>  
> -static int check_host_key_knownhosts(BDRVSSHState *s,
> - const char *host, int port, Error 
> **errp)
> +static int check_host_key_knownhosts(BDRVSSHState *s, Error **errp)
>  {

[...]

> -switch (r) {
> -case LIBSSH2_KNOWNHOST_CHECK_MATCH:
> +switch (state) {
> +case SSH_KNOWN_HOSTS_OK:
>  /* OK */
> -trace_ssh_check_host_key_knownhosts(found->key);
> +trace_ssh_check_host_key_knownhosts();
>  break;
> -case LIBSSH2_KNOWNHOST_CHECK_MISMATCH:
> +case SSH_KNOWN_HOSTS_CHANGED:
>  ret = -EINVAL;
> -session_error_setg(errp, s,
> -  "host key does not match the one in known_hosts"
> -  " (found key %s)", found->key);
> +error_setg(errp, "host key does not match the one in known_hosts");

So what about the possible attack warning that the specification
technically requires us to print? O:-)

>  goto out;
> -case LIBSSH2_KNOWNHOST_CHECK_NOTFOUND:
> +case SSH_KNOWN_HOSTS_OTHER:
>  ret = -EINVAL;
> -session_error_setg(errp, s, "no host key was found in known_hosts");
> +error_setg(errp,
> +   "host key for this server not found, another type 
> exists");
>   

Re: [Qemu-block] [Qemu-devel] [PATCH v3 00/15] monitor: Split monitor.c in core/HMP/QMP/misc

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190613153405.24769-1-kw...@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20190613153405.24769-1-kw...@redhat.com
Subject: [Qemu-devel] [PATCH v3 00/15] monitor: Split monitor.c in 
core/HMP/QMP/misc
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
6d522fb vl: Deprecate -mon pretty=... for HMP monitors
e34ac59 monitor: Replace monitor_init() with monitor_init_{hmp, qmp}()
e75b96d monitor: Split Monitor.flags into separate bools
24a36f9 monitor: Split out monitor/monitor.c
27c4127 monitor: Split out monitor/hmp.c
26f727a monitor: Split out monitor/qmp.c
b2685af monitor: Create monitor-internal.h with common definitions
e1f6a6e monitor: Move {hmp, qmp}.c to monitor/{hmp, qmp}-cmds.c
ea374f7 Move monitor.c to monitor/misc.c
21e390e monitor: Rename HMP command type and tables
3335bfc monitor: Remove Monitor.cmd_table indirection
b935214 monitor: Create MonitorHMP with readline state
0800382 monitor: Make MonitorQMP a child class of Monitor
7c3f4f7 monitor: Split monitor_init in HMP and QMP function
6e68255 monitor: Remove unused password prompting fields

=== OUTPUT BEGIN ===
1/15 Checking commit 6e68255df41e (monitor: Remove unused password prompting 
fields)
2/15 Checking commit 7c3f4f792c86 (monitor: Split monitor_init in HMP and QMP 
function)
3/15 Checking commit 080038232dc8 (monitor: Make MonitorQMP a child class of 
Monitor)
4/15 Checking commit b935214b2c2c (monitor: Create MonitorHMP with readline 
state)
5/15 Checking commit 3335bfc40060 (monitor: Remove Monitor.cmd_table 
indirection)
6/15 Checking commit 21e390e01266 (monitor: Rename HMP command type and tables)
ERROR: spaces required around that '/' (ctx:VxV)
#226: FILE: monitor.c:4543:
+array_num = sizeof(hmp_cmds)/elem_size-1;
 ^

ERROR: spaces required around that '-' (ctx:VxV)
#226: FILE: monitor.c:4543:
+array_num = sizeof(hmp_cmds)/elem_size-1;
   ^

ERROR: spaces required around that '/' (ctx:VxV)
#231: FILE: monitor.c:4546:
+array_num = sizeof(hmp_info_cmds)/elem_size-1;
  ^

ERROR: spaces required around that '-' (ctx:VxV)
#231: FILE: monitor.c:4546:
+array_num = sizeof(hmp_info_cmds)/elem_size-1;
^

total: 4 errors, 0 warnings, 194 lines checked

Patch 6/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

7/15 Checking commit ea374f7b2aad (Move monitor.c to monitor/misc.c)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#80: 
new file mode 100644

total: 0 errors, 1 warnings, 78 lines checked

Patch 7/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
8/15 Checking commit e1f6a6ef1c06 (monitor: Move {hmp, qmp}.c to monitor/{hmp, 
qmp}-cmds.c)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#104: 
rename from hmp.c

total: 0 errors, 1 warnings, 73 lines checked

Patch 8/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
9/15 Checking commit b2685af3a437 (monitor: Create monitor-internal.h with 
common definitions)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#175: 
new file mode 100644

total: 0 errors, 1 warnings, 290 lines checked

Patch 9/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
10/15 Checking commit 26f727a38094 (monitor: Split out monitor/qmp.c)
ERROR: return is not a function, parentheses are not required
#564: FILE: monitor/monitor-internal.h:153:
+return (mon->flags & MONITOR_USE_CONTROL);

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#590: 
new file mode 100644

total: 1 errors, 1 warnings, 954 lines checked

Patch 10/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

11/15 Checking commit 27c412749fec (monitor: Split out monitor/hmp.c)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#28: 
new file mode 100644

ERROR: consider using qemu_strtoull in preference to strtoull
#432: FILE: monitor/hmp.c:400:
+n = strtoull(pch, , 0);

WARNING: Block comments use a leading /* on a separate line
#1314: FILE: monitor/hmp.c:1282:
+

[Qemu-block] [PATCH] iotests: Hide timestamps for skipped tests

[Max Reitz]

Currently, the "thistime" variable is not reinitialized on every loop
iteration.  This leads to tests that do not yield a run time (because
they failed or were skipped) printing the run time of the previous test
that did.  Fix that by reinitializing "thistime" for every test.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/check | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 44ebf24080..f925606cc5 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -773,6 +773,7 @@ do
 printdiff=false # show diff to reference output?
 status=""   # test result summary
 results=""  # test result details
+thistime="" # time the test took
 
 if [ -n "$TESTS_REMAINING_LOG" ] ; then
 sed -e "s/$seq//" -e 's/  / /' -e 's/^ *//' $TESTS_REMAINING_LOG > 
$TESTS_REMAINING_LOG.tmp
-- 
2.21.0

Re: [Qemu-block] [Qemu-devel] [PATCH v8 0/7] backup-top filter driver for backup

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20190529154654.95870-1-vsement...@virtuozzo.com/



Hi,

This series failed build test on s390x host. Please find the details below.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
set -e
CC=$HOME/bin/cc
INSTALL=$PWD/install
BUILD=$PWD/build
mkdir -p $BUILD $INSTALL
SRC=$PWD
cd $BUILD
$SRC/configure --cc=$CC --prefix=$INSTALL
make -j4
# XXX: we need reliable clean up
# make check -j4 V=1
make install

echo
echo "=== ENV ==="
env

echo
echo "=== PACKAGES ==="
rpm -qa
=== TEST SCRIPT END ===

  CC  block/replication.o
  CC  block/throttle.o
/var/tmp/patchew-tester-tmp-eg3dw7w5/src/block/backup.c: In function 
‘backup_run’:
/var/tmp/patchew-tester-tmp-eg3dw7w5/src/block/backup.c:283:8: error: 
‘error_is_read’ may be used uninitialized in this function 
[-Werror=maybe-uninitialized]
  283 | if (read) {
  |^
/var/tmp/patchew-tester-tmp-eg3dw7w5/src/block/backup.c:332:10: note: 
‘error_is_read’ was declared here


The full log is available at
http://patchew.org/logs/20190529154654.95870-1-vsement...@virtuozzo.com/testing.s390x/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [Qemu-block] [Qemu-devel] [PATCH v9] ssh: switch from libssh2 to libssh

[Paolo Bonzini]

On 13/06/19 19:15, Philippe Mathieu-Daudé wrote:
> 
> On 6/13/19 6:24 PM, no-re...@patchew.org wrote:
>> === TEST SCRIPT BEGIN ===
>> #!/bin/bash
>> time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
>> === TEST SCRIPT END ===
> 
>> The full log is available at
>> http://patchew.org/logs/20190613132000.2146-1-ptosc...@redhat.com/testing.asan/?type=message.
> 
> 
> === OUTPUT BEGIN ===
>   BUILD   fedora
> The command '/bin/sh -c dnf install -y $PACKAGES' returned a non-zero
> code: 1
> Traceback (most recent call last):
>   File "./tests/docker/docker.py", line 615, in 
> sys.exit(main())
>   File "./tests/docker/docker.py", line 611, in main
> return args.cmdobj.run(args, argv)
>   File "./tests/docker/docker.py", line 413, in run
> extra_files_cksum=cksum)
>   File "./tests/docker/docker.py", line 280, in build_image
> quiet=quiet)
>   File "./tests/docker/docker.py", line 207, in _do_check
> return subprocess.check_call(self._command + cmd, **kwargs)
>   File "/usr/lib64/python2.7/subprocess.py", line 542, in check_call
> raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['sudo', '-n', 'docker',
> 'build', '-t', 'qemu:fedora', '-f',
> '/tmp/docker_buildN2FMKc/tmpz_9Up_.docker', '/tmp/docker_buildN2FMKc']'
> returned non-zero exit status 1
> make: *** [docker-image-fedora] Error 1
> 
> real  3m54.376s
> 
> Not sure this is a network issue or something else, should we rebuild
> docker images with V=1 on patchew?
> 

I restarted the job with V=1.

Paolo

Re: [Qemu-block] [PATCH v8 7/7] block/backup: use backup-top instead of write notifiers

[Max Reitz]

On 29.05.19 17:46, Vladimir Sementsov-Ogievskiy wrote:
> Drop write notifiers and use filter node instead. Changes:
> 
> 1. copy-before-writes now handled by filter node, so, drop all
>is_write_notifier arguments.
> 
> 2. we don't have intersecting requests, so their handling is dropped.
> Instead, synchronization works as follows:
> when backup or backup-top starts copying of some area it firstly
> clears copy-bitmap bits, and nobody touches areas, not marked with
> dirty bits in copy-bitmap, so there is no intersection. Also, backup
> job copy operations are surrounded by bdrv region lock, which is
> actually serializing request, to not interfere with guest writes and
> not read changed data from source (before reading we clear
> corresponding bit in copy-bitmap, so, this area is not more handled by
> backup-top).
> 
> 3. To sync with in-flight requests at job finish we now have drained
> removing of the filter, we don't need rw-lock.
> 
> == RFC part ==
> 
> iotests changed:
> 56: op-blocker doesn't shot now, as we set it on source, but then check
> on filter, when trying to start second backup... Should I workaround it
> somehow?

Hm.  Where does that error message even come from?  The fact that the
target image is in use already (Due to file locks)?

It appears that way indeed.

It seems reasonable to me that you can now run a backup on top of
another backup.  Well, I mean, it is a stupid thing to do, but I don’t
see why the block layer would forbid doing so.

So the test seems superfluous to me.  If we want to keep it (why not),
it should test the opposite, namely that a backup to a different image
(with a different job ID) works.  (It seems simple enough to modify the
job that way, so why not.)

> 129: Hmm, now it is not busy at this moment.. But it's illegal to check
> busy, as job has pause-points and set busy to false in these points.
> Why we assert it in this test?

Nobody knows, it’s probably wrong.  All I know is that 129 is just
broken anyway.

> 141: Obvious, as drv0 is not root node now, but backing of the filter,
> when we try to remove it.

I get a failed assertion in 256.  That is probably because the
bdrv_set_aio_context() calls weren’t as unnecessary as I deemed them to be.

> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/backup.c | 171 ++---
>  tests/qemu-iotests/056 |   2 +-
>  tests/qemu-iotests/129 |   1 -
>  tests/qemu-iotests/141.out |   2 +-
>  4 files changed, 68 insertions(+), 108 deletions(-)

For some reason, my gcc starts to complain that backup_loop() may not
initialize error_is_read after this patch.  I don’t know why that is.
Perhaps it inlines backup_do_cow() now?  (So before it just saw that a
pointer to error_is_read was passed to backup_do_cow() and took it as an
opaque function, so it surely would set this value somewhere.  Now it
inlines it and it can’t find whether that will definitely happen, so it
complains.)

I don’t think it is strictly necessary to initialize error_is_read, but,
well, it won’t hurt.

> diff --git a/block/backup.c b/block/backup.c
> index 00f4f8af53..a5b8e04c9c 100644
> --- a/block/backup.c
> +++ b/block/backup.c

[...]

> @@ -60,56 +53,17 @@ typedef struct BackupBlockJob {
>  
>  static const BlockJobDriver backup_job_driver;
>  
> -/* See if in-flight requests overlap and wait for them to complete */
> -static void coroutine_fn wait_for_overlapping_requests(BackupBlockJob *job,
> -   int64_t start,
> -   int64_t end)
> -{
> -CowRequest *req;
> -bool retry;
> -
> -do {
> -retry = false;
> -QLIST_FOREACH(req, >inflight_reqs, list) {
> -if (end > req->start_byte && start < req->end_byte) {
> -qemu_co_queue_wait(>wait_queue, NULL);
> -retry = true;
> -break;
> -}
> -}
> -} while (retry);
> -}
> -
> -/* Keep track of an in-flight request */
> -static void cow_request_begin(CowRequest *req, BackupBlockJob *job,
> -  int64_t start, int64_t end)
> -{
> -req->start_byte = start;
> -req->end_byte = end;
> -qemu_co_queue_init(>wait_queue);
> -QLIST_INSERT_HEAD(>inflight_reqs, req, list);
> -}
> -
> -/* Forget about a completed request */
> -static void cow_request_end(CowRequest *req)
> -{
> -QLIST_REMOVE(req, list);
> -qemu_co_queue_restart_all(>wait_queue);
> -}
> -
>  /* Copy range to target with a bounce buffer and return the bytes copied. If
>   * error occurred, return a negative error number */
>  static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
>int64_t start,
>int64_t end,
> -  bool is_write_notifier,
>  

Re: [Qemu-block] [Qemu-devel] [RFC 0/3] block: Inquire images’ rotational info

[Max Reitz]

On 13.06.19 18:12, Stefan Hajnoczi wrote:
> On Fri, May 24, 2019 at 07:28:09PM +0200, Max Reitz wrote:
>> Hi,
>>
>> http://lists.nongnu.org/archive/html/qemu-block/2019-05/msg00569.html
>> shows that optimizations affect HDDs and SSDs differently.  It would be
>> nice if we could differentiate between the two and then choose to adjust
>> our behavior depending on whether a given image is stored on an HDD or
>> not.
>>
>> Or maybe it isn’t so nice.  That’s one reason this is an RFC.
> 
> Seems useful.
> 
> As long as this isn't exposed to the guest automatically (e.g. via SCSI)
> then it's fine since guest-visible values are not allowed to change
> across live migration or storage migration.

Urgh.  I wanted to do that in v1.  Then I guess I better won’t...

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v8 0/7] backup-top filter driver for backup

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20190529154654.95870-1-vsement...@virtuozzo.com/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC  crypto/cipher.o
  CC  crypto/tlscreds.o
  CC  crypto/tlscredsanon.o
/tmp/qemu-test/src/block/backup-top.c:268:5: error: implicit declaration of 
function 'bdrv_set_aio_context' is invalid in C99 
[-Werror,-Wimplicit-function-declaration]
bdrv_set_aio_context(top, bdrv_get_aio_context(source));
^
/tmp/qemu-test/src/block/backup-top.c:268:5: note: did you mean 
'bdrv_get_aio_context'?
/tmp/qemu-test/src/include/block/block.h:579:13: note: 'bdrv_get_aio_context' 
declared here
AioContext *bdrv_get_aio_context(BlockDriverState *bs);
^
/tmp/qemu-test/src/block/backup-top.c:268:5: error: this function declaration 
is not a prototype [-Werror,-Wstrict-prototypes]
bdrv_set_aio_context(top, bdrv_get_aio_context(source));
^
2 errors generated.


The full log is available at
http://patchew.org/logs/20190529154654.95870-1-vsement...@virtuozzo.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [Qemu-block] [Qemu-devel] [RFC 0/3] block: Inquire images’ rotational info

[Stefan Hajnoczi]

On Fri, May 24, 2019 at 07:28:09PM +0200, Max Reitz wrote:
> Hi,
> 
> http://lists.nongnu.org/archive/html/qemu-block/2019-05/msg00569.html
> shows that optimizations affect HDDs and SSDs differently.  It would be
> nice if we could differentiate between the two and then choose to adjust
> our behavior depending on whether a given image is stored on an HDD or
> not.
> 
> Or maybe it isn’t so nice.  That’s one reason this is an RFC.

Seems useful.

As long as this isn't exposed to the guest automatically (e.g. via SCSI)
then it's fine since guest-visible values are not allowed to change
across live migration or storage migration.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH 3/3] block/nbd: merge NBDClientSession struct back to BDRVNBDState

[Max Reitz]

On 13.06.19 17:16, Eric Blake wrote:
> On 6/11/19 5:27 AM, Vladimir Sementsov-Ogievskiy wrote:
>> No reason to keep it separate, it differs from others block driver
>> behavior and therefor confuses. Instead of generic
> 
> s/therefor/therefore/ (both spellings are valid, but the former looks
> archaic)
The things you teach me.

I will happily use “therefor” from now on whenever not too inappropriate.

(I like how Wiktionary notes that “therefor” is an anagram of
“therfore”.  That one looks nice, too.  I suppose I will just switch
between both at random.)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v9] ssh: switch from libssh2 to libssh

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190613132000.2146-1-ptosc...@redhat.com/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===




The full log is available at
http://patchew.org/logs/20190613132000.2146-1-ptosc...@redhat.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [Qemu-block] [PATCH v8 6/7] block: add lock/unlock range functions

[Max Reitz]

On 29.05.19 17:46, Vladimir Sementsov-Ogievskiy wrote:
> From: Vladimir Sementsov-Ogievskiy 
> 
> Introduce lock/unlock range functionality, based on serialized
> requests. This is needed to refactor backup, dropping local
> tracked-request-like synchronization.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  include/block/block_int.h |  4 
>  block/io.c| 44 ++-
>  2 files changed, 47 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 00/42] block: Deal with filters

[Max Reitz]

On 13.06.19 17:28, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 1:09, Max Reitz wrote:
>> Hi,
>>
>> When we introduced filters, we did it a bit casually.  Sure, we talked a
>> lot about them before, but that was mostly discussion about where
>> implicit filters should be added to the graph (note that we currently
>> only have two implicit filters, those being mirror and commit).  But in
>> the end, we really just designated some drivers filters (Quorum,
>> blkdebug, etc.) and added some specifically (throttle, COR), without
>> really looking through the block layer to see where issues might occur.
>>
>> It turns out vast areas of the block layer just don’t know about filters
>> and cannot really handle them.  Many cases will work in practice, in
>> others, well, too bad, you cannot use some feature because some part
>> deep inside the block layer looks at your filters and thinks they are
>> format nodes.
>>
>> This is one reason why this series is needed.  Over time (since v1), a
>> second reason has made its way in:
>>
>> bs->file is not necessarily the place where a node’s data is stored.
>> qcow2 now has external data files, and currently there is no way for the
>> general block layer to know that the data is not stored in bs->file.
>> Right now, I do not think that has any real consequences (all functions
>> that need access to the actual data storage file should only do so as a
>> fallback if the driver does not provide some functionality, but qcow2
>> should provide it all), but it still shows that we need some way to let
>> the general block layer know about such data files.  (Also, I will need
>> this for v1 of my “Inquire images’ rotational info” series.)
>>
>> I won’t go on and on about this series now, I think the patches pretty
>> much speak for themselves now.  If the cover letter gets too long,
>> nobody reads it anyway (see previous versions).
>>
>>
>> *** This series depends on some others. ***
>>
>> Dependencies:
>> - [PATCH 0/4] block: Keep track of parent quiescing
>> - [PATCH 0/2] vl: Drain before (block) job cancel when quitting
>> - [PATCH v2 0/2] blockdev: Overlays are not snapshots
>>
>> Based-on: <20190605161118.14544-1-mre...@redhat.com>
>> Based-on: <20190612220839.1374-1-mre...@redhat.com>
>> Based-on: <20190603202236.1342-1-mre...@redhat.com>
> 
> Could you please export a branch?

Sure:

https://git.xanclic.moe/XanClic/qemu child-access-functions-v5
Or:
https://github.com/XanClic/qemu child-access-functions-v5


(And the base branch is:

https://git.xanclic.moe/XanClic/qemu child-access-functions-base
https://github.com/XanClic/qemu child-access-functions-base
)

>> v5:
>> - Split the huge patches 2 and 3 from the previous version into many
>>smaller patches to maintain the potential reviewers’ sanity [Vladimir]
> 
> Thank you! In spite of frightening amount of patches, reviewing became a lot
> simpler.

I had hoped making it exactly 42 patches would make it a bit more welcoming.

Again, thanks a lot for reviewing!

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v9] ssh: switch from libssh2 to libssh

[Philippe Mathieu-Daudé]

On 6/13/19 6:24 PM, no-re...@patchew.org wrote:
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
> === TEST SCRIPT END ===

> The full log is available at
> http://patchew.org/logs/20190613132000.2146-1-ptosc...@redhat.com/testing.asan/?type=message.


=== OUTPUT BEGIN ===
  BUILD   fedora
The command '/bin/sh -c dnf install -y $PACKAGES' returned a non-zero
code: 1
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 615, in 
sys.exit(main())
  File "./tests/docker/docker.py", line 611, in main
return args.cmdobj.run(args, argv)
  File "./tests/docker/docker.py", line 413, in run
extra_files_cksum=cksum)
  File "./tests/docker/docker.py", line 280, in build_image
quiet=quiet)
  File "./tests/docker/docker.py", line 207, in _do_check
return subprocess.check_call(self._command + cmd, **kwargs)
  File "/usr/lib64/python2.7/subprocess.py", line 542, in check_call
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker',
'build', '-t', 'qemu:fedora', '-f',
'/tmp/docker_buildN2FMKc/tmpz_9Up_.docker', '/tmp/docker_buildN2FMKc']'
returned non-zero exit status 1
make: *** [docker-image-fedora] Error 1

real3m54.376s

Not sure this is a network issue or something else, should we rebuild
docker images with V=1 on patchew?

Re: [Qemu-block] [PATCH v8 4/7] block: introduce backup-top filter driver

[Max Reitz]

On 29.05.19 17:46, Vladimir Sementsov-Ogievskiy wrote:
> Backup-top filter does copy-before-write operation. It should be
> inserted above active disk and has a target node for CBW, like the
> following:
> 
> +---+
> | Guest |
> +---+
> |r,w
> v
> ++  target   +---+
> | backup_top |-->| target(qcow2) |
> ++   CBW +---+
> |
> backing |r,w
> v
> +-+
> | Active disk |
> +-+
> 
> The driver will be used in backup instead of write-notifiers.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/backup-top.h  |  64 +
>  block/backup-top.c  | 322 
>  block/Makefile.objs |   2 +
>  3 files changed, 388 insertions(+)
>  create mode 100644 block/backup-top.h
>  create mode 100644 block/backup-top.c
> 
> diff --git a/block/backup-top.h b/block/backup-top.h
> new file mode 100644
> index 00..788e18c358
> --- /dev/null
> +++ b/block/backup-top.h
> @@ -0,0 +1,64 @@
> +/*
> + * backup-top filter driver
> + *
> + * The driver performs Copy-Before-Write (CBW) operation: it is injected 
> above
> + * some node, and before each write it copies _old_ data to the target node.
> + *
> + * Copyright (c) 2018 Virtuozzo International GmbH. All rights reserved.
> + *
> + * Author:
> + *  Sementsov-Ogievskiy Vladimir 
> + *
> + * 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 .
> + */
> +
> +#ifndef BACKUP_TOP_H
> +#define BACKUP_TOP_H
> +
> +#include "qemu/osdep.h"
> +
> +#include "block/block_int.h"
> +
> +typedef void (*BackupTopProgressCallback)(uint64_t done, void *opaque);
> +typedef struct BDRVBackupTopState {
> +HBitmap *copy_bitmap; /* what should be copied to @target on guest 
> write. */
> +BdrvChild *target;
> +
> +BackupTopProgressCallback progress_cb;
> +void *progress_opaque;
> +} BDRVBackupTopState;
> +
> +/*
> + * bdrv_backup_top_append
> + *
> + * Append backup_top filter node above @source node. @target node will 
> receive
> + * the data backed up during CBE operations. New filter together with @target
> + * node are attached to @source aio context.
> + *
> + * The resulting filter node is implicit.

Why?  It’s just as easy for the caller to just make it implicit if it
should be.  (And usually the caller should decide that.)

> + *
> + * @copy_bitmap selects regions which needs CBW. Furthermore, backup_top will
> + * use exactly this bitmap, so it may be used to control backup_top behavior
> + * dynamically. Caller should not release @copy_bitmap during life-time of
> + * backup_top. Progress is tracked by calling @progress_cb function.
> + */
> +BlockDriverState *bdrv_backup_top_append(
> +BlockDriverState *source, BlockDriverState *target,
> +HBitmap *copy_bitmap, Error **errp);
> +void bdrv_backup_top_set_progress_callback(
> +BlockDriverState *bs, BackupTopProgressCallback progress_cb,
> +void *progress_opaque);
> +void bdrv_backup_top_drop(BlockDriverState *bs);
> +
> +#endif /* BACKUP_TOP_H */
> diff --git a/block/backup-top.c b/block/backup-top.c
> new file mode 100644
> index 00..1daa02f539
> --- /dev/null
> +++ b/block/backup-top.c
> @@ -0,0 +1,322 @@
> +/*
> + * backup-top filter driver
> + *
> + * The driver performs Copy-Before-Write (CBW) operation: it is injected 
> above
> + * some node, and before each write it copies _old_ data to the target node.
> + *
> + * Copyright (c) 2018 Virtuozzo International GmbH. All rights reserved.
> + *
> + * Author:
> + *  Sementsov-Ogievskiy Vladimir 
> + *
> + * 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 .
> + */
> +
> +#include "qemu/osdep.h"
> +
> 

Re: [Qemu-block] [Qemu-devel] [PATCH 1/2] vl: Drain before (block) job cancel when quitting

[Max Reitz]

[re-adding the original CCs, why not]

On 13.06.19 16:30, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 17:21, Max Reitz wrote:
>> On 13.06.19 16:19, Vladimir Sementsov-Ogievskiy wrote:
>>> 13.06.2019 1:08, Max Reitz wrote:
 If the main loop cancels all block jobs while the block layer is not
 drained, this cancelling may not happen instantaneously.  We can start a
 drained section before vm_shutdown(), which entails another
 bdrv_drain_all(); this nested bdrv_drain_all() will thus be a no-op,
 basically.

 We do not have to end the drained section, because we actually do not
 want any requests to happen from this point on.

 Signed-off-by: Max Reitz 
 ---
 I don't know whether it actually makes sense to never end this drained
 section.  It makes sense to me.  Please correct me if I'm wrong.
 ---
vl.c | 11 +++
1 file changed, 11 insertions(+)

 diff --git a/vl.c b/vl.c
 index cd1fbc4cdc..3f8b3f74f5 100644
 --- a/vl.c
 +++ b/vl.c
 @@ -4538,6 +4538,17 @@ int main(int argc, char **argv, char **envp)
 */
migration_shutdown();

 +/*
 + * We must cancel all block jobs while the block layer is drained,
 + * or cancelling will be affected by throttling and thus may block
 + * for an extended period of time.
 + * vm_shutdown() will bdrv_drain_all(), so we may as well include
 + * it in the drained section.
 + * We do not need to end this section, because we do not want any
 + * requests happening from here on anyway.
 + */
 +bdrv_drain_all_begin();
 +
/* No more vcpu or device emulation activity beyond this point */
vm_shutdown();


>>>
>>> So, actually, the problem is that we may wait for job requests twice:
>>> on drain and then on cancel.
>>
>> We don’t wait on drain.  When the throttle node is drained, it will
>> ignore throttling (as noted in the cover letter).
>>
>> We do wait when cancelling a job while the throttle node isn’t drained,
>> though.  That’s the problem.
> 
> Ah, understand now.
> 
> Is it safe to drain_begin before stopping cpus? We may finish up then with 
> some queued
> somewhere IO requests..

Hm...  Aren’t guest devices prohibited from issuing requests to the
block layer while their respective block device is drained?

Otherwise, I suppose I’ll have to move the bdrv_drain_all_begin() below
the vm_shutdown().  That wouldn’t be too big of a problem.

Max



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PULL 4/9] iotests: Use qemu-nbd's --pid-file

[Eric Blake]

From: Max Reitz 

Signed-off-by: Max Reitz 
Message-Id: <20190508211820.17851-5-mre...@redhat.com>
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/common.rc | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 93f87389b60e..5502c3da2f87 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -105,10 +105,8 @@ _qemu_io_wrapper()

 _qemu_nbd_wrapper()
 {
-(
-echo $BASHPID > "${QEMU_TEST_DIR}/qemu-nbd.pid"
-exec "$QEMU_NBD_PROG" $QEMU_NBD_OPTIONS "$@"
-)
+"$QEMU_NBD_PROG" --pid-file="${QEMU_TEST_DIR}/qemu-nbd.pid" \
+ $QEMU_NBD_OPTIONS "$@"
 }

 _qemu_vxhs_wrapper()
-- 
2.20.1

[Qemu-block] [PULL 6/9] nbd/server: Nicer spelling of max BLOCK_STATUS reply length

[Eric Blake]

Commit 3d068aff (3.0) introduced NBD_MAX_BITMAP_EXTENTS as a limit on
how large we would allow a reply to NBD_CMD_BLOCK_STATUS to grow when
it is visiting a qemu:dirty-bitmap: context.  Later, commit fb7afc79
(3.1) reused the constant to limit base:allocation context replies,
although the name is now less appropriate in that situation.

Rename things, and improve the macro to use units.h for better
legibility. Then reformat the comment to comply with checkpatch rules
added in the meantime. No semantic change.

Signed-off-by: Eric Blake 
Message-Id: <20190510151735.29687-1-ebl...@redhat.com>
Reviewed-by: Stefano Garzarella 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 nbd/server.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index aeca3893fe2b..10faedcfc55d 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -21,15 +21,18 @@
 #include "qapi/error.h"
 #include "trace.h"
 #include "nbd-internal.h"
+#include "qemu/units.h"

 #define NBD_META_ID_BASE_ALLOCATION 0
 #define NBD_META_ID_DIRTY_BITMAP 1

-/* NBD_MAX_BITMAP_EXTENTS: 1 mb of extents data. An empirical
+/*
+ * NBD_MAX_BLOCK_STATUS_EXTENTS: 1 MiB of extents data. An empirical
  * constant. If an increase is needed, note that the NBD protocol
  * recommends no larger than 32 mb, so that the client won't consider
- * the reply as a denial of service attack. */
-#define NBD_MAX_BITMAP_EXTENTS (0x10 / 8)
+ * the reply as a denial of service attack.
+ */
+#define NBD_MAX_BLOCK_STATUS_EXTENTS (1 * MiB / 8)

 static int system_errno_to_nbd_errno(int err)
 {
@@ -1960,7 +1963,7 @@ static int nbd_co_send_block_status(NBDClient *client, 
uint64_t handle,
 Error **errp)
 {
 int ret;
-unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BITMAP_EXTENTS;
+unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BLOCK_STATUS_EXTENTS;
 NBDExtent *extents = g_new(NBDExtent, nb_extents);
 uint64_t final_length = length;

@@ -2045,7 +2048,7 @@ static int nbd_co_send_bitmap(NBDClient *client, uint64_t 
handle,
   uint32_t context_id, Error **errp)
 {
 int ret;
-unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BITMAP_EXTENTS;
+unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BLOCK_STATUS_EXTENTS;
 NBDExtent *extents = g_new(NBDExtent, nb_extents);
 uint64_t final_length = length;

-- 
2.20.1

[Qemu-block] [PULL 1/9] qemu-nbd: Add --pid-file option

[Eric Blake]

From: Max Reitz 

--fork is a bit boring if there is no way to get the child's PID.  This
option helps.

Signed-off-by: Max Reitz 
Message-Id: <20190508211820.17851-2-mre...@redhat.com>
Signed-off-by: Eric Blake 
---
 qemu-nbd.texi |  2 ++
 qemu-nbd.c| 11 +++
 2 files changed, 13 insertions(+)

diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index de342c76b873..7f55657722bd 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -117,6 +117,8 @@ option; or provide the credentials needed for connecting as 
a client
 in list mode.
 @item --fork
 Fork off the server process and exit the parent once the server is running.
+@item --pid-file=PATH
+Store the server's process ID in the given file.
 @item --tls-authz=ID
 Specify the ID of a qauthz object previously created with the
 --object option. This will be used to authorize connecting users
diff --git a/qemu-nbd.c b/qemu-nbd.c
index e24dd2f76766..99377a3f14e2 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -61,6 +61,7 @@
 #define QEMU_NBD_OPT_IMAGE_OPTS262
 #define QEMU_NBD_OPT_FORK  263
 #define QEMU_NBD_OPT_TLSAUTHZ  264
+#define QEMU_NBD_OPT_PID_FILE  265

 #define MBR_SIZE 512

@@ -113,6 +114,7 @@ static void usage(const char *name)
 "specify tracing options\n"
 "  --forkfork off the server process and exit the parent\n"
 "once the server is running\n"
+"  --pid-file=PATH   store the server's process ID in the given file\n"
 #if HAVE_NBD_DEVICE
 "\n"
 "Kernel NBD client support:\n"
@@ -641,6 +643,7 @@ int main(int argc, char **argv)
 { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
 { "trace", required_argument, NULL, 'T' },
 { "fork", no_argument, NULL, QEMU_NBD_OPT_FORK },
+{ "pid-file", required_argument, NULL, QEMU_NBD_OPT_PID_FILE },
 { NULL, 0, NULL, 0 }
 };
 int ch;
@@ -667,6 +670,7 @@ int main(int argc, char **argv)
 bool list = false;
 int old_stderr = -1;
 unsigned socket_activation;
+const char *pid_file_name = NULL;

 /* The client thread uses SIGTERM to interrupt the server.  A signal
  * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
@@ -866,6 +870,9 @@ int main(int argc, char **argv)
 case 'L':
 list = true;
 break;
+case QEMU_NBD_OPT_PID_FILE:
+pid_file_name = optarg;
+break;
 }
 }

@@ -1186,6 +1193,10 @@ int main(int argc, char **argv)

 nbd_update_server_watch();

+if (pid_file_name) {
+qemu_write_pidfile(pid_file_name, _fatal);
+}
+
 /* now when the initialization is (almost) complete, chdir("/")
  * to free any busy filesystems */
 if (chdir("/") < 0) {
-- 
2.20.1

[Qemu-block] [PULL 7/9] block/nbd-client: drop stale logout

[Eric Blake]

From: Vladimir Sementsov-Ogievskiy 

Drop one on failure path (we have errp) and turn two others into trace
points.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20190611102720.86114-2-vsement...@virtuozzo.com>
Reviewed-by: Eric Blake 
Signed-off-by: Eric Blake 
---
 block/nbd-client.h | 9 -
 block/nbd-client.c | 6 +++---
 block/trace-events | 2 ++
 3 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index 4587053114af..570538f4c884 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -5,15 +5,6 @@
 #include "block/block_int.h"
 #include "io/channel-socket.h"

-/* #define DEBUG_NBD */
-
-#if defined(DEBUG_NBD)
-#define logout(fmt, ...) \
-fprintf(stderr, "nbd\t%-24s" fmt, __func__, ##__VA_ARGS__)
-#else
-#define logout(fmt, ...) ((void)0)
-#endif
-
 #define MAX_NBD_REQUESTS16

 typedef struct {
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 790ecc1ee1cb..f89a67c23b64 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -1136,7 +1136,7 @@ static int nbd_client_connect(BlockDriverState *bs,
 }

 /* NBD handshake */
-logout("session init %s\n", export);
+trace_nbd_client_connect(export);
 qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);

 client->info.request_sizes = true;
@@ -1149,7 +1149,6 @@ static int nbd_client_connect(BlockDriverState *bs,
 g_free(client->info.x_dirty_bitmap);
 g_free(client->info.name);
 if (ret < 0) {
-logout("Failed to negotiate with the NBD server\n");
 object_unref(OBJECT(sioc));
 return ret;
 }
@@ -1187,7 +1186,8 @@ static int nbd_client_connect(BlockDriverState *bs,
 bdrv_inc_in_flight(bs);
 nbd_client_attach_aio_context(bs, bdrv_get_aio_context(bs));

-logout("Established connection with NBD server\n");
+trace_nbd_client_connect_success(export);
+
 return 0;

  fail:
diff --git a/block/trace-events b/block/trace-events
index eab51497fc0f..01fa5eb081be 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -165,6 +165,8 @@ nbd_parse_blockstatus_compliance(const char *err) "ignoring 
extra data from non-
 nbd_structured_read_compliance(const char *type) "server sent non-compliant 
unaligned read %s chunk"
 nbd_read_reply_entry_fail(int ret, const char *err) "ret = %d, err: %s"
 nbd_co_request_fail(uint64_t from, uint32_t len, uint64_t handle, uint16_t 
flags, uint16_t type, const char *name, int ret, const char *err) "Request 
failed { .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags 
= 0x%" PRIx16 ", .type = %" PRIu16 " (%s) } ret = %d, err: %s"
+nbd_client_connect(const char *export_name) "export '%s'"
+nbd_client_connect_success(const char *export_name) "export '%s'"

 # ssh.c
 ssh_restart_coroutine(void *co) "co=%p"
-- 
2.20.1

[Qemu-block] [PULL 5/9] iotests: Let 233 run concurrently

[Eric Blake]

From: Max Reitz 

common.nbd's nbd_server_set_tcp_port() tries to find a free port, and
then uses it for the whole test run.  However, this is racy because even
if the port was free at the beginning, there is no guarantee it will
continue to be available.  Therefore, 233 currently cannot reliably be
run concurrently with other NBD TCP tests.

This patch addresses the problem by dropping nbd_server_set_tcp_port(),
and instead finding a new port every time nbd_server_start_tcp_socket()
is invoked.  For this, we run qemu-nbd with --fork and on error evaluate
the output to see whether it contains "Address already in use".  If so,
we try the next port.

On success, we still want to continually redirect the output from
qemu-nbd to stderr.  To achieve both, we redirect qemu-nbd's stderr to a
FIFO that we then open in bash.  If the parent process exits with status
0 (which means that the server has started successfully), we launch a
background cat process that copies the FIFO to stderr.  On failure, we
read the whole content into a variable and then evaluate it.

While at it, use --fork in nbd_server_start_unix_socket(), too.  Doing
so allows us to drop nbd_server_wait_for_*_socket().

Note that the reason common.nbd did not use --fork before is that
qemu-nbd did not have --pid-file.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Message-Id: <20190508211820.17851-6-mre...@redhat.com>
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/common.nbd | 100 +++---
 tests/qemu-iotests/233|   1 -
 2 files changed, 45 insertions(+), 56 deletions(-)

diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
index 25fc9ffaa46b..24b01b60aae1 100644
--- a/tests/qemu-iotests/common.nbd
+++ b/tests/qemu-iotests/common.nbd
@@ -22,6 +22,11 @@
 nbd_unix_socket="${TEST_DIR}/qemu-nbd.sock"
 nbd_tcp_addr="127.0.0.1"
 nbd_pid_file="${TEST_DIR}/qemu-nbd.pid"
+nbd_stderr_fifo="${TEST_DIR}/qemu-nbd.fifo"
+
+# If bash version is >= 4.1, this will be overwritten by a dynamically
+# assigned file descriptor value.
+nbd_fifo_fd=10

 nbd_server_stop()
 {
@@ -33,77 +38,62 @@ nbd_server_stop()
 kill "$NBD_PID"
 fi
 fi
-rm -f "$nbd_unix_socket"
-}
-
-nbd_server_wait_for_unix_socket()
-{
-pid=$1
-
-for ((i = 0; i < 300; i++))
-do
-if [ -r "$nbd_unix_socket" ]; then
-return
-fi
-kill -s 0 $pid 2>/dev/null
-if test $? != 0
-then
-echo "qemu-nbd unexpectedly quit"
-exit 1
-fi
-sleep 0.1
-done
-echo "Failed in check of unix socket created by qemu-nbd"
-exit 1
+rm -f "$nbd_unix_socket" "$nbd_stderr_fifo"
 }

 nbd_server_start_unix_socket()
 {
 nbd_server_stop
-$QEMU_NBD -v -t -k "$nbd_unix_socket" "$@" &
-nbd_server_wait_for_unix_socket $!
+$QEMU_NBD -v -t -k "$nbd_unix_socket" --fork "$@"
 }

-nbd_server_set_tcp_port()
+nbd_server_start_tcp_socket()
 {
-(ss --help) >/dev/null 2>&1 || _notrun "ss utility not found, skipping 
test"
+nbd_server_stop

+mkfifo "$nbd_stderr_fifo"
 for ((port = 10809; port <= 10909; port++))
 do
-if ! ss -tln | grep -sqE ":$port\b"; then
+# Redirect stderr to FIFO, so we can later decide whether we
+# want to read it or to redirect it to our stderr, depending
+# on whether the command fails or not
+$QEMU_NBD -v -t -b $nbd_tcp_addr -p $port --fork "$@" \
+2> "$nbd_stderr_fifo" &
+
+# Taken from common.qemu
+if [[ "${BASH_VERSINFO[0]}" -ge "5" ||
+("${BASH_VERSINFO[0]}" -ge "4" && "${BASH_VERSINFO[1]}" -ge "1") ]]
+then
+exec {nbd_fifo_fd}<"$nbd_stderr_fifo"
+else
+let _nbd_fifo_fd++
+eval "exec ${_nbd_fifo_fd}<'$nbd_stderr_fifo'"
+fi
+wait $!
+
+if test $? == 0
+then
+# Success, redirect qemu-nbd's stderr to our stderr
 nbd_tcp_port=$port
+(cat <&$nbd_fifo_fd >&2) &
+eval "exec $nbd_fifo_fd>&-"
 return
 fi
+
+# Failure, read the output
+output=$(cat <&$nbd_fifo_fd)
+eval "exec $nbd_fifo_fd>&-"
+
+if ! echo "$output" | grep -q "Address already in use"
+then
+# Unknown error, print it
+echo "$output" >&2
+rm -f "$nbd_stderr_fifo"
+exit 1
+fi
 done

 echo "Cannot find free TCP port for nbd in range 10809-10909"
+rm -f "$nbd_stderr_fifo"
 exit 1
 }
-
-nbd_server_wait_for_tcp_socket()
-{
-pid=$1
-
-for ((i = 0; i < 300; i++))
-do
-if ss -tln | grep -sqE ":$nbd_tcp_port\b"; then
-return
-fi
-kill -s 0 $pid 2>/dev/null
-if test $? != 0
-then
-echo "qemu-nbd unexpectedly quit"
-exit 1
-fi
-sleep 0.1
-done
-echo "Failed in check of TCP socket created 

[Qemu-block] [PULL 2/9] iotests.py: Add qemu_nbd_early_pipe()

[Eric Blake]

From: Max Reitz 

qemu_nbd_pipe() currently unconditionally reads qemu-nbd's output.  That
is not ideal because qemu-nbd may keep stderr open after the parent
process has exited.

Currently, the only user of qemu_nbd_pipe() is 147, which discards the
whole output if the parent process returned success and only evaluates
it on error.  Therefore, we can replace qemu_nbd_pipe() by
qemu_nbd_early_pipe() that does the same: Discard the output on success,
and return it on error.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Message-Id: <20190508211820.17851-3-mre...@redhat.com>
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/147| 4 ++--
 tests/qemu-iotests/iotests.py | 9 ++---
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
index 82513279b028..2d84fddb0107 100755
--- a/tests/qemu-iotests/147
+++ b/tests/qemu-iotests/147
@@ -24,7 +24,7 @@ import socket
 import stat
 import time
 import iotests
-from iotests import cachemode, imgfmt, qemu_img, qemu_nbd, qemu_nbd_pipe
+from iotests import cachemode, imgfmt, qemu_img, qemu_nbd, qemu_nbd_early_pipe

 NBD_PORT_START  = 32768
 NBD_PORT_END= NBD_PORT_START + 1024
@@ -93,7 +93,7 @@ class QemuNBD(NBDBlockdevAddBase):
 pass

 def _try_server_up(self, *args):
-status, msg = qemu_nbd_pipe('-f', imgfmt, test_img, *args)
+status, msg = qemu_nbd_early_pipe('-f', imgfmt, test_img, *args)
 if status == 0:
 return True
 if 'Address already in use' in msg:
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 6bcddd887048..f11482f3dc9b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -209,9 +209,9 @@ def qemu_nbd(*args):
 '''Run qemu-nbd in daemon mode and return the parent's exit code'''
 return subprocess.call(qemu_nbd_args + ['--fork'] + list(args))

-def qemu_nbd_pipe(*args):
+def qemu_nbd_early_pipe(*args):
 '''Run qemu-nbd in daemon mode and return both the parent's exit code
-   and its output'''
+   and its output in case of an error'''
 subp = subprocess.Popen(qemu_nbd_args + ['--fork'] + list(args),
 stdout=subprocess.PIPE,
 stderr=subprocess.STDOUT,
@@ -221,7 +221,10 @@ def qemu_nbd_pipe(*args):
 sys.stderr.write('qemu-nbd received signal %i: %s\n' %
  (-exitcode,
   ' '.join(qemu_nbd_args + ['--fork'] + list(args
-return exitcode, subp.communicate()[0]
+if exitcode == 0:
+return exitcode, ''
+else:
+return exitcode, subp.communicate()[0]

 def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
 '''Return True if two image files are identical'''
-- 
2.20.1

[Qemu-block] [PULL 3/9] qemu-nbd: Do not close stderr

[Eric Blake]

From: Max Reitz 

We kept old_stderr specifically so we could keep emitting error message
on stderr.  However, qemu_daemon() closes stderr.  Therefore, we need to
dup() stderr to old_stderr before invoking qemu_daemon().

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Message-Id: <20190508211820.17851-4-mre...@redhat.com>
Signed-off-by: Eric Blake 
---
 qemu-nbd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 99377a3f14e2..a8cb39e51043 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1004,10 +1004,11 @@ int main(int argc, char **argv)
 exit(EXIT_FAILURE);
 } else if (pid == 0) {
 close(stderr_fd[0]);
+
+old_stderr = dup(STDERR_FILENO);
 ret = qemu_daemon(1, 0);

 /* Temporarily redirect stderr to the parent's pipe...  */
-old_stderr = dup(STDERR_FILENO);
 dup2(stderr_fd[1], STDERR_FILENO);
 if (ret < 0) {
 error_report("Failed to daemonize: %s", strerror(errno));
-- 
2.20.1

[Qemu-block] [PATCH v3 15/15] vl: Deprecate -mon pretty=... for HMP monitors

[Kevin Wolf]

The -mon pretty=on|off switch of the -mon option applies only the QMP
monitors. It used to be silently ignored for HMP. Deprecate this
combination so that we can make it an error in future versions.

Signed-off-by: Kevin Wolf 
---
 vl.c | 10 +-
 qemu-deprecated.texi |  6 ++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index 32daa434eb..99a56b5556 100644
--- a/vl.c
+++ b/vl.c
@@ -2317,6 +2317,10 @@ static int mon_init_func(void *opaque, QemuOpts *opts, 
Error **errp)
 return -1;
 }
 
+if (!qmp && qemu_opt_get(opts, "pretty")) {
+warn_report("'pretty' is deprecated for HMP monitors, it has no effect 
"
+"and will be removed in future versions");
+}
 if (qemu_opt_get_bool(opts, "pretty", 0)) {
 pretty = true;
 }
@@ -2362,7 +2366,11 @@ static void monitor_parse(const char *optarg, const char 
*mode, bool pretty)
 opts = qemu_opts_create(qemu_find_opts("mon"), label, 1, _fatal);
 qemu_opt_set(opts, "mode", mode, _abort);
 qemu_opt_set(opts, "chardev", label, _abort);
-qemu_opt_set_bool(opts, "pretty", pretty, _abort);
+if (!strcmp(mode, "control")) {
+qemu_opt_set_bool(opts, "pretty", pretty, _abort);
+} else {
+assert(pretty == false);
+}
 monitor_device_index++;
 }
 
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 50292d820b..df04f2840b 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -72,6 +72,12 @@ backend settings instead of environment variables.  To ease 
migration to
 the new format, the ``-audiodev-help'' option can be used to convert
 the current values of the environment variables to ``-audiodev'' options.
 
+@subsection -mon ...,control=readline,pretty=on|off (since 4.1)
+
+The @code{pretty=on|off} switch has no effect for HMP monitors, but is
+silently ignored. Using the switch with HMP monitors will become an
+error in the future.
+
 @subsection -realtime (since 4.1)
 
 The @code{-realtime mlock=on|off} argument has been replaced by the
-- 
2.20.1

[Qemu-block] [PATCH v3 14/15] monitor: Replace monitor_init() with monitor_init_{hmp, qmp}()

[Kevin Wolf]

Most callers know which monitor type they want to have. Instead of
calling monitor_init() with flags that can describe both types of
monitors, make monitor_init_{hmp,qmp}() public interfaces that take
specific bools instead of flags and call these functions directly.

Signed-off-by: Kevin Wolf 
---
 include/monitor/monitor.h  |  9 ++---
 monitor/monitor-internal.h |  3 ---
 chardev/char.c |  2 +-
 gdbstub.c  |  2 +-
 monitor/hmp.c  |  4 ++--
 monitor/monitor.c  |  9 -
 monitor/qmp.c  |  7 ++-
 stubs/monitor.c|  6 +-
 tests/test-util-sockets.c  |  3 ++-
 vl.c   | 18 --
 10 files changed, 27 insertions(+), 36 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index af5ddbe785..6e64f062db 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -8,19 +8,14 @@
 extern __thread Monitor *cur_mon;
 typedef struct MonitorHMP MonitorHMP;
 
-/* flags for monitor_init */
-/* 0x01 unused */
-#define MONITOR_USE_READLINE  0x02
-#define MONITOR_USE_CONTROL   0x04
-#define MONITOR_USE_PRETTY0x08
-
 #define QMP_REQ_QUEUE_LEN_MAX 8
 
 bool monitor_cur_is_qmp(void);
 
 void monitor_init_globals(void);
 void monitor_init_globals_core(void);
-void monitor_init(Chardev *chr, int flags);
+void monitor_init_qmp(Chardev *chr, bool pretty);
+void monitor_init_hmp(Chardev *chr, bool use_readline);
 void monitor_cleanup(void);
 
 int monitor_suspend(Monitor *mon);
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 260725e51b..333ebf89e4 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -167,9 +167,6 @@ extern int mon_refcount;
 
 extern HMPCommand hmp_cmds[];
 
-void monitor_init_qmp(Chardev *chr, int flags);
-void monitor_init_hmp(Chardev *chr, int flags);
-
 int monitor_puts(Monitor *mon, const char *str);
 void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
bool use_io_thread);
diff --git a/chardev/char.c b/chardev/char.c
index e4887bcc82..7b6b2cb123 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -731,7 +731,7 @@ Chardev *qemu_chr_new_noreplay(const char *label, const 
char *filename,
 
 if (qemu_opt_get_bool(opts, "mux", 0)) {
 assert(permit_mux_mon);
-monitor_init(chr, MONITOR_USE_READLINE);
+monitor_init_hmp(chr, true);
 }
 
 out:
diff --git a/gdbstub.c b/gdbstub.c
index 17dcd9186d..939134eec9 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2565,7 +2565,7 @@ int gdbserver_start(const char *device)
 /* Initialize a monitor terminal for gdb */
 mon_chr = qemu_chardev_new(NULL, TYPE_CHARDEV_GDB,
NULL, NULL, _abort);
-monitor_init(mon_chr, 0);
+monitor_init_hmp(mon_chr, false);
 } else {
 qemu_chr_fe_deinit(>chr, true);
 mon_chr = s->mon_chr;
diff --git a/monitor/hmp.c b/monitor/hmp.c
index 5ba8b6e8d5..43185a7445 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -1393,14 +1393,14 @@ static void monitor_readline_flush(void *opaque)
 monitor_flush(>common);
 }
 
-void monitor_init_hmp(Chardev *chr, int flags)
+void monitor_init_hmp(Chardev *chr, bool use_readline)
 {
 MonitorHMP *mon = g_malloc0(sizeof(*mon));
 
 monitor_data_init(>common, false, false, false);
 qemu_chr_fe_init(>common.chr, chr, _abort);
 
-mon->use_readline = flags & MONITOR_USE_READLINE;
+mon->use_readline = use_readline;
 if (mon->use_readline) {
 mon->rs = readline_init(monitor_readline_printf,
 monitor_readline_flush,
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 7325e4362b..01d8fb5d30 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -555,15 +555,6 @@ void monitor_data_destroy(Monitor *mon)
 qemu_mutex_destroy(>mon_lock);
 }
 
-void monitor_init(Chardev *chr, int flags)
-{
-if (flags & MONITOR_USE_CONTROL) {
-monitor_init_qmp(chr, flags);
-} else {
-monitor_init_hmp(chr, flags);
-}
-}
-
 void monitor_cleanup(void)
 {
 /*
diff --git a/monitor/qmp.c b/monitor/qmp.c
index 3abc718ca3..7258f2b088 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -363,18 +363,15 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
 monitor_list_append(>common);
 }
 
-void monitor_init_qmp(Chardev *chr, int flags)
+void monitor_init_qmp(Chardev *chr, bool pretty)
 {
 MonitorQMP *mon = g_malloc0(sizeof(*mon));
 
-/* Only HMP supports readline */
-assert(!(flags & MONITOR_USE_READLINE));
-
 /* Note: we run QMP monitor in I/O thread when @chr supports that */
 monitor_data_init(>common, true, false,
   qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT));
 
-mon->pretty = flags & MONITOR_USE_PRETTY;
+mon->pretty = pretty;
 
 qemu_mutex_init(>qmp_queue_lock);
 mon->qmp_requests = g_queue_new();
diff --git 

[Qemu-block] [PATCH v3 12/15] monitor: Split out monitor/monitor.c

[Kevin Wolf]

Move the monitor core infrastructure from monitor/misc.c to
monitor/monitor.c. This is code that can be shared for all targets, so
compile it only once.

What remains in monitor/misc.c after this patch is mostly monitor
command implementations (which could move to hmp-cmds.c or qmp-cmds.c
later) and code that requires a system emulator or is even
target-dependent (including HMP command completion code).

The amount of function and particularly extern variables in
monitor_int.h is probably a bit larger than it needs to be, but this way
no non-trivial code modifications are needed. The interfaces between all
monitor parts can be cleaned up later.

Signed-off-by: Kevin Wolf 
---
 include/monitor/monitor.h  |   1 +
 monitor/monitor-internal.h |   1 +
 monitor/misc.c | 599 +-
 monitor/monitor.c  | 637 +
 MAINTAINERS|   2 +
 monitor/Makefile.objs  |   2 +-
 monitor/trace-events   |   2 +-
 7 files changed, 644 insertions(+), 600 deletions(-)
 create mode 100644 monitor/monitor.c

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index efdea83bb3..af5ddbe785 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -19,6 +19,7 @@ typedef struct MonitorHMP MonitorHMP;
 bool monitor_cur_is_qmp(void);
 
 void monitor_init_globals(void);
+void monitor_init_globals_core(void);
 void monitor_init(Chardev *chr, int flags);
 void monitor_cleanup(void);
 
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index ad3b7ee7ec..30679d522e 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -171,6 +171,7 @@ void monitor_init_hmp(Chardev *chr, int flags);
 int monitor_puts(Monitor *mon, const char *str);
 void monitor_data_init(Monitor *mon, int flags, bool skip_flush,
bool use_io_thread);
+void monitor_data_destroy(Monitor *mon);
 int monitor_can_read(void *opaque);
 void monitor_list_append(Monitor *mon);
 void monitor_fdsets_cleanup(void);
diff --git a/monitor/misc.c b/monitor/misc.c
index c8289959c0..dddbddb21f 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -64,7 +64,6 @@
 #include "qapi/qmp/json-parser.h"
 #include "qapi/qmp/qlist.h"
 #include "qom/object_interfaces.h"
-#include "trace.h"
 #include "trace/control.h"
 #include "monitor/hmp-target.h"
 #ifdef CONFIG_TRACE_SIMPLE
@@ -119,429 +118,15 @@ struct MonFdset {
 QLIST_ENTRY(MonFdset) next;
 };
 
-/*
- * To prevent flooding clients, events can be throttled. The
- * throttling is calculated globally, rather than per-Monitor
- * instance.
- */
-typedef struct MonitorQAPIEventState {
-QAPIEvent event;/* Throttling state for this event type and... */
-QDict *data;/* ... data, see qapi_event_throttle_equal() */
-QEMUTimer *timer;   /* Timer for handling delayed events */
-QDict *qdict;   /* Delayed event (if any) */
-} MonitorQAPIEventState;
-
-typedef struct {
-int64_t rate;   /* Minimum time (in ns) between two events */
-} MonitorQAPIEventConf;
-
-/* Shared monitor I/O thread */
-IOThread *mon_iothread;
-
-/* Bottom half to dispatch the requests received from I/O thread */
-QEMUBH *qmp_dispatcher_bh;
-
 /* QMP checker flags */
 #define QMP_ACCEPT_UNKNOWNS 1
 
-/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed.  */
-QemuMutex monitor_lock;
-static GHashTable *monitor_qapi_event_state;
-MonitorList mon_list;
-static bool monitor_destroyed;
-
 /* Protects mon_fdsets */
 static QemuMutex mon_fdsets_lock;
 static QLIST_HEAD(, MonFdset) mon_fdsets;
 
-int mon_refcount;
-
 static HMPCommand hmp_info_cmds[];
 
-__thread Monitor *cur_mon;
-
-/**
- * Is @mon is using readline?
- * Note: not all HMP monitors use readline, e.g., gdbserver has a
- * non-interactive HMP monitor, so readline is not used there.
- */
-static inline bool monitor_uses_readline(const Monitor *mon)
-{
-return mon->flags & MONITOR_USE_READLINE;
-}
-
-static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
-{
-return !monitor_is_qmp(mon) && !monitor_uses_readline(mon);
-}
-
-/*
- * Return the clock to use for recording an event's time.
- * It's QEMU_CLOCK_REALTIME, except for qtests it's
- * QEMU_CLOCK_VIRTUAL, to support testing rate limits.
- * Beware: result is invalid before configure_accelerator().
- */
-static inline QEMUClockType monitor_get_event_clock(void)
-{
-return qtest_enabled() ? QEMU_CLOCK_VIRTUAL : QEMU_CLOCK_REALTIME;
-}
-
-/**
- * Is the current monitor, if any, a QMP monitor?
- */
-bool monitor_cur_is_qmp(void)
-{
-return cur_mon && monitor_is_qmp(cur_mon);
-}
-
-static void monitor_flush_locked(Monitor *mon);
-
-static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
-  void *opaque)
-{
-Monitor *mon = opaque;
-
-qemu_mutex_lock(>mon_lock);
-mon->out_watch = 0;
-monitor_flush_locked(mon);
-qemu_mutex_unlock(>mon_lock);
-

[Qemu-block] [PATCH v3 13/15] monitor: Split Monitor.flags into separate bools

[Kevin Wolf]

Monitor.flags contains three different flags: One to distinguish HMP
from QMP; one specific to HMP (MONITOR_USE_READLINE) that is ignored
with QMP; and another one specific to QMP (MONITOR_USE_PRETTY) that is
ignored with HMP.

Split the flags field into three bools and move them to the right
subclass. Flags are still in use for the monitor_init() interface.

Signed-off-by: Kevin Wolf 
---
 monitor/monitor-internal.h |  8 +---
 monitor/hmp.c  |  6 +++---
 monitor/misc.c |  2 +-
 monitor/monitor.c  | 14 +-
 monitor/qmp.c  |  7 ---
 5 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 30679d522e..260725e51b 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -90,8 +90,8 @@ typedef struct HMPCommand {
 struct Monitor {
 CharBackend chr;
 int reset_seen;
-int flags;
 int suspend_cnt;/* Needs to be accessed atomically */
+bool is_qmp;
 bool skip_flush;
 bool use_io_thread;
 
@@ -116,6 +116,7 @@ struct Monitor {
 
 struct MonitorHMP {
 Monitor common;
+bool use_readline;
 /*
  * State used only in the thread "owning" the monitor.
  * If @use_io_thread, this is @mon_iothread. (This does not actually happen
@@ -129,6 +130,7 @@ struct MonitorHMP {
 typedef struct {
 Monitor common;
 JSONMessageParser parser;
+bool pretty;
 /*
  * When a client connects, we're in capabilities negotiation mode.
  * @commands is _cap_negotiation_commands then.  When command
@@ -152,7 +154,7 @@ typedef struct {
  */
 static inline bool monitor_is_qmp(const Monitor *mon)
 {
-return (mon->flags & MONITOR_USE_CONTROL);
+return mon->is_qmp;
 }
 
 typedef QTAILQ_HEAD(MonitorList, Monitor) MonitorList;
@@ -169,7 +171,7 @@ void monitor_init_qmp(Chardev *chr, int flags);
 void monitor_init_hmp(Chardev *chr, int flags);
 
 int monitor_puts(Monitor *mon, const char *str);
-void monitor_data_init(Monitor *mon, int flags, bool skip_flush,
+void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
bool use_io_thread);
 void monitor_data_destroy(Monitor *mon);
 int monitor_can_read(void *opaque);
diff --git a/monitor/hmp.c b/monitor/hmp.c
index 3621b195ed..5ba8b6e8d5 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -1396,12 +1396,12 @@ static void monitor_readline_flush(void *opaque)
 void monitor_init_hmp(Chardev *chr, int flags)
 {
 MonitorHMP *mon = g_malloc0(sizeof(*mon));
-bool use_readline = flags & MONITOR_USE_READLINE;
 
-monitor_data_init(>common, flags, false, false);
+monitor_data_init(>common, false, false, false);
 qemu_chr_fe_init(>common.chr, chr, _abort);
 
-if (use_readline) {
+mon->use_readline = flags & MONITOR_USE_READLINE;
+if (mon->use_readline) {
 mon->rs = readline_init(monitor_readline_printf,
 monitor_readline_flush,
 mon,
diff --git a/monitor/misc.c b/monitor/misc.c
index dddbddb21f..10c056394e 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -134,7 +134,7 @@ char *qmp_human_monitor_command(const char *command_line, 
bool has_cpu_index,
 Monitor *old_mon;
 MonitorHMP hmp = {};
 
-monitor_data_init(, 0, true, false);
+monitor_data_init(, false, true, false);
 
 old_mon = cur_mon;
 cur_mon = 
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 6802b8e491..7325e4362b 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -80,14 +80,18 @@ bool monitor_cur_is_qmp(void)
  * Note: not all HMP monitors use readline, e.g., gdbserver has a
  * non-interactive HMP monitor, so readline is not used there.
  */
-static inline bool monitor_uses_readline(const Monitor *mon)
+static inline bool monitor_uses_readline(const MonitorHMP *mon)
 {
-return mon->flags & MONITOR_USE_READLINE;
+return mon->use_readline;
 }
 
 static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
 {
-return !monitor_is_qmp(mon) && !monitor_uses_readline(mon);
+if (monitor_is_qmp(mon)) {
+return false;
+}
+
+return !monitor_uses_readline(container_of(mon, MonitorHMP, common));
 }
 
 static void monitor_flush_locked(Monitor *mon);
@@ -523,17 +527,17 @@ static void monitor_iothread_init(void)
 mon_iothread = iothread_create("mon_iothread", _abort);
 }
 
-void monitor_data_init(Monitor *mon, int flags, bool skip_flush,
+void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
bool use_io_thread)
 {
 if (use_io_thread && !mon_iothread) {
 monitor_iothread_init();
 }
 qemu_mutex_init(>mon_lock);
+mon->is_qmp = is_qmp;
 mon->outbuf = qstring_new();
 mon->skip_flush = skip_flush;
 mon->use_io_thread = use_io_thread;
-mon->flags = flags;
 }
 
 void monitor_data_destroy(Monitor *mon)
diff --git a/monitor/qmp.c b/monitor/qmp.c
index 

[Qemu-block] [PATCH v3 09/15] monitor: Create monitor-internal.h with common definitions

[Kevin Wolf]

Before we can split monitor/misc.c, we need to create a header file that
contains the common definitions that will be used by multiple source
files.

For a start, add the type definitions for Monitor, MonitorHMP and
MonitorQMP and their dependencies. We'll add functions as needed when
splitting monitor/misc.c.

Signed-off-by: Kevin Wolf 
Reviewed-by: Dr. David Alan Gilbert 
---
 monitor/monitor-internal.h | 148 +
 monitor/misc.c | 110 +--
 MAINTAINERS|   2 +
 3 files changed, 151 insertions(+), 109 deletions(-)
 create mode 100644 monitor/monitor-internal.h

diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
new file mode 100644
index 00..17a632b0ad
--- /dev/null
+++ b/monitor/monitor-internal.h
@@ -0,0 +1,148 @@
+/*
+ * QEMU monitor
+ *
+ * Copyright (c) 2003-2004 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef MONITOR_INT_H
+#define MONITOR_INT_H
+
+#include "monitor/monitor.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/json-parser.h"
+#include "qapi/qmp/dispatch.h"
+#include "qapi/qapi-types-misc.h"
+
+#include "qemu/readline.h"
+#include "chardev/char-fe.h"
+#include "sysemu/iothread.h"
+
+/*
+ * Supported types:
+ *
+ * 'F'  filename
+ * 'B'  block device name
+ * 's'  string (accept optional quote)
+ * 'S'  it just appends the rest of the string (accept optional quote)
+ * 'O'  option string of the form NAME=VALUE,...
+ *  parsed according to QemuOptsList given by its name
+ *  Example: 'device:O' uses qemu_device_opts.
+ *  Restriction: only lists with empty desc are supported
+ *  TODO lift the restriction
+ * 'i'  32 bit integer
+ * 'l'  target long (32 or 64 bit)
+ * 'M'  Non-negative target long (32 or 64 bit), in user mode the
+ *  value is multiplied by 2^20 (think Mebibyte)
+ * 'o'  octets (aka bytes)
+ *  user mode accepts an optional E, e, P, p, T, t, G, g, M, m,
+ *  K, k suffix, which multiplies the value by 2^60 for suffixes E
+ *  and e, 2^50 for suffixes P and p, 2^40 for suffixes T and t,
+ *  2^30 for suffixes G and g, 2^20 for M and m, 2^10 for K and k
+ * 'T'  double
+ *  user mode accepts an optional ms, us, ns suffix,
+ *  which divides the value by 1e3, 1e6, 1e9, respectively
+ * '/'  optional gdb-like print format (like "/10x")
+ *
+ * '?'  optional type (for all types, except '/')
+ * '.'  other form of optional type (for 'i' and 'l')
+ * 'b'  boolean
+ *  user mode accepts "on" or "off"
+ * '-'  optional parameter (eg. '-f')
+ *
+ */
+
+typedef struct HMPCommand {
+const char *name;
+const char *args_type;
+const char *params;
+const char *help;
+const char *flags; /* p=preconfig */
+void (*cmd)(Monitor *mon, const QDict *qdict);
+/*
+ * @sub_table is a list of 2nd level of commands. If it does not exist,
+ * cmd should be used. If it exists, sub_table[?].cmd should be
+ * used, and cmd of 1st level plays the role of help function.
+ */
+struct HMPCommand *sub_table;
+void (*command_completion)(ReadLineState *rs, int nb_args, const char 
*str);
+} HMPCommand;
+
+struct Monitor {
+CharBackend chr;
+int reset_seen;
+int flags;
+int suspend_cnt;/* Needs to be accessed atomically */
+bool skip_flush;
+bool use_io_thread;
+
+gchar *mon_cpu_path;
+QTAILQ_ENTRY(Monitor) entry;
+
+/*
+ * The per-monitor lock. We can't access guest memory when holding
+ * the lock.
+ */
+QemuMutex mon_lock;
+
+/*
+ * Members that are protected by the per-monitor lock
+ */
+QLIST_HEAD(, mon_fd_t) fds;
+QString *outbuf;
+guint 

[Qemu-block] [PATCH v3 03/15] monitor: Make MonitorQMP a child class of Monitor

[Kevin Wolf]

Currently, struct Monitor mixes state that is only relevant for HMP,
state that is only relevant for QMP, and some actually shared state.
In particular, a MonitorQMP field is present in the state of any
monitor, even if it's not a QMP monitor and therefore doesn't use the
state.

As a first step towards a clean separation between QMP and HMP, let
MonitorQMP extend Monitor and create a MonitorQMP object only when the
monitor is actually a QMP monitor.

Some places accessed Monitor.qmp unconditionally, even for HMP monitors.
They can't keep doing this now, so during the conversion, they are
either changed to become conditional on monitor_is_qmp() or to assert()
that they always get a QMP monitor.

Signed-off-by: Kevin Wolf 
Reviewed-by: Dr. David Alan Gilbert 
---
 monitor.c | 220 ++
 1 file changed, 124 insertions(+), 96 deletions(-)

diff --git a/monitor.c b/monitor.c
index a70c1283b1..855a147723 100644
--- a/monitor.c
+++ b/monitor.c
@@ -168,26 +168,6 @@ struct MonFdset {
 QLIST_ENTRY(MonFdset) next;
 };
 
-typedef struct {
-JSONMessageParser parser;
-/*
- * When a client connects, we're in capabilities negotiation mode.
- * @commands is _cap_negotiation_commands then.  When command
- * qmp_capabilities succeeds, we go into command mode, and
- * @command becomes _commands.
- */
-QmpCommandList *commands;
-bool capab_offered[QMP_CAPABILITY__MAX]; /* capabilities offered */
-bool capab[QMP_CAPABILITY__MAX]; /* offered and accepted */
-/*
- * Protects qmp request/response queue.
- * Take monitor_lock first when you need both.
- */
-QemuMutex qmp_queue_lock;
-/* Input queue that holds all the parsed QMP requests */
-GQueue *qmp_requests;
-} MonitorQMP;
-
 /*
  * To prevent flooding clients, events can be throttled. The
  * throttling is calculated globally, rather than per-Monitor
@@ -220,7 +200,6 @@ struct Monitor {
  */
 ReadLineState *rs;
 
-MonitorQMP qmp;
 gchar *mon_cpu_path;
 mon_cmd_t *cmd_table;
 QTAILQ_ENTRY(Monitor) entry;
@@ -241,6 +220,27 @@ struct Monitor {
 int mux_out;
 };
 
+typedef struct {
+Monitor common;
+JSONMessageParser parser;
+/*
+ * When a client connects, we're in capabilities negotiation mode.
+ * @commands is _cap_negotiation_commands then.  When command
+ * qmp_capabilities succeeds, we go into command mode, and
+ * @command becomes _commands.
+ */
+QmpCommandList *commands;
+bool capab_offered[QMP_CAPABILITY__MAX]; /* capabilities offered */
+bool capab[QMP_CAPABILITY__MAX]; /* offered and accepted */
+/*
+ * Protects qmp request/response queue.
+ * Take monitor_lock first when you need both.
+ */
+QemuMutex qmp_queue_lock;
+/* Input queue that holds all the parsed QMP requests */
+GQueue *qmp_requests;
+} MonitorQMP;
+
 /* Shared monitor I/O thread */
 IOThread *mon_iothread;
 
@@ -249,7 +249,7 @@ QEMUBH *qmp_dispatcher_bh;
 
 struct QMPRequest {
 /* Owner of the request */
-Monitor *mon;
+MonitorQMP *mon;
 /*
  * Request object to be handled or Error to be reported
  * (exactly one of them is non-null)
@@ -357,18 +357,18 @@ static void qmp_request_free(QMPRequest *req)
 }
 
 /* Caller must hold mon->qmp.qmp_queue_lock */
-static void monitor_qmp_cleanup_req_queue_locked(Monitor *mon)
+static void monitor_qmp_cleanup_req_queue_locked(MonitorQMP *mon)
 {
-while (!g_queue_is_empty(mon->qmp.qmp_requests)) {
-qmp_request_free(g_queue_pop_head(mon->qmp.qmp_requests));
+while (!g_queue_is_empty(mon->qmp_requests)) {
+qmp_request_free(g_queue_pop_head(mon->qmp_requests));
 }
 }
 
-static void monitor_qmp_cleanup_queues(Monitor *mon)
+static void monitor_qmp_cleanup_queues(MonitorQMP *mon)
 {
-qemu_mutex_lock(>qmp.qmp_queue_lock);
+qemu_mutex_lock(>qmp_queue_lock);
 monitor_qmp_cleanup_req_queue_locked(mon);
-qemu_mutex_unlock(>qmp.qmp_queue_lock);
+qemu_mutex_unlock(>qmp_queue_lock);
 }
 
 
@@ -480,17 +480,17 @@ int monitor_printf(Monitor *mon, const char *fmt, ...)
 return ret;
 }
 
-static void qmp_send_response(Monitor *mon, const QDict *rsp)
+static void qmp_send_response(MonitorQMP *mon, const QDict *rsp)
 {
 const QObject *data = QOBJECT(rsp);
 QString *json;
 
-json = mon->flags & MONITOR_USE_PRETTY ? qobject_to_json_pretty(data) :
- qobject_to_json(data);
+json = mon->common.flags & MONITOR_USE_PRETTY ?
+   qobject_to_json_pretty(data) : qobject_to_json(data);
 assert(json != NULL);
 
 qstring_append_chr(json, '\n');
-monitor_puts(mon, qstring_get_str(json));
+monitor_puts(>common, qstring_get_str(json));
 
 qobject_unref(json);
 }
@@ -513,12 +513,17 @@ static MonitorQAPIEventConf 
monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
 static void monitor_qapi_event_emit(QAPIEvent event, 

[Qemu-block] [PATCH v3 04/15] monitor: Create MonitorHMP with readline state

[Kevin Wolf]

The ReadLineState in Monitor is only used for HMP monitors. Create
MonitorHMP and move it there.

Signed-off-by: Kevin Wolf 
Reviewed-by: Dr. David Alan Gilbert 
---
 include/monitor/monitor.h |   5 +-
 hmp.c |   4 +-
 monitor.c | 129 +-
 3 files changed, 79 insertions(+), 59 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 06cfcd8f36..efdea83bb3 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -6,6 +6,7 @@
 #include "qemu/readline.h"
 
 extern __thread Monitor *cur_mon;
+typedef struct MonitorHMP MonitorHMP;
 
 /* flags for monitor_init */
 /* 0x01 unused */
@@ -34,8 +35,8 @@ void monitor_flush(Monitor *mon);
 int monitor_set_cpu(int cpu_index);
 int monitor_get_cpu_index(void);
 
-void monitor_read_command(Monitor *mon, int show_prompt);
-int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
+void monitor_read_command(MonitorHMP *mon, int show_prompt);
+int monitor_read_password(MonitorHMP *mon, ReadLineFunc *readline_func,
   void *opaque);
 
 AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
diff --git a/hmp.c b/hmp.c
index be5e345c6f..99414cd39c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1943,6 +1943,8 @@ static void hmp_change_read_arg(void *opaque, const char 
*password,
 
 void hmp_change(Monitor *mon, const QDict *qdict)
 {
+/* FIXME Make MonitorHMP public and use container_of */
+MonitorHMP *hmp_mon = (MonitorHMP *) mon;
 const char *device = qdict_get_str(qdict, "device");
 const char *target = qdict_get_str(qdict, "target");
 const char *arg = qdict_get_try_str(qdict, "arg");
@@ -1960,7 +1962,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
 if (strcmp(target, "passwd") == 0 ||
 strcmp(target, "password") == 0) {
 if (!arg) {
-monitor_read_password(mon, hmp_change_read_arg, NULL);
+monitor_read_password(hmp_mon, hmp_change_read_arg, NULL);
 return;
 }
 }
diff --git a/monitor.c b/monitor.c
index 855a147723..572449f6db 100644
--- a/monitor.c
+++ b/monitor.c
@@ -192,14 +192,6 @@ struct Monitor {
 bool skip_flush;
 bool use_io_thread;
 
-/*
- * State used only in the thread "owning" the monitor.
- * If @use_io_thread, this is @mon_iothread.
- * Else, it's the main thread.
- * These members can be safely accessed without locks.
- */
-ReadLineState *rs;
-
 gchar *mon_cpu_path;
 mon_cmd_t *cmd_table;
 QTAILQ_ENTRY(Monitor) entry;
@@ -220,6 +212,18 @@ struct Monitor {
 int mux_out;
 };
 
+struct MonitorHMP {
+Monitor common;
+/*
+ * State used only in the thread "owning" the monitor.
+ * If @use_io_thread, this is @mon_iothread. (This does not actually happen
+ * in the current state of the code.)
+ * Else, it's the main thread.
+ * These members can be safely accessed without locks.
+ */
+ReadLineState *rs;
+};
+
 typedef struct {
 Monitor common;
 JSONMessageParser parser;
@@ -326,7 +330,7 @@ bool monitor_cur_is_qmp(void)
 return cur_mon && monitor_is_qmp(cur_mon);
 }
 
-void monitor_read_command(Monitor *mon, int show_prompt)
+void monitor_read_command(MonitorHMP *mon, int show_prompt)
 {
 if (!mon->rs)
 return;
@@ -336,7 +340,7 @@ void monitor_read_command(Monitor *mon, int show_prompt)
 readline_show_prompt(mon->rs);
 }
 
-int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
+int monitor_read_password(MonitorHMP *mon, ReadLineFunc *readline_func,
   void *opaque)
 {
 if (mon->rs) {
@@ -344,7 +348,8 @@ int monitor_read_password(Monitor *mon, ReadLineFunc 
*readline_func,
 /* prompt is printed on return from the command handler */
 return 0;
 } else {
-monitor_printf(mon, "terminal does not support password prompting\n");
+monitor_printf(>common,
+   "terminal does not support password prompting\n");
 return -ENOTTY;
 }
 }
@@ -705,7 +710,7 @@ static void monitor_qapi_event_init(void)
 qapi_event_throttle_equal);
 }
 
-static void handle_hmp_command(Monitor *mon, const char *cmdline);
+static void handle_hmp_command(MonitorHMP *mon, const char *cmdline);
 
 static void monitor_iothread_init(void);
 
@@ -715,7 +720,6 @@ static void monitor_data_init(Monitor *mon, int flags, bool 
skip_flush,
 if (use_io_thread && !mon_iothread) {
 monitor_iothread_init();
 }
-memset(mon, 0, sizeof(Monitor));
 qemu_mutex_init(>mon_lock);
 mon->outbuf = qstring_new();
 /* Use *mon_cmds by default. */
@@ -740,8 +744,10 @@ static void monitor_data_destroy(Monitor *mon)
 if (monitor_is_qmp(mon)) {
 MonitorQMP *qmp_mon = container_of(mon, MonitorQMP, common);
 

[Qemu-block] [PATCH v3 11/15] monitor: Split out monitor/hmp.c

[Kevin Wolf]

Move HMP infrastructure from monitor/misc.c to monitor/hmp.c. This is
code that can be shared for all targets, so compile it only once.

The amount of function and particularly extern variables in
monitor_int.h is probably a bit larger than it needs to be, but this way
no non-trivial code modifications are needed. The interfaces between HMP
and the monitor core can be cleaned up later.

Signed-off-by: Kevin Wolf 
---
 monitor/monitor-internal.h |   10 +
 monitor/hmp.c  | 1415 
 monitor/misc.c | 1360 +-
 monitor/Makefile.objs  |2 +-
 monitor/trace-events   |4 +-
 5 files changed, 1443 insertions(+), 1348 deletions(-)
 create mode 100644 monitor/hmp.c

diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 0a26f702dd..ad3b7ee7ec 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -26,6 +26,8 @@
 #define MONITOR_INT_H
 
 #include "monitor/monitor.h"
+#include "qemu/cutils.h"
+
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/json-parser.h"
 #include "qapi/qmp/dispatch.h"
@@ -161,7 +163,10 @@ extern QemuMutex monitor_lock;
 extern MonitorList mon_list;
 extern int mon_refcount;
 
+extern HMPCommand hmp_cmds[];
+
 void monitor_init_qmp(Chardev *chr, int flags);
+void monitor_init_hmp(Chardev *chr, int flags);
 
 int monitor_puts(Monitor *mon, const char *str);
 void monitor_data_init(Monitor *mon, int flags, bool skip_flush,
@@ -174,4 +179,9 @@ void qmp_send_response(MonitorQMP *mon, const QDict *rsp);
 void monitor_data_destroy_qmp(MonitorQMP *mon);
 void monitor_qmp_bh_dispatcher(void *data);
 
+int get_monitor_def(int64_t *pval, const char *name);
+void help_cmd(Monitor *mon, const char *name);
+void handle_hmp_command(MonitorHMP *mon, const char *cmdline);
+int hmp_compare_cmd(const char *name, const char *list);
+
 #endif
diff --git a/monitor/hmp.c b/monitor/hmp.c
new file mode 100644
index 00..3621b195ed
--- /dev/null
+++ b/monitor/hmp.c
@@ -0,0 +1,1415 @@
+/*
+ * QEMU monitor
+ *
+ * Copyright (c) 2003-2004 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "monitor-internal.h"
+
+#include "qapi/error.h"
+#include "qapi/qmp/qnum.h"
+
+#include "qemu/config-file.h"
+#include "qemu/ctype.h"
+#include "qemu/log.h"
+#include "qemu/option.h"
+#include "qemu/units.h"
+#include "sysemu/block-backend.h"
+#include "sysemu/sysemu.h"
+
+#include "trace.h"
+
+static void monitor_command_cb(void *opaque, const char *cmdline,
+   void *readline_opaque)
+{
+MonitorHMP *mon = opaque;
+
+monitor_suspend(>common);
+handle_hmp_command(mon, cmdline);
+monitor_resume(>common);
+}
+
+void monitor_read_command(MonitorHMP *mon, int show_prompt)
+{
+if (!mon->rs) {
+return;
+}
+
+readline_start(mon->rs, "(qemu) ", 0, monitor_command_cb, NULL);
+if (show_prompt) {
+readline_show_prompt(mon->rs);
+}
+}
+
+int monitor_read_password(MonitorHMP *mon, ReadLineFunc *readline_func,
+  void *opaque)
+{
+if (mon->rs) {
+readline_start(mon->rs, "Password: ", 1, readline_func, opaque);
+/* prompt is printed on return from the command handler */
+return 0;
+} else {
+monitor_printf(>common,
+   "terminal does not support password prompting\n");
+return -ENOTTY;
+}
+}
+
+static int get_str(char *buf, int buf_size, const char **pp)
+{
+const char *p;
+char *q;
+int c;
+
+q = buf;
+p = *pp;
+while (qemu_isspace(*p)) {
+p++;
+}
+if (*p == '\0') {
+fail:
+*q = '\0';
+*pp = p;
+return -1;
+}
+if (*p == '\"') {
+p++;
+while (*p != '\0' && *p != '\"') {
+if (*p == '\\') {
+p++;
+c = *p++;
+

[Qemu-block] [PATCH v3 06/15] monitor: Rename HMP command type and tables

[Kevin Wolf]

This renames the type for HMP monitor commands and the tables holding
the commands to make clear that they are related to HMP and to allow
making them public later:

* mon_cmd_t -> HMPCommand (fixing use of a reserved name, too)
* mon_cmds -> hmp_cmds
* info_cmds -> hmp_info_cmds

Signed-off-by: Kevin Wolf 
---
 monitor.c   | 68 -
 hmp-commands.hx |  2 +-
 2 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/monitor.c b/monitor.c
index 5eacaa48a6..006c650761 100644
--- a/monitor.c
+++ b/monitor.c
@@ -127,7 +127,7 @@
  *
  */
 
-typedef struct mon_cmd_t {
+typedef struct HMPCommand {
 const char *name;
 const char *args_type;
 const char *params;
@@ -138,9 +138,9 @@ typedef struct mon_cmd_t {
  * cmd should be used. If it exists, sub_table[?].cmd should be
  * used, and cmd of 1st level plays the role of help function.
  */
-struct mon_cmd_t *sub_table;
+struct HMPCommand *sub_table;
 void (*command_completion)(ReadLineState *rs, int nb_args, const char 
*str);
-} mon_cmd_t;
+} HMPCommand;
 
 /* file descriptors passed via SCM_RIGHTS */
 typedef struct mon_fd_t mon_fd_t;
@@ -277,8 +277,8 @@ static QLIST_HEAD(, MonFdset) mon_fdsets;
 
 static int mon_refcount;
 
-static mon_cmd_t mon_cmds[];
-static mon_cmd_t info_cmds[];
+static HMPCommand hmp_cmds[];
+static HMPCommand hmp_info_cmds[];
 
 QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
 
@@ -935,7 +935,7 @@ static int parse_cmdline(const char *cmdline,
 /*
  * Can command @cmd be executed in preconfig state?
  */
-static bool cmd_can_preconfig(const mon_cmd_t *cmd)
+static bool cmd_can_preconfig(const HMPCommand *cmd)
 {
 if (!cmd->flags) {
 return false;
@@ -945,7 +945,7 @@ static bool cmd_can_preconfig(const mon_cmd_t *cmd)
 }
 
 static void help_cmd_dump_one(Monitor *mon,
-  const mon_cmd_t *cmd,
+  const HMPCommand *cmd,
   char **prefix_args,
   int prefix_args_nb)
 {
@@ -962,10 +962,10 @@ static void help_cmd_dump_one(Monitor *mon,
 }
 
 /* @args[@arg_index] is the valid command need to find in @cmds */
-static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
+static void help_cmd_dump(Monitor *mon, const HMPCommand *cmds,
   char **args, int nb_args, int arg_index)
 {
-const mon_cmd_t *cmd;
+const HMPCommand *cmd;
 size_t i;
 
 /* No valid arg need to compare with, dump all in *cmds */
@@ -1023,7 +1023,7 @@ static void help_cmd(Monitor *mon, const char *name)
 }
 
 /* 2. dump the contents according to parsed args */
-help_cmd_dump(mon, mon_cmds, args, nb_args, 0);
+help_cmd_dump(mon, hmp_cmds, args, nb_args, 0);
 
 free_cmdline_args(args, nb_args);
 }
@@ -2691,13 +2691,13 @@ int monitor_fd_param(Monitor *mon, const char *fdname, 
Error **errp)
 }
 
 /* Please update hmp-commands.hx when adding or changing commands */
-static mon_cmd_t info_cmds[] = {
+static HMPCommand hmp_info_cmds[] = {
 #include "hmp-commands-info.h"
 { NULL, NULL, },
 };
 
-/* mon_cmds and info_cmds would be sorted at runtime */
-static mon_cmd_t mon_cmds[] = {
+/* hmp_cmds and hmp_info_cmds would be sorted at runtime */
+static HMPCommand hmp_cmds[] = {
 #include "hmp-commands.h"
 { NULL, NULL, },
 };
@@ -3039,10 +3039,10 @@ static int is_valid_option(const char *c, const char 
*typestr)
 return (typestr != NULL);
 }
 
-static const mon_cmd_t *search_dispatch_table(const mon_cmd_t *disp_table,
-  const char *cmdname)
+static const HMPCommand *search_dispatch_table(const HMPCommand *disp_table,
+   const char *cmdname)
 {
-const mon_cmd_t *cmd;
+const HMPCommand *cmd;
 
 for (cmd = disp_table; cmd->name != NULL; cmd++) {
 if (compare_cmd(cmdname, cmd->name)) {
@@ -3063,14 +3063,14 @@ static const mon_cmd_t *search_dispatch_table(const 
mon_cmd_t *disp_table,
  * Do not assume the return value points into @table!  It doesn't when
  * the command is found in a sub-command table.
  */
-static const mon_cmd_t *monitor_parse_command(MonitorHMP *hmp_mon,
-  const char *cmdp_start,
-  const char **cmdp,
-  mon_cmd_t *table)
+static const HMPCommand *monitor_parse_command(MonitorHMP *hmp_mon,
+   const char *cmdp_start,
+   const char **cmdp,
+   HMPCommand *table)
 {
 Monitor *mon = _mon->common;
 const char *p;
-const mon_cmd_t *cmd;
+const HMPCommand *cmd;
 char cmdname[256];
 
 /* extract the command name */
@@ -3114,7 +3114,7 @@ static const mon_cmd_t *monitor_parse_command(MonitorHMP 

[Qemu-block] [PATCH v3 10/15] monitor: Split out monitor/qmp.c

[Kevin Wolf]

Move QMP infrastructure from monitor/misc.c to monitor/qmp.c. This is
code that can be shared for all targets, so compile it only once.

The amount of function and particularly extern variables in
monitor_int.h is probably a bit larger than it needs to be, but this way
no non-trivial code modifications are needed. The interfaces between QMP
and the monitor core can be cleaned up later.

Signed-off-by: Kevin Wolf 
Reviewed-by: Dr. David Alan Gilbert 
---
 monitor/monitor-internal.h |  29 +++
 monitor/misc.c | 397 +---
 monitor/qmp.c  | 405 +
 Makefile.objs  |   1 +
 monitor/Makefile.objs  |   1 +
 monitor/trace-events   |   4 +-
 6 files changed, 448 insertions(+), 389 deletions(-)
 create mode 100644 monitor/qmp.c

diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 17a632b0ad..0a26f702dd 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -145,4 +145,33 @@ typedef struct {
 GQueue *qmp_requests;
 } MonitorQMP;
 
+/**
+ * Is @mon a QMP monitor?
+ */
+static inline bool monitor_is_qmp(const Monitor *mon)
+{
+return (mon->flags & MONITOR_USE_CONTROL);
+}
+
+typedef QTAILQ_HEAD(MonitorList, Monitor) MonitorList;
+extern IOThread *mon_iothread;
+extern QEMUBH *qmp_dispatcher_bh;
+extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
+extern QemuMutex monitor_lock;
+extern MonitorList mon_list;
+extern int mon_refcount;
+
+void monitor_init_qmp(Chardev *chr, int flags);
+
+int monitor_puts(Monitor *mon, const char *str);
+void monitor_data_init(Monitor *mon, int flags, bool skip_flush,
+   bool use_io_thread);
+int monitor_can_read(void *opaque);
+void monitor_list_append(Monitor *mon);
+void monitor_fdsets_cleanup(void);
+
+void qmp_send_response(MonitorQMP *mon, const QDict *rsp);
+void monitor_data_destroy_qmp(MonitorQMP *mon);
+void monitor_qmp_bh_dispatcher(void *data);
+
 #endif
diff --git a/monitor/misc.c b/monitor/misc.c
index 6aa4a21676..368b8297d4 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -142,51 +142,29 @@ IOThread *mon_iothread;
 /* Bottom half to dispatch the requests received from I/O thread */
 QEMUBH *qmp_dispatcher_bh;
 
-struct QMPRequest {
-/* Owner of the request */
-MonitorQMP *mon;
-/*
- * Request object to be handled or Error to be reported
- * (exactly one of them is non-null)
- */
-QObject *req;
-Error *err;
-};
-typedef struct QMPRequest QMPRequest;
-
 /* QMP checker flags */
 #define QMP_ACCEPT_UNKNOWNS 1
 
 /* Protects mon_list, monitor_qapi_event_state, monitor_destroyed.  */
-static QemuMutex monitor_lock;
+QemuMutex monitor_lock;
 static GHashTable *monitor_qapi_event_state;
-static QTAILQ_HEAD(, Monitor) mon_list;
+MonitorList mon_list;
 static bool monitor_destroyed;
 
 /* Protects mon_fdsets */
 static QemuMutex mon_fdsets_lock;
 static QLIST_HEAD(, MonFdset) mon_fdsets;
 
-static int mon_refcount;
+int mon_refcount;
 
 static HMPCommand hmp_cmds[];
 static HMPCommand hmp_info_cmds[];
 
-QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
-
 __thread Monitor *cur_mon;
 
 static void monitor_command_cb(void *opaque, const char *cmdline,
void *readline_opaque);
 
-/**
- * Is @mon a QMP monitor?
- */
-static inline bool monitor_is_qmp(const Monitor *mon)
-{
-return (mon->flags & MONITOR_USE_CONTROL);
-}
-
 /**
  * Is @mon is using readline?
  * Note: not all HMP monitors use readline, e.g., gdbserver has a
@@ -245,28 +223,6 @@ int monitor_read_password(MonitorHMP *mon, ReadLineFunc 
*readline_func,
 }
 }
 
-static void qmp_request_free(QMPRequest *req)
-{
-qobject_unref(req->req);
-error_free(req->err);
-g_free(req);
-}
-
-/* Caller must hold mon->qmp.qmp_queue_lock */
-static void monitor_qmp_cleanup_req_queue_locked(MonitorQMP *mon)
-{
-while (!g_queue_is_empty(mon->qmp_requests)) {
-qmp_request_free(g_queue_pop_head(mon->qmp_requests));
-}
-}
-
-static void monitor_qmp_cleanup_queues(MonitorQMP *mon)
-{
-qemu_mutex_lock(>qmp_queue_lock);
-monitor_qmp_cleanup_req_queue_locked(mon);
-qemu_mutex_unlock(>qmp_queue_lock);
-}
-
 
 static void monitor_flush_locked(Monitor *mon);
 
@@ -326,7 +282,7 @@ void monitor_flush(Monitor *mon)
 }
 
 /* flush at every end of line */
-static int monitor_puts(Monitor *mon, const char *str)
+int monitor_puts(Monitor *mon, const char *str)
 {
 int i;
 char c;
@@ -376,21 +332,6 @@ int monitor_printf(Monitor *mon, const char *fmt, ...)
 return ret;
 }
 
-static void qmp_send_response(MonitorQMP *mon, const QDict *rsp)
-{
-const QObject *data = QOBJECT(rsp);
-QString *json;
-
-json = mon->common.flags & MONITOR_USE_PRETTY ?
-   qobject_to_json_pretty(data) : qobject_to_json(data);
-assert(json != NULL);
-
-qstring_append_chr(json, '\n');
-monitor_puts(>common, qstring_get_str(json));
-
- 

[Qemu-block] [PATCH v3 00/15] monitor: Split monitor.c in core/HMP/QMP/misc

[Kevin Wolf]

monitor.c mixes a lot of different things in a single file: The core
monitor infrastructure, HMP infrastrcture, QMP infrastructure, and the
implementation of several HMP and QMP commands. Almost worse, struct
Monitor mixes state for HMP, for QMP, and state actually shared between
all monitors. monitor.c must be linked with a system emulator and even
requires per-target compilation because some of the commands it
implements access system emulator state.

The reason why I care about this is that I'm working on a protoype for a
storage daemon, which wants to use QMP (but probably not HMP) and
obviously doesn't have any system emulator state. So I'm interested in
some core monitor parts that can be linked to non-system-emulator tools.

This series first creates separate structs MonitorQMP and MonitorHMP
which inherit from Monitor, and then moves the associated infrastructure
code into separate source files.

While the split is probably not perfect, I think it's an improvement of
the current state even for QEMU proper, and it's good enough so I can
link my storage daemon against just monitor/core.o and monitor/qmp.o and
get a useless QMP monitor that parses the JSON input and rejects
everything as an unknown command.

Next I'll try to teach it a subset of QMP commands that can actually be
supported in a tool, but while there will be a few follow-up patches to
achieve this, I don't expect that this work will bring up much that
needs to be changed in the splitting process done in this series.

v3:
- Assert monitor_is_qmp() before casting to MonitorQMP in two places
- Added note that HMP doesn't currently use iothread to the
  documentation of MonitorHMP
- Removed unnecessary memset() in monitor_data_init()
- Removed Monitor.cmd_table instead of moving it to MonitorHMP. Renamed
  the tables to have an hmp_ prefix.
- monitor_int.h of v2 becomes monitor-internal.h now
- Cleaned up #include directives in new files
- Moved some more functions between files
- Removed monitor_init() in favour of public monitor_init_hmp/qmp()
- Deprecate -mon control=readline,pretty=on|off
- Improved several commit messages

v2:
- Fix coding style while moving files to make checkpatch happier
- Updated file name references in docs/devel/writing-qmp-commands.txt
- Updated MAINTAINERS for moved and newly created files
- Created monitor/trace-events instead of using the root directory one
- Move {hmp,qmp}.c to monitor/{hmp,qmp}-cmds.c

Kevin Wolf (15):
  monitor: Remove unused password prompting fields
  monitor: Split monitor_init in HMP and QMP function
  monitor: Make MonitorQMP a child class of Monitor
  monitor: Create MonitorHMP with readline state
  monitor: Remove Monitor.cmd_table indirection
  monitor: Rename HMP command type and tables
  Move monitor.c to monitor/misc.c
  monitor: Move {hmp,qmp}.c to monitor/{hmp,qmp}-cmds.c
  monitor: Create monitor-internal.h with common definitions
  monitor: Split out monitor/qmp.c
  monitor: Split out monitor/hmp.c
  monitor: Split out monitor/monitor.c
  monitor: Split Monitor.flags into separate bools
  monitor: Replace monitor_init() with monitor_init_{hmp,qmp}()
  vl: Deprecate -mon pretty=... for HMP monitors

 docs/devel/writing-qmp-commands.txt |   11 +-
 include/monitor/monitor.h   |   15 +-
 monitor/monitor-internal.h  |  187 ++
 chardev/char.c  |2 +-
 gdbstub.c   |2 +-
 monitor.c   | 4729 ---
 hmp.c => monitor/hmp-cmds.c |6 +-
 monitor/hmp.c   | 1415 
 monitor/misc.c  | 2368 ++
 monitor/monitor.c   |  632 
 qmp.c => monitor/qmp-cmds.c |2 +-
 monitor/qmp.c   |  403 +++
 stubs/monitor.c |6 +-
 tests/test-util-sockets.c   |3 +-
 vl.c|   28 +-
 MAINTAINERS |   13 +-
 Makefile.objs   |4 +-
 Makefile.target |3 +-
 hmp-commands.hx |2 +-
 monitor/Makefile.objs   |3 +
 monitor/trace-events|   15 +
 qemu-deprecated.texi|6 +
 trace-events|   10 -
 23 files changed, 5091 insertions(+), 4774 deletions(-)
 create mode 100644 monitor/monitor-internal.h
 delete mode 100644 monitor.c
 rename hmp.c => monitor/hmp-cmds.c (99%)
 create mode 100644 monitor/hmp.c
 create mode 100644 monitor/misc.c
 create mode 100644 monitor/monitor.c
 rename qmp.c => monitor/qmp-cmds.c (99%)
 create mode 100644 monitor/qmp.c
 create mode 100644 monitor/Makefile.objs
 create mode 100644 monitor/trace-events

-- 
2.20.1

[Qemu-block] [PATCH v3 08/15] monitor: Move {hmp, qmp}.c to monitor/{hmp, qmp}-cmds.c

[Kevin Wolf]

Now that we have a monitor/ subdirectory, let's move hmp.c and qmp.c
from the root directory there. As they contain implementations of
monitor commands, rename them to {hmp,qmp}-cmds.c, so that {hmp,qmp}.c
are free for the HMP and QMP infrastructure.

Signed-off-by: Kevin Wolf 
Reviewed-by: Dr. David Alan Gilbert 
Reviewed-by: Markus Armbruster 
---
 docs/devel/writing-qmp-commands.txt | 9 +
 hmp.c => monitor/hmp-cmds.c | 2 +-
 qmp.c => monitor/qmp-cmds.c | 2 +-
 MAINTAINERS | 5 +++--
 Makefile.objs   | 2 +-
 monitor/Makefile.objs   | 1 +
 6 files changed, 12 insertions(+), 9 deletions(-)
 rename hmp.c => monitor/hmp-cmds.c (99%)
 rename qmp.c => monitor/qmp-cmds.c (99%)

diff --git a/docs/devel/writing-qmp-commands.txt 
b/docs/devel/writing-qmp-commands.txt
index cc6ecd6d5d..46a6c48683 100644
--- a/docs/devel/writing-qmp-commands.txt
+++ b/docs/devel/writing-qmp-commands.txt
@@ -20,7 +20,7 @@ new QMP command.
 
 2. Write the QMP command itself, which is a regular C function. Preferably,
the command should be exported by some QEMU subsystem. But it can also be
-   added to the qmp.c file
+   added to the monitor/qmp-cmds.c file
 
 3. At this point the command can be tested under the QMP protocol
 
@@ -101,7 +101,8 @@ protocol data.
 
 The next step is to write the "hello-world" implementation. As explained
 earlier, it's preferable for commands to live in QEMU subsystems. But
-"hello-world" doesn't pertain to any, so we put its implementation in qmp.c:
+"hello-world" doesn't pertain to any, so we put its implementation in
+monitor/qmp-cmds.c:
 
 void qmp_hello_world(Error **errp)
 {
@@ -146,7 +147,7 @@ for mandatory arguments). Finally, 'str' is the argument's 
type, which
 stands for "string". The QAPI also supports integers, booleans, enumerations
 and user defined types.
 
-Now, let's update our C implementation in qmp.c:
+Now, let's update our C implementation in monitor/qmp-cmds.c:
 
 void qmp_hello_world(bool has_message, const char *message, Error **errp)
 {
@@ -267,7 +268,7 @@ monitor (HMP).
 
 With the introduction of the QAPI, HMP commands make QMP calls. Most of the
 time HMP commands are simple wrappers. All HMP commands implementation exist in
-the hmp.c file.
+the monitor/hmp-cmds.c file.
 
 Here's the implementation of the "hello-world" HMP command:
 
diff --git a/hmp.c b/monitor/hmp-cmds.c
similarity index 99%
rename from hmp.c
rename to monitor/hmp-cmds.c
index 99414cd39c..712737cd18 100644
--- a/hmp.c
+++ b/monitor/hmp-cmds.c
@@ -1,5 +1,5 @@
 /*
- * Human Monitor Interface
+ * Human Monitor Interface commands
  *
  * Copyright IBM, Corp. 2011
  *
diff --git a/qmp.c b/monitor/qmp-cmds.c
similarity index 99%
rename from qmp.c
rename to monitor/qmp-cmds.c
index 6797568444..f1b1e4f08b 100644
--- a/qmp.c
+++ b/monitor/qmp-cmds.c
@@ -1,5 +1,5 @@
 /*
- * QEMU Management Protocol
+ * QEMU Management Protocol commands
  *
  * Copyright IBM, Corp. 2011
  *
diff --git a/MAINTAINERS b/MAINTAINERS
index 231964c1da..884f4b16bb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1917,7 +1917,8 @@ Human Monitor (HMP)
 M: Dr. David Alan Gilbert 
 S: Maintained
 F: monitor/misc.c
-F: hmp.[ch]
+F: monitor/hmp*
+F: hmp.h
 F: hmp-commands*.hx
 F: include/monitor/hmp-target.h
 F: tests/test-hmp.c
@@ -2037,7 +2038,7 @@ F: tests/check-qom-proplist.c
 QMP
 M: Markus Armbruster 
 S: Supported
-F: qmp.c
+F: monitor/qmp*
 F: monitor/misc.c
 F: docs/devel/*qmp-*
 F: docs/interop/*qmp-*
diff --git a/Makefile.objs b/Makefile.objs
index dd39a70b48..9495fcbc7e 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -83,8 +83,8 @@ common-obj-$(CONFIG_FDT) += device_tree.o
 ##
 # qapi
 
-common-obj-y += qmp.o hmp.o
 common-obj-y += qapi/
+common-obj-y += monitor/
 endif
 
 ###
diff --git a/monitor/Makefile.objs b/monitor/Makefile.objs
index e783b0616b..a7170af6e1 100644
--- a/monitor/Makefile.objs
+++ b/monitor/Makefile.objs
@@ -1 +1,2 @@
 obj-y += misc.o
+common-obj-y += qmp-cmds.o hmp-cmds.o
-- 
2.20.1

[Qemu-block] testsuite spurious failure due to leftover files

[Eric Blake]

iotests ./check -qcow2 failed for me today:

191  fail   [10:46:04] [10:46:13]   0s (last: 7s)output
mismatch (see 191.out.bad)
--- /home/eblake/qemu/tests/qemu-iotests/191.out2019-05-24
09:27:46.449348997 -0500
+++ /home/eblake/qemu/tests/qemu-iotests/191.out.bad2019-06-13
10:46:13.906004319 -0500
@@ -8,6 +8,8 @@
 Formatting 'TEST_DIR/t.IMGFMT.ovl2', fmt=IMGFMT size=67108864
backing_file=TEST_DIR/t.IMGFMT.mid
 wrote 65536/65536 bytes at offset 1048576
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+mkfifo: cannot create fifo
'/home/eblake/qemu/tests/qemu-iotests/scratch/qmp-out-17782_0': File exists
+mkfifo: cannot create fifo
'/home/eblake/qemu/tests/qemu-iotests/scratch/qmp-in-17782_0': File exists
 {
 "return": {
 }

If someone wants a good project to take on - it would be nice to revive
Jeff Cody's initial work on refactoring the iotests to operate with one
directory per test, allowing things to be run in parallel, and allowing
cleanup to be much easier (nuke the temp directories, rather than trying
to track down which test left a possibly-colliding file name behind)

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PULL 8/9] block/nbd: merge nbd-client.* to nbd.c

[Eric Blake]

From: Vladimir Sementsov-Ogievskiy 

No reason for keeping driver handlers realization separate from driver
structure. We can get rid of extra header file.

While being here, fix comments style, restore forgotten comments for
NBD_FOREACH_REPLY_CHUNK and nbd_reply_chunk_iter_receive, remove extra
includes.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20190611102720.86114-3-vsement...@virtuozzo.com>
Reviewed-by: Eric Blake 
Signed-off-by: Eric Blake 
---
 block/nbd-client.h  |   62 ---
 block/nbd-client.c  | 1226 -
 block/nbd.c | 1285 +--
 block/Makefile.objs |2 +-
 block/trace-events  |2 +-
 5 files changed, 1255 insertions(+), 1322 deletions(-)
 delete mode 100644 block/nbd-client.h
 delete mode 100644 block/nbd-client.c

diff --git a/block/nbd-client.h b/block/nbd-client.h
deleted file mode 100644
index 570538f4c884..
--- a/block/nbd-client.h
+++ /dev/null
@@ -1,62 +0,0 @@
-#ifndef NBD_CLIENT_H
-#define NBD_CLIENT_H
-
-#include "block/nbd.h"
-#include "block/block_int.h"
-#include "io/channel-socket.h"
-
-#define MAX_NBD_REQUESTS16
-
-typedef struct {
-Coroutine *coroutine;
-uint64_t offset;/* original offset of the request */
-bool receiving; /* waiting for connection_co? */
-} NBDClientRequest;
-
-typedef struct NBDClientSession {
-QIOChannelSocket *sioc; /* The master data channel */
-QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
-NBDExportInfo info;
-
-CoMutex send_mutex;
-CoQueue free_sema;
-Coroutine *connection_co;
-int in_flight;
-
-NBDClientRequest requests[MAX_NBD_REQUESTS];
-NBDReply reply;
-BlockDriverState *bs;
-bool quit;
-} NBDClientSession;
-
-NBDClientSession *nbd_get_client_session(BlockDriverState *bs);
-
-int nbd_client_init(BlockDriverState *bs,
-SocketAddress *saddr,
-const char *export_name,
-QCryptoTLSCreds *tlscreds,
-const char *hostname,
-const char *x_dirty_bitmap,
-Error **errp);
-void nbd_client_close(BlockDriverState *bs);
-
-int nbd_client_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes);
-int nbd_client_co_flush(BlockDriverState *bs);
-int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t offset,
-  uint64_t bytes, QEMUIOVector *qiov, int flags);
-int nbd_client_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
-int bytes, BdrvRequestFlags flags);
-int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,
- uint64_t bytes, QEMUIOVector *qiov, int flags);
-
-void nbd_client_detach_aio_context(BlockDriverState *bs);
-void nbd_client_attach_aio_context(BlockDriverState *bs,
-   AioContext *new_context);
-
-int coroutine_fn nbd_client_co_block_status(BlockDriverState *bs,
-bool want_zero,
-int64_t offset, int64_t bytes,
-int64_t *pnum, int64_t *map,
-BlockDriverState **file);
-
-#endif /* NBD_CLIENT_H */
diff --git a/block/nbd-client.c b/block/nbd-client.c
deleted file mode 100644
index f89a67c23b64..
--- a/block/nbd-client.c
+++ /dev/null
@@ -1,1226 +0,0 @@
-/*
- * QEMU Block driver for  NBD
- *
- * Copyright (C) 2016 Red Hat, Inc.
- * Copyright (C) 2008 Bull S.A.S.
- * Author: Laurent Vivier 
- *
- * Some parts:
- *Copyright (C) 2007 Anthony Liguori 
- *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to 
deal
- * in the Software without restriction, including without limitation the rights
- * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- * copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
- * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
- * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- * THE SOFTWARE.
- */
-
-#include "qemu/osdep.h"
-
-#include "trace.h"
-#include "qapi/error.h"
-#include "nbd-client.h"
-
-#define HANDLE_TO_INDEX(bs, handle) ((handle) ^ (uint64_t)(intptr_t)(bs))

[Qemu-block] [PULL 9/9] block/nbd: merge NBDClientSession struct back to BDRVNBDState

[Eric Blake]

From: Vladimir Sementsov-Ogievskiy 

No reason to keep it separate, it differs from others block driver
behavior and therefore confuses. Instead of generic
  'state = (State*)bs->opaque' we have to use special helper.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20190611102720.86114-4-vsement...@virtuozzo.com>
Reviewed-by: Eric Blake 
Signed-off-by: Eric Blake 
---
 block/nbd.c | 197 +---
 1 file changed, 94 insertions(+), 103 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 1f00be2d664e..81edabbf35ed 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -53,7 +53,7 @@ typedef struct {
 bool receiving; /* waiting for connection_co? */
 } NBDClientRequest;

-typedef struct NBDClientSession {
+typedef struct BDRVNBDState {
 QIOChannelSocket *sioc; /* The master data channel */
 QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
 NBDExportInfo info;
@@ -67,24 +67,13 @@ typedef struct NBDClientSession {
 NBDReply reply;
 BlockDriverState *bs;
 bool quit;
-} NBDClientSession;
-
-typedef struct BDRVNBDState {
-NBDClientSession client;

 /* For nbd_refresh_filename() */
 SocketAddress *saddr;
 char *export, *tlscredsid;
 } BDRVNBDState;

-static NBDClientSession *nbd_get_client_session(BlockDriverState *bs)
-{
-BDRVNBDState *s = bs->opaque;
-return >client;
-}
-
-
-static void nbd_recv_coroutines_wake_all(NBDClientSession *s)
+static void nbd_recv_coroutines_wake_all(BDRVNBDState *s)
 {
 int i;

@@ -99,14 +88,15 @@ static void nbd_recv_coroutines_wake_all(NBDClientSession 
*s)

 static void nbd_client_detach_aio_context(BlockDriverState *bs)
 {
-NBDClientSession *client = nbd_get_client_session(bs);
-qio_channel_detach_aio_context(QIO_CHANNEL(client->ioc));
+BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
+
+qio_channel_detach_aio_context(QIO_CHANNEL(s->ioc));
 }

 static void nbd_client_attach_aio_context_bh(void *opaque)
 {
 BlockDriverState *bs = opaque;
-NBDClientSession *client = nbd_get_client_session(bs);
+BDRVNBDState *s = (BDRVNBDState *)bs->opaque;

 /*
  * The node is still drained, so we know the coroutine has yielded in
@@ -114,15 +104,16 @@ static void nbd_client_attach_aio_context_bh(void *opaque)
  * entered for the first time. Both places are safe for entering the
  * coroutine.
  */
-qemu_aio_coroutine_enter(bs->aio_context, client->connection_co);
+qemu_aio_coroutine_enter(bs->aio_context, s->connection_co);
 bdrv_dec_in_flight(bs);
 }

 static void nbd_client_attach_aio_context(BlockDriverState *bs,
   AioContext *new_context)
 {
-NBDClientSession *client = nbd_get_client_session(bs);
-qio_channel_attach_aio_context(QIO_CHANNEL(client->ioc), new_context);
+BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
+
+qio_channel_attach_aio_context(QIO_CHANNEL(s->ioc), new_context);

 bdrv_inc_in_flight(bs);

@@ -136,26 +127,26 @@ static void 
nbd_client_attach_aio_context(BlockDriverState *bs,

 static void nbd_teardown_connection(BlockDriverState *bs)
 {
-NBDClientSession *client = nbd_get_client_session(bs);
+BDRVNBDState *s = (BDRVNBDState *)bs->opaque;

-assert(client->ioc);
+assert(s->ioc);

 /* finish any pending coroutines */
-qio_channel_shutdown(client->ioc,
+qio_channel_shutdown(s->ioc,
  QIO_CHANNEL_SHUTDOWN_BOTH,
  NULL);
-BDRV_POLL_WHILE(bs, client->connection_co);
+BDRV_POLL_WHILE(bs, s->connection_co);

 nbd_client_detach_aio_context(bs);
-object_unref(OBJECT(client->sioc));
-client->sioc = NULL;
-object_unref(OBJECT(client->ioc));
-client->ioc = NULL;
+object_unref(OBJECT(s->sioc));
+s->sioc = NULL;
+object_unref(OBJECT(s->ioc));
+s->ioc = NULL;
 }

 static coroutine_fn void nbd_connection_entry(void *opaque)
 {
-NBDClientSession *s = opaque;
+BDRVNBDState *s = opaque;
 uint64_t i;
 int ret = 0;
 Error *local_err = NULL;
@@ -223,7 +214,7 @@ static int nbd_co_send_request(BlockDriverState *bs,
NBDRequest *request,
QEMUIOVector *qiov)
 {
-NBDClientSession *s = nbd_get_client_session(bs);
+BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
 int rc, i;

 qemu_co_mutex_lock(>send_mutex);
@@ -298,7 +289,7 @@ static inline uint64_t payload_advance64(uint8_t **payload)
 return ldq_be_p(*payload - 8);
 }

-static int nbd_parse_offset_hole_payload(NBDClientSession *client,
+static int nbd_parse_offset_hole_payload(BDRVNBDState *s,
  NBDStructuredReplyChunk *chunk,
  uint8_t *payload, uint64_t 
orig_offset,
  QEMUIOVector *qiov, Error **errp)
@@ -321,8 +312,8 @@ static int 

[Qemu-block] [PATCH v3 05/15] monitor: Remove Monitor.cmd_table indirection

[Kevin Wolf]

Monitor.cmd_table is initialised to point to mon_cmds and never changed
afterwards. We can remove the indirection and just reference mon_cmds
directly instead.

Signed-off-by: Kevin Wolf 
---
 monitor.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/monitor.c b/monitor.c
index 572449f6db..5eacaa48a6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -193,7 +193,6 @@ struct Monitor {
 bool use_io_thread;
 
 gchar *mon_cpu_path;
-mon_cmd_t *cmd_table;
 QTAILQ_ENTRY(Monitor) entry;
 
 /*
@@ -722,8 +721,6 @@ static void monitor_data_init(Monitor *mon, int flags, bool 
skip_flush,
 }
 qemu_mutex_init(>mon_lock);
 mon->outbuf = qstring_new();
-/* Use *mon_cmds by default. */
-mon->cmd_table = mon_cmds;
 mon->skip_flush = skip_flush;
 mon->use_io_thread = use_io_thread;
 mon->flags = flags;
@@ -1026,7 +1023,7 @@ static void help_cmd(Monitor *mon, const char *name)
 }
 
 /* 2. dump the contents according to parsed args */
-help_cmd_dump(mon, mon->cmd_table, args, nb_args, 0);
+help_cmd_dump(mon, mon_cmds, args, nb_args, 0);
 
 free_cmdline_args(args, nb_args);
 }
@@ -3487,7 +3484,7 @@ static void handle_hmp_command(MonitorHMP *mon, const 
char *cmdline)
 
 trace_handle_hmp_command(mon, cmdline);
 
-cmd = monitor_parse_command(mon, cmdline, , mon->common.cmd_table);
+cmd = monitor_parse_command(mon, cmdline, , mon_cmds);
 if (!cmd) {
 return;
 }
@@ -4134,7 +4131,7 @@ static void monitor_find_completion(void *opaque,
 }
 
 /* 2. auto complete according to args */
-monitor_find_completion_by_table(mon, mon->common.cmd_table, args, 
nb_args);
+monitor_find_completion_by_table(mon, mon_cmds, args, nb_args);
 
 cleanup:
 free_cmdline_args(args, nb_args);
-- 
2.20.1

[Qemu-block] [PATCH v3 07/15] Move monitor.c to monitor/misc.c

[Kevin Wolf]

Create a new monitor/ subdirectory and move monitor.c there. As the plan
is to move the monitor core into separate files, use the chance to
rename it to misc.c.

Signed-off-by: Kevin Wolf 
Reviewed-by: Dr. David Alan Gilbert 
Reviewed-by: Markus Armbruster 
---
 docs/devel/writing-qmp-commands.txt |  2 +-
 monitor.c => monitor/misc.c |  2 +-
 MAINTAINERS |  4 ++--
 Makefile.objs   |  1 +
 Makefile.target |  3 ++-
 monitor/Makefile.objs   |  1 +
 monitor/trace-events| 11 +++
 trace-events| 10 --
 8 files changed, 19 insertions(+), 15 deletions(-)
 rename monitor.c => monitor/misc.c (99%)
 create mode 100644 monitor/Makefile.objs
 create mode 100644 monitor/trace-events

diff --git a/docs/devel/writing-qmp-commands.txt 
b/docs/devel/writing-qmp-commands.txt
index 9dfc62bf5a..cc6ecd6d5d 100644
--- a/docs/devel/writing-qmp-commands.txt
+++ b/docs/devel/writing-qmp-commands.txt
@@ -470,7 +470,7 @@ it's good practice to always check for errors.
 
 Another important detail is that HMP's "info" commands don't go into the
 hmp-commands.hx. Instead, they go into the info_cmds[] table, which is defined
-in the monitor.c file. The entry for the "info alarmclock" follows:
+in the monitor/misc.c file. The entry for the "info alarmclock" follows:
 
 {
 .name   = "alarmclock",
diff --git a/monitor.c b/monitor/misc.c
similarity index 99%
rename from monitor.c
rename to monitor/misc.c
index 006c650761..e5db04265d 100644
--- a/monitor.c
+++ b/monitor/misc.c
@@ -64,7 +64,7 @@
 #include "qapi/qmp/json-parser.h"
 #include "qapi/qmp/qlist.h"
 #include "qom/object_interfaces.h"
-#include "trace-root.h"
+#include "trace.h"
 #include "trace/control.h"
 #include "monitor/hmp-target.h"
 #ifdef CONFIG_TRACE_SIMPLE
diff --git a/MAINTAINERS b/MAINTAINERS
index 588c8d947a..231964c1da 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1916,7 +1916,7 @@ F: qapi/run-state.json
 Human Monitor (HMP)
 M: Dr. David Alan Gilbert 
 S: Maintained
-F: monitor.c
+F: monitor/misc.c
 F: hmp.[ch]
 F: hmp-commands*.hx
 F: include/monitor/hmp-target.h
@@ -2038,7 +2038,7 @@ QMP
 M: Markus Armbruster 
 S: Supported
 F: qmp.c
-F: monitor.c
+F: monitor/misc.c
 F: docs/devel/*qmp-*
 F: docs/interop/*qmp-*
 F: scripts/qmp/
diff --git a/Makefile.objs b/Makefile.objs
index c8337fa34b..dd39a70b48 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -130,6 +130,7 @@ trace-events-subdirs =
 trace-events-subdirs += accel/kvm
 trace-events-subdirs += accel/tcg
 trace-events-subdirs += crypto
+trace-events-subdirs += monitor
 ifeq ($(CONFIG_USER_ONLY),y)
 trace-events-subdirs += linux-user
 endif
diff --git a/Makefile.target b/Makefile.target
index ecd856e3a3..72c267f7dc 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -148,9 +148,10 @@ endif #CONFIG_BSD_USER
 #
 # System emulator target
 ifdef CONFIG_SOFTMMU
-obj-y += arch_init.o cpus.o monitor.o gdbstub.o balloon.o ioport.o numa.o
+obj-y += arch_init.o cpus.o gdbstub.o balloon.o ioport.o numa.o
 obj-y += qtest.o
 obj-y += hw/
+obj-y += monitor/
 obj-y += qapi/
 obj-y += memory.o
 obj-y += memory_mapping.o
diff --git a/monitor/Makefile.objs b/monitor/Makefile.objs
new file mode 100644
index 00..e783b0616b
--- /dev/null
+++ b/monitor/Makefile.objs
@@ -0,0 +1 @@
+obj-y += misc.o
diff --git a/monitor/trace-events b/monitor/trace-events
new file mode 100644
index 00..abfdf20b14
--- /dev/null
+++ b/monitor/trace-events
@@ -0,0 +1,11 @@
+# See docs/devel/tracing.txt for syntax documentation.
+
+# misc.c
+monitor_protocol_event_handler(uint32_t event, void *qdict) "event=%d data=%p"
+monitor_protocol_event_emit(uint32_t event, void *data) "event=%d data=%p"
+monitor_protocol_event_queue(uint32_t event, void *qdict, uint64_t rate) 
"event=%d data=%p rate=%" PRId64
+handle_hmp_command(void *mon, const char *cmdline) "mon %p cmdline: %s"
+handle_qmp_command(void *mon, const char *req) "mon %p req: %s"
+monitor_suspend(void *ptr, int cnt) "mon %p: %d"
+monitor_qmp_cmd_in_band(const char *id) "%s"
+monitor_qmp_cmd_out_of_band(const char *id) "%s"
diff --git a/trace-events b/trace-events
index 844ee58dd9..aeea3c2bdb 100644
--- a/trace-events
+++ b/trace-events
@@ -41,16 +41,6 @@ system_wakeup_request(int reason) "reason=%d"
 qemu_system_shutdown_request(int reason) "reason=%d"
 qemu_system_powerdown_request(void) ""
 
-# monitor.c
-monitor_protocol_event_handler(uint32_t event, void *qdict) "event=%d data=%p"
-monitor_protocol_event_emit(uint32_t event, void *data) "event=%d data=%p"
-monitor_protocol_event_queue(uint32_t event, void *qdict, uint64_t rate) 
"event=%d data=%p rate=%" PRId64
-handle_hmp_command(void *mon, const char *cmdline) "mon %p cmdline: %s"
-handle_qmp_command(void *mon, const char *req) "mon %p req: %s"
-monitor_suspend(void *ptr, int cnt) "mon %p: %d"
-monitor_qmp_cmd_in_band(const 

[Qemu-block] [PATCH v3 02/15] monitor: Split monitor_init in HMP and QMP function

[Kevin Wolf]

Instead of mixing HMP and QMP monitors in the same function, separate
the monitor creation function for both.

While in theory, one could pass both MONITOR_USE_CONTROL and
MONITOR_USE_READLINE before this patch and both flags would do
something, readline support is tightly coupled with HMP: QMP never feeds
its input to readline, and the tab completion function treats the input
as an HMP command. Therefore, this configuration is useless.

After this patch, the QMP path asserts that MONITOR_USE_READLINE is not
set. The HMP path can be used with or without MONITOR_USE_READLINE, like
before.

Signed-off-by: Kevin Wolf 
Reviewed-by: Dr. David Alan Gilbert 
Reviewed-by: Markus Armbruster 
---
 monitor.c | 89 ---
 1 file changed, 52 insertions(+), 37 deletions(-)

diff --git a/monitor.c b/monitor.c
index 9846a5623b..a70c1283b1 100644
--- a/monitor.c
+++ b/monitor.c
@@ -704,7 +704,7 @@ static void handle_hmp_command(Monitor *mon, const char 
*cmdline);
 
 static void monitor_iothread_init(void);
 
-static void monitor_data_init(Monitor *mon, bool skip_flush,
+static void monitor_data_init(Monitor *mon, int flags, bool skip_flush,
   bool use_io_thread)
 {
 if (use_io_thread && !mon_iothread) {
@@ -719,6 +719,7 @@ static void monitor_data_init(Monitor *mon, bool skip_flush,
 mon->skip_flush = skip_flush;
 mon->use_io_thread = use_io_thread;
 mon->qmp.qmp_requests = g_queue_new();
+mon->flags = flags;
 }
 
 static void monitor_data_destroy(Monitor *mon)
@@ -742,7 +743,7 @@ char *qmp_human_monitor_command(const char *command_line, 
bool has_cpu_index,
 char *output = NULL;
 Monitor *old_mon, hmp;
 
-monitor_data_init(, true, false);
+monitor_data_init(, 0, true, false);
 
 old_mon = cur_mon;
 cur_mon = 
@@ -4605,19 +4606,51 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
 monitor_list_append(mon);
 }
 
-void monitor_init(Chardev *chr, int flags)
+static void monitor_init_qmp(Chardev *chr, int flags)
 {
 Monitor *mon = g_malloc(sizeof(*mon));
-bool use_readline = flags & MONITOR_USE_READLINE;
+
+/* Only HMP supports readline */
+assert(!(flags & MONITOR_USE_READLINE));
 
 /* Note: we run QMP monitor in I/O thread when @chr supports that */
-monitor_data_init(mon, false,
-  (flags & MONITOR_USE_CONTROL)
-  && qemu_chr_has_feature(chr,
-  QEMU_CHAR_FEATURE_GCONTEXT));
+monitor_data_init(mon, flags, false,
+  qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT));
 
 qemu_chr_fe_init(>chr, chr, _abort);
-mon->flags = flags;
+qemu_chr_fe_set_echo(>chr, true);
+
+json_message_parser_init(>qmp.parser, handle_qmp_command, mon, NULL);
+if (mon->use_io_thread) {
+/*
+ * Make sure the old iowatch is gone.  It's possible when
+ * e.g. the chardev is in client mode, with wait=on.
+ */
+remove_fd_in_watch(chr);
+/*
+ * We can't call qemu_chr_fe_set_handlers() directly here
+ * since chardev might be running in the monitor I/O
+ * thread.  Schedule a bottom half.
+ */
+aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
+monitor_qmp_setup_handlers_bh, mon);
+/* The bottom half will add @mon to @mon_list */
+} else {
+qemu_chr_fe_set_handlers(>chr, monitor_can_read,
+ monitor_qmp_read, monitor_qmp_event,
+ NULL, mon, NULL, true);
+monitor_list_append(mon);
+}
+}
+
+static void monitor_init_hmp(Chardev *chr, int flags)
+{
+Monitor *mon = g_malloc(sizeof(*mon));
+bool use_readline = flags & MONITOR_USE_READLINE;
+
+monitor_data_init(mon, flags, false, false);
+qemu_chr_fe_init(>chr, chr, _abort);
+
 if (use_readline) {
 mon->rs = readline_init(monitor_readline_printf,
 monitor_readline_flush,
@@ -4626,36 +4659,18 @@ void monitor_init(Chardev *chr, int flags)
 monitor_read_command(mon, 0);
 }
 
-if (monitor_is_qmp(mon)) {
-qemu_chr_fe_set_echo(>chr, true);
-json_message_parser_init(>qmp.parser, handle_qmp_command,
- mon, NULL);
-if (mon->use_io_thread) {
-/*
- * Make sure the old iowatch is gone.  It's possible when
- * e.g. the chardev is in client mode, with wait=on.
- */
-remove_fd_in_watch(chr);
-/*
- * We can't call qemu_chr_fe_set_handlers() directly here
- * since chardev might be running in the monitor I/O
- * thread.  Schedule a bottom half.
- */
-aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
-

Re: [Qemu-block] [PATCH v5 00/42] block: Deal with filters

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> Hi,
> 
> When we introduced filters, we did it a bit casually.  Sure, we talked a
> lot about them before, but that was mostly discussion about where
> implicit filters should be added to the graph (note that we currently
> only have two implicit filters, those being mirror and commit).  But in
> the end, we really just designated some drivers filters (Quorum,
> blkdebug, etc.) and added some specifically (throttle, COR), without
> really looking through the block layer to see where issues might occur.
> 
> It turns out vast areas of the block layer just don’t know about filters
> and cannot really handle them.  Many cases will work in practice, in
> others, well, too bad, you cannot use some feature because some part
> deep inside the block layer looks at your filters and thinks they are
> format nodes.
> 
> This is one reason why this series is needed.  Over time (since v1), a
> second reason has made its way in:
> 
> bs->file is not necessarily the place where a node’s data is stored.
> qcow2 now has external data files, and currently there is no way for the
> general block layer to know that the data is not stored in bs->file.
> Right now, I do not think that has any real consequences (all functions
> that need access to the actual data storage file should only do so as a
> fallback if the driver does not provide some functionality, but qcow2
> should provide it all), but it still shows that we need some way to let
> the general block layer know about such data files.  (Also, I will need
> this for v1 of my “Inquire images’ rotational info” series.)
> 
> I won’t go on and on about this series now, I think the patches pretty
> much speak for themselves now.  If the cover letter gets too long,
> nobody reads it anyway (see previous versions).
> 
> 
> *** This series depends on some others. ***
> 
> Dependencies:
> - [PATCH 0/4] block: Keep track of parent quiescing
> - [PATCH 0/2] vl: Drain before (block) job cancel when quitting
> - [PATCH v2 0/2] blockdev: Overlays are not snapshots
> 
> Based-on: <20190605161118.14544-1-mre...@redhat.com>
> Based-on: <20190612220839.1374-1-mre...@redhat.com>
> Based-on: <20190603202236.1342-1-mre...@redhat.com>

Could you please export a branch?

> 
> 
> v5:
> - Split the huge patches 2 and 3 from the previous version into many
>smaller patches to maintain the potential reviewers’ sanity [Vladimir]

Thank you! In spite of frightening amount of patches, reviewing became a lot
simpler.

> 
> - Added support for compressed writes to the COR and throttle filter
>drivers to demonstrate how that looks, because the backup job needs to
>deal with filters that have such support
> 
> - Added differentiation between bdrv_storage_child(),
>bdrv_primary_child(), and bdrv_metadata_child()
> 
> - A whole lot of things Vladimir has noted
> 
> - Made the block jobs really work with filters.  In case of commit and
>stream, this now means that filters go away if they are between top
>and base.  I think that’s OK because it’s the user’s choice to include
>filters or not.  (They can move the filters around if they prefer a
>different result.)
>- This changes the “Add filter commit test cases” from checking that
>  most things do not work to checking that they do
> 
> - Added the “blockdev: Fix active commit choice” patch because it turned
>out this became necessary after I allowed committing through and with
>filters.
> 
> 
> Max Reitz (42):
>block: Mark commit and mirror as filter drivers
>copy-on-read: Support compressed writes
>throttle: Support compressed writes
>block: Add child access functions
>block: Add chain helper functions
>qcow2: Implement .bdrv_storage_child()
>block: *filtered_cow_child() for *has_zero_init()
>block: bdrv_set_backing_hd() is about bs->backing
>block: Include filters when freezing backing chain
>block: Use CAF in bdrv_is_encrypted()
>block: Add bdrv_supports_compressed_writes()
>block: Use bdrv_filtered_rw* where obvious
>block: Use CAFs in block status functions
>block: Use CAFs when working with backing chains
>block: Re-evaluate backing file handling in reopen
>block: Use child access functions when flushing
>block: Use CAFs in bdrv_refresh_limits()
>block: Use CAFs in bdrv_refresh_filename()
>block: Use CAF in bdrv_co_rw_vmstate()
>block/snapshot: Fall back to storage child
>block: Use CAFs for debug breakpoints
>block: Use CAFs in bdrv_get_allocated_file_size()
>blockdev: Use CAF in external_snapshot_prepare()
>block: Use child access functions for QAPI queries
>mirror: Deal with filters
>backup: Deal with filters
>commit: Deal with filters
>stream: Deal with filters
>nbd: Use CAF when looking for dirty bitmap
>qemu-img: Use child access functions
>block: Drop backing_bs()
>block: Make bdrv_get_cumulative_perm() public
>blockdev: Fix active 

[Qemu-block] [PATCH v3 01/15] monitor: Remove unused password prompting fields

[Kevin Wolf]

Commit 788cf9f8c removed the code for password prompting from the
monitor. Since then, the Monitor fields password_completion_cb and
password_opaque have been unused. Remove them.

Signed-off-by: Kevin Wolf 
Reviewed-by: Dr. David Alan Gilbert 
Reviewed-by: Markus Armbruster 
---
 monitor.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index 5c5cbe254a..9846a5623b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -222,8 +222,6 @@ struct Monitor {
 
 MonitorQMP qmp;
 gchar *mon_cpu_path;
-BlockCompletionFunc *password_completion_cb;
-void *password_opaque;
 mon_cmd_t *cmd_table;
 QTAILQ_ENTRY(Monitor) entry;
 
-- 
2.20.1

Re: [Qemu-block] [PATCH 2/3] block/nbd: merge nbd-client.* to nbd.c

[Eric Blake]

On 6/11/19 5:27 AM, Vladimir Sementsov-Ogievskiy wrote:
> No reason of keeping driver handlers realization in separate of driver
> structure. We can get rid of extra header file.
> 
> While being here, fix comments style, restore forgotten comments for
> NBD_FOREACH_REPLY_CHUNK and nbd_reply_chunk_iter_receive, remove extra
> includes.

It's a little bit harder to review for accuracy when the patch does
cleanups at the same time; but this shows that the patch is fairly
simple to understand:

$ diff -u <(git log -p -1 | sed -n 's/^-//p') <(git log -p -1 | sed -n
's/^\+//p')

the most confusing parts were that HANDLE_TO_INDEX/INDEX_TO_HANDLE moved
differently from nbd_get_client_session(), and
nbd_client_{attach,detach}_aio_conext moved to a different relative
position.  Perhaps the patch could have been split to make the code
motion even easier to follow, but it's not worth respinning now.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/nbd-client.h  |   63 ---
>  block/nbd-client.c  | 1226 -
>  block/nbd.c | 1285 +--
>  block/Makefile.objs |2 +-
>  block/trace-events  |2 +-
>  5 files changed, 1255 insertions(+), 1323 deletions(-)
>  delete mode 100644 block/nbd-client.h
>  delete mode 100644 block/nbd-client.c

Reviewed-by: Eric Blake 

This will tend to cause merge conflicts to anything else touching either
file, so I plan to send a pull request soon.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 3/3] block/nbd: merge NBDClientSession struct back to BDRVNBDState

[Vladimir Sementsov-Ogievskiy]

13.06.2019 18:16, Eric Blake wrote:
> On 6/11/19 5:27 AM, Vladimir Sementsov-Ogievskiy wrote:
>> No reason to keep it separate, it differs from others block driver
>> behavior and therefor confuses. Instead of generic
> 
> s/therefor/therefore/ (both spellings are valid, but the former looks
> archaic)

Interesting, for me it was just a mistake.

> 
>>'state = (State*)bs->opaque' we have to use special helper.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
>>   block/nbd.c | 197 +---
>>   1 file changed, 94 insertions(+), 103 deletions(-)
>>
> 
> Reviewed-by: Eric Blake 
> 
> I'm queuing this series through my NBD tree.
> 

Thanks!! I'll rebase reconnect series on this.

-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH 3/3] block/nbd: merge NBDClientSession struct back to BDRVNBDState

[Eric Blake]

On 6/11/19 5:27 AM, Vladimir Sementsov-Ogievskiy wrote:
> No reason to keep it separate, it differs from others block driver
> behavior and therefor confuses. Instead of generic

s/therefor/therefore/ (both spellings are valid, but the former looks
archaic)

>   'state = (State*)bs->opaque' we have to use special helper.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/nbd.c | 197 +---
>  1 file changed, 94 insertions(+), 103 deletions(-)
> 

Reviewed-by: Eric Blake 

I'm queuing this series through my NBD tree.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 10/42] block: Use CAF in bdrv_is_encrypted()

[Max Reitz]

On 13.06.19 15:16, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 1:09, Max Reitz wrote:
>> bdrv_is_encrypted() should not only check the BDS's backing child, but
>> any filtered child: If a filter's child is encrypted, the filter node
>> itself naturally is encrypted, too.  Furthermore, we need to recurse
>> down the chain.
>>
>> (CAF means child access function.)
> 
> Hmm, so, if only one node in the backing chain is encrypted, all overlays,
> filters or not are considered encrypted too? Even if all the data is in top
> node and is not encrypted?
> 
> Checked that the function is used only for reporting through
> bdrv_query_image_info, which is called from bdrv_block_device_info() (which
> loops through backings), and from collect_image_info_list(), which loops 
> through
> backings if @chain=true.
> 
> And collect_image_info_list() is used only in img_info(), @chain is a mirrored
> --backing-chain parameter..
> 
> So, isn't it more correct to return exactly bs->encrypted in this function? 
> It will
> give more correct and informative results for queries for the whole chain.

Hm.  Maybe? :-)

I personally feel more comfortable to report more devices as being
reported than less.  The description of @encrypted in @BlockDeviceInfo
is vague enough that we can just “make it more precise”.

You’re right, it does sound more useful.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 11/42] block: Add bdrv_supports_compressed_writes()

[Max Reitz]

On 13.06.19 15:29, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 1:09, Max Reitz wrote:
>> Filters cannot compress data themselves but they have to implement
>> .bdrv_co_pwritev_compressed() still (or they cannot forward compressed
>> writes).  Therefore, checking whether
>> bs->drv->bdrv_co_pwritev_compressed is non-NULL is not sufficient to
>> know whether the node can actually handle compressed writes.  This
>> function looks down the filter chain to see whether there is a
>> non-filter that can actually convert the compressed writes into
>> compressed data (and thus normal writes).
> 
> Why not to use this function in (as I remember only 2-3 cases) when
> we check for bs->drv->bdrv_co_pwritev_compressed? It would be a complete fix
> for described problem.

Well, bdrv_driver_pwritev_compressed() doesn’t really care, it will find
out sooner or later anyway (while being passed down the chain).  This is
only really important for the backup job, which will use this function
as of patch 26.  (It isn’t important before 26, because using filters
with backup generally is a gamble before that patch.)

> (hmm, ok, other new APIs are added separately too, for some reason they don't
> confuse me and this confuses)
> 
> On the other hand, (the second time I think about it during review), could
> we handle compression through flags completely?
> We have supported_write_flags feature, which should be used for all these 
> checks..
> And may be, we may drop .bdrv_co_pwritev_compressed at all.

We probably could, yes.  I just felt like this wasn’t the time to do it.
O:-)

> But if you want to keep it as is, it's OK too:
> Reviewed-by: Vladimir Sementsov-Ogievskiy 

Thanks for reviewing!

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 1/3] block/nbd-client: drop stale logout

[Eric Blake]

On 6/11/19 5:27 AM, Vladimir Sementsov-Ogievskiy wrote:
> Drop one on failure path (we have errp) and turn two others into trace
> points.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/nbd-client.h | 9 -
>  block/nbd-client.c | 6 +++---
>  block/trace-events | 2 ++
>  3 files changed, 5 insertions(+), 12 deletions(-)
> 

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 09/42] block: Include filters when freezing backing chain

[Max Reitz]

On 13.06.19 15:04, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 1:09, Max Reitz wrote:
>> In order to make filters work in backing chains, the associated
>> functions must be able to deal with them and freeze all filter links, be
>> they COW or R/W filter links.
>>
>> While at it, add some comments that note which functions require their
>> caller to ensure that a given child link is not frozen, and how the
>> callers do so.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>   block.c | 45 -
>>   1 file changed, 32 insertions(+), 13 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 8438b0699e..45882a3470 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2214,12 +2214,15 @@ static void bdrv_replace_child_noperm(BdrvChild 
>> *child,
>>* If @new_bs is not NULL, bdrv_check_perm() must be called beforehand, as 
>> this
>>* function uses bdrv_set_perm() to update the permissions according to 
>> the new
>>* reference that @new_bs gets.
>> + *
>> + * Callers must ensure that child->frozen is false.
>>*/
>>   static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
>>   {
>>   BlockDriverState *old_bs = child->bs;
>>   uint64_t perm, shared_perm;
>>   
>> +/* Asserts that child->frozen == false */
>>   bdrv_replace_child_noperm(child, new_bs);
>>   
>>   if (old_bs) {
>> @@ -2360,6 +2363,7 @@ static void bdrv_detach_child(BdrvChild *child)
>>   g_free(child);
>>   }
>>   
>> +/* Callers must ensure that child->frozen is false. */
> 
> Is such a comment better than one-line extra assertion at start of the 
> function body?

Well, there already is an assertion, it is in
bdrv_replace_child_noperm().  I personally prefer to read comments than
assertions.

>>   void bdrv_root_unref_child(BdrvChild *child)
>>   {
>>   BlockDriverState *child_bs;
>> @@ -2369,6 +2373,7 @@ void bdrv_root_unref_child(BdrvChild *child)
>>   bdrv_unref(child_bs);
>>   }
>>   
>> +/* Callers must ensure that child->frozen is false. */
>>   void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child)
>>   {
>>   if (child == NULL) {
>> @@ -2435,6 +2440,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
>> BlockDriverState *backing_hd,
>>   }
>>   
>>   if (bs->backing) {
>> +/* Cannot be frozen, we checked that above */
>>   bdrv_unref_child(bs, bs->backing);
>>   }
>>   
>> @@ -3908,6 +3914,7 @@ static void bdrv_close(BlockDriverState *bs)
>>   
>>   if (bs->drv) {
>>   if (bs->drv->bdrv_close) {
>> +/* Must unfreeze all children, so bdrv_unref_child() works */
>>   bs->drv->bdrv_close(bs);
>>   }
>>   bs->drv = NULL;
>> @@ -4281,17 +4288,20 @@ BlockDriverState *bdrv_find_base(BlockDriverState 
>> *bs)
>>* Return true if at least one of the backing links between @bs and
>>* @base is frozen. @errp is set if that's the case.
>>* @base must be reachable from @bs, or NULL.
>> + * (Filters are treated as normal elements of the backing chain.)
>>*/
>>   bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState 
>> *base,
>> Error **errp)
>>   {
>>   BlockDriverState *i;
>> +BdrvChild *child;
>>   
>> -for (i = bs; i != base; i = backing_bs(i)) {
>> -if (i->backing && i->backing->frozen) {
>> +for (i = bs; i != base; i = child_bs(child)) {
>> +child = bdrv_filtered_child(i);
>> +
>> +if (child && child->frozen) {
>>   error_setg(errp, "Cannot change '%s' link from '%s' to '%s'",
>> -   i->backing->name, i->node_name,
>> -   backing_bs(i)->node_name);
>> +   child->name, i->node_name, child->bs->node_name);
>>   return true;
>>   }
>>   }
>> @@ -4305,19 +4315,22 @@ bool bdrv_is_backing_chain_frozen(BlockDriverState 
>> *bs, BlockDriverState *base,
>>* none of the links are modified.
>>* @base must be reachable from @bs, or NULL.
>>* Returns 0 on success. On failure returns < 0 and sets @errp.
>> + * (Filters are treated as normal elements of the backing chain.)
>>*/
>>   int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
>> Error **errp)
>>   {
>>   BlockDriverState *i;
>> +BdrvChild *child;
>>   
>>   if (bdrv_is_backing_chain_frozen(bs, base, errp)) {
>>   return -EPERM;
>>   }
>>   
>> -for (i = bs; i != base; i = backing_bs(i)) {
>> -if (i->backing) {
>> -i->backing->frozen = true;
>> +for (i = bs; i != base; i = child_bs(child)) {
>> +child = bdrv_filtered_child(i);
>> +if (child) {
>> +child->frozen = true;
>>   }
>>   }
>>   
>> @@ -4328,15 +4341,18 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, 
>> BlockDriverState *base,
>>* Unfreeze all backing links between @bs and 

Re: [Qemu-block] [PATCH v5 10/42] block: Use CAF in bdrv_is_encrypted()

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> bdrv_is_encrypted() should not only check the BDS's backing child, but
> any filtered child: If a filter's child is encrypted, the filter node
> itself naturally is encrypted, too.  Furthermore, we need to recurse
> down the chain.
> 
> (CAF means child access function.)

Hmm, so, if only one node in the backing chain is encrypted, all overlays,
filters or not are considered encrypted too? Even if all the data is in top
node and is not encrypted?

Checked that the function is used only for reporting through
bdrv_query_image_info, which is called from bdrv_block_device_info() (which
loops through backings), and from collect_image_info_list(), which loops through
backings if @chain=true.

And collect_image_info_list() is used only in img_info(), @chain is a mirrored
--backing-chain parameter..

So, isn't it more correct to return exactly bs->encrypted in this function? It 
will
give more correct and informative results for queries for the whole chain.


> 
> Signed-off-by: Max Reitz 
> ---
>   block.c | 8 ++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 45882a3470..567a0f82c8 100644
> --- a/block.c
> +++ b/block.c
> @@ -4574,10 +4574,14 @@ bool bdrv_is_sg(BlockDriverState *bs)
>   
>   bool bdrv_is_encrypted(BlockDriverState *bs)
>   {
> -if (bs->backing && bs->backing->bs->encrypted) {
> +BlockDriverState *filtered = bdrv_filtered_bs(bs);
> +if (bs->encrypted) {
>   return true;
>   }
> -return bs->encrypted;
> +if (filtered && bdrv_is_encrypted(filtered)) {
> +return true;
> +}
> +return false;
>   }
>   
>   const char *bdrv_get_format_name(BlockDriverState *bs)
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v8 2/7] block: swap operation order in bdrv_append

[Vladimir Sementsov-Ogievskiy]

13.06.2019 16:45, Max Reitz wrote:
> On 29.05.19 17:46, Vladimir Sementsov-Ogievskiy wrote:
>> bs_top parents may conflict with bs_new backing child permissions, so
>> let's do bdrv_replace_node first, it covers more possible cases.
>>
>> It is needed for further implementation of backup-top filter, which
>> don't want to share write permission on its backing child.
>>
>> Side effect is that we may set backing hd when device name is already
>> available, so 085 iotest output is changed.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
>>   block.c| 11 ---
>>   tests/qemu-iotests/085.out |  2 +-
>>   2 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index e6e9770704..57216f4115 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4088,22 +4088,27 @@ void bdrv_append(BlockDriverState *bs_new, 
>> BlockDriverState *bs_top,
>>   {
>>   Error *local_err = NULL;
>>   
>> -bdrv_set_backing_hd(bs_new, bs_top, _err);
>> +bdrv_ref(bs_top);
>> +
>> +bdrv_replace_node(bs_top, bs_new, _err);
>>   if (local_err) {
>>   error_propagate(errp, local_err);
>> +error_prepend(errp, "Failed to replace node: ");
>>   goto out;
>>   }
>>   
>> -bdrv_replace_node(bs_top, bs_new, _err);
>> +bdrv_set_backing_hd(bs_new, bs_top, _err);
>>   if (local_err) {
>> +bdrv_replace_node(bs_new, bs_top, _abort);
> 
> Hm.  I see the need for switching this stuff around, but this
> _abort is much more difficult to verify than the previous one for
> bdrv_set_backing_hd(..., NULL, ...).  I think it at least warrants a
> comment why the order has to be this way (using git blame has the
> disadvantage of fading over time as other people modify a piece of

so, I always use git log -p --  instead, and search through it)

> code), and why this _abort is safe.

ok, will add

> 
> Max
> 
>>   error_propagate(errp, local_err);
>> -bdrv_set_backing_hd(bs_new, NULL, _abort);
>> +error_prepend(errp, "Failed to set backing: ");
>>   goto out;
>>   }
>>   
>>   /* bs_new is now referenced by its new parents, we don't need the
>>* additional reference any more. */
>>   out:
>> +bdrv_unref(bs_top);
>>   bdrv_unref(bs_new);
>>   }
>>   
>> diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out
>> index 6edf107f55..e5a2645bf5 100644
>> --- a/tests/qemu-iotests/085.out
>> +++ b/tests/qemu-iotests/085.out
>> @@ -74,7 +74,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
>> backing_file=TEST_DIR/
>>   
>>   === Invalid command - snapshot node used as backing hd ===
>>   
>> -{"error": {"class": "GenericError", "desc": "Node 'snap_11' is busy: node 
>> is used as backing hd of 'snap_12'"}}
>> +{"error": {"class": "GenericError", "desc": "Node 'snap_11' is busy: node 
>> is used as backing hd of 'virtio0'"}}
>>   
>>   === Invalid command - snapshot node has a backing image ===
>>   
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [Qemu-devel] [PATCH v2 09/11] monitor: Split out monitor/qmp.c

[Kevin Wolf]

Am 13.06.2019 um 07:38 hat Markus Armbruster geschrieben:
> Kevin Wolf  writes:
> 
> > Am 12.06.2019 um 15:11 hat Markus Armbruster geschrieben:
> >> Kevin Wolf  writes:
> >> > --- a/include/monitor/monitor.h
> >> > +++ b/include/monitor/monitor.h
> >> > @@ -21,6 +21,7 @@ bool monitor_cur_is_qmp(void);
> >> >  
> >> >  void monitor_init_globals(void);
> >> >  void monitor_init(Chardev *chr, int flags);
> >> > +void monitor_init_qmp(Chardev *chr, int flags);
> >> 
> >> Why does this one go to the non-internal header?
> >
> > Most callers already know whether they want QMP or HMP, so they can just
> > directly create the right thing instead of going through the
> > monitor_init() wrapper.
> >
> > If you prefer, I can move it to the internal header, though. It's not
> > called externally yet.
> 
> As is, monitor_init_qmp() and monitor_init_hmp() are awkward interfaces:
> what if you pass MONITOR_USE_CONTROL to monitor_init_hmp()?
> 
> I can see just one call passing flags that aren't compile-time
> constant.  I think a better interface would be
> 
> monitor_init_hmp(Chardev *chr);
> monitor_init_qmp(Chardev *chr, bool pretty);
> 
> replacing monitor_init() entirely.  This is my first preference.
> 
> My (somewhat distant) second is hiding the awkward interfaces in the
> internal header for now, and clean them up later.
> 
> Your choice.

I'm doing both, as in move it to the internal header in the code motion
patches, but add some patches at the end of the series to clean it up.

Kevin

Re: [Qemu-block] [PATCH v8 3/7] block: allow not one child for implicit node

[Max Reitz]

On 29.05.19 17:46, Vladimir Sementsov-Ogievskiy wrote:
> Upcoming backup-top filter wants to operate like usual implicit filter
> node with fall-through to backing child. But also needs additional
> target child, let's support that.
> 
> On the other hand, after backup completion (before job dismiss) filter
> is still attached to job blk, but don't have any children. Support this
> too.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block.c | 15 +--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 57216f4115..3f4de3ae32 100644
> --- a/block.c
> +++ b/block.c
> @@ -6200,9 +6200,20 @@ void bdrv_refresh_filename(BlockDriverState *bs)
>  }
>  
>  if (bs->implicit) {
> -/* For implicit nodes, just copy everything from the single child */
> +/*
> + * For implicit nodes, just copy everything from the single child or
> + * from backing, if there are several children.
> + * If there are no children for some reason (filter is still attached
> + * to block-job blk, but already removed from backing chain of 
> device)
> + * do nothing.
> + */
>  child = QLIST_FIRST(>children);
> -assert(QLIST_NEXT(child, next) == NULL);
> +if (!child) {
> +return;
> +} else if (QLIST_NEXT(child, next)) {
> +assert(bs->backing);
> +child = bs->backing;
> +}
>  
>  pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
>  child->bs->exact_filename);

Reviewed-by: Max Reitz 

(To be changed to bdrv_filtered_rw_bs(), of course)



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v8 2/7] block: swap operation order in bdrv_append

[Max Reitz]

On 29.05.19 17:46, Vladimir Sementsov-Ogievskiy wrote:
> bs_top parents may conflict with bs_new backing child permissions, so
> let's do bdrv_replace_node first, it covers more possible cases.
> 
> It is needed for further implementation of backup-top filter, which
> don't want to share write permission on its backing child.
> 
> Side effect is that we may set backing hd when device name is already
> available, so 085 iotest output is changed.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block.c| 11 ---
>  tests/qemu-iotests/085.out |  2 +-
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/block.c b/block.c
> index e6e9770704..57216f4115 100644
> --- a/block.c
> +++ b/block.c
> @@ -4088,22 +4088,27 @@ void bdrv_append(BlockDriverState *bs_new, 
> BlockDriverState *bs_top,
>  {
>  Error *local_err = NULL;
>  
> -bdrv_set_backing_hd(bs_new, bs_top, _err);
> +bdrv_ref(bs_top);
> +
> +bdrv_replace_node(bs_top, bs_new, _err);
>  if (local_err) {
>  error_propagate(errp, local_err);
> +error_prepend(errp, "Failed to replace node: ");
>  goto out;
>  }
>  
> -bdrv_replace_node(bs_top, bs_new, _err);
> +bdrv_set_backing_hd(bs_new, bs_top, _err);
>  if (local_err) {
> +bdrv_replace_node(bs_new, bs_top, _abort);

Hm.  I see the need for switching this stuff around, but this
_abort is much more difficult to verify than the previous one for
bdrv_set_backing_hd(..., NULL, ...).  I think it at least warrants a
comment why the order has to be this way (using git blame has the
disadvantage of fading over time as other people modify a piece of
code), and why this _abort is safe.

Max

>  error_propagate(errp, local_err);
> -bdrv_set_backing_hd(bs_new, NULL, _abort);
> +error_prepend(errp, "Failed to set backing: ");
>  goto out;
>  }
>  
>  /* bs_new is now referenced by its new parents, we don't need the
>   * additional reference any more. */
>  out:
> +bdrv_unref(bs_top);
>  bdrv_unref(bs_new);
>  }
>  
> diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out
> index 6edf107f55..e5a2645bf5 100644
> --- a/tests/qemu-iotests/085.out
> +++ b/tests/qemu-iotests/085.out
> @@ -74,7 +74,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
> backing_file=TEST_DIR/
>  
>  === Invalid command - snapshot node used as backing hd ===
>  
> -{"error": {"class": "GenericError", "desc": "Node 'snap_11' is busy: node is 
> used as backing hd of 'snap_12'"}}
> +{"error": {"class": "GenericError", "desc": "Node 'snap_11' is busy: node is 
> used as backing hd of 'virtio0'"}}
>  
>  === Invalid command - snapshot node has a backing image ===
>  
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 12/42] block: Use bdrv_filtered_rw* where obvious

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> Places that use patterns like
> 
>  if (bs->drv->is_filter && bs->file) {
>  ... something about bs->file->bs ...
>  }
> 
> should be
> 
>  BlockDriverState *filtered = bdrv_filtered_rw_bs(bs);
>  if (filtered) {
>  ... something about @filtered ...
>  }
> 
> instead.

Hmm, in other words, support filters with backing child in all places, where 
only file-based
filters are supported, as we don't want make any semantic difference between 
these two
types of filters.

> 
> Signed-off-by: Max Reitz 

Reviewed-by: Vladimir Sementsov-Ogievskiy 



-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v8 1/7] block: teach bdrv_debug_breakpoint skip filters with backing

[Max Reitz]

On 29.05.19 17:46, Vladimir Sementsov-Ogievskiy wrote:
> Teach bdrv_debug_breakpoint and bdrv_debug_remove_breakpoint skip
> filters with backing. This is needed to implement and use in backup job
> it's own backup_top filter driver (like mirror already has one), and
> without this improvement, breakpoint removal will fail at least in 55
> iotest.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block.c | 34 ++
>  1 file changed, 26 insertions(+), 8 deletions(-)

Well, it would work in the meantime, but the real fix of course is to
use bdrv_primary_bs().

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 11/42] block: Add bdrv_supports_compressed_writes()

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> Filters cannot compress data themselves but they have to implement
> .bdrv_co_pwritev_compressed() still (or they cannot forward compressed
> writes).  Therefore, checking whether
> bs->drv->bdrv_co_pwritev_compressed is non-NULL is not sufficient to
> know whether the node can actually handle compressed writes.  This
> function looks down the filter chain to see whether there is a
> non-filter that can actually convert the compressed writes into
> compressed data (and thus normal writes).

Why not to use this function in (as I remember only 2-3 cases) when
we check for bs->drv->bdrv_co_pwritev_compressed? It would be a complete fix
for described problem.
(hmm, ok, other new APIs are added separately too, for some reason they don't
confuse me and this confuses)

On the other hand, (the second time I think about it during review), could
we handle compression through flags completely?
We have supported_write_flags feature, which should be used for all these 
checks..
And may be, we may drop .bdrv_co_pwritev_compressed at all.

But if you want to keep it as is, it's OK too:
Reviewed-by: Vladimir Sementsov-Ogievskiy 

> 
> Signed-off-by: Max Reitz 
> ---
>   include/block/block.h |  1 +
>   block.c   | 22 ++
>   2 files changed, 23 insertions(+)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 687c03b275..7835c5b370 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -487,6 +487,7 @@ void bdrv_next_cleanup(BdrvNextIterator *it);
>   
>   BlockDriverState *bdrv_next_monitor_owned(BlockDriverState *bs);
>   bool bdrv_is_encrypted(BlockDriverState *bs);
> +bool bdrv_supports_compressed_writes(BlockDriverState *bs);
>   void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
>void *opaque, bool read_only);
>   const char *bdrv_get_node_name(const BlockDriverState *bs);
> diff --git a/block.c b/block.c
> index 567a0f82c8..97774b7b06 100644
> --- a/block.c
> +++ b/block.c
> @@ -4584,6 +4584,28 @@ bool bdrv_is_encrypted(BlockDriverState *bs)
>   return false;
>   }
>   
> +/**
> + * Return whether the given node supports compressed writes.
> + */
> +bool bdrv_supports_compressed_writes(BlockDriverState *bs)
> +{
> +BlockDriverState *filtered = bdrv_filtered_rw_bs(bs);
> +
> +if (!bs->drv || !bs->drv->bdrv_co_pwritev_compressed) {
> +return false;
> +}
> +
> +if (filtered) {
> +/*
> + * Filters can only forward compressed writes, so we have to
> + * check the child.
> + */
> +return bdrv_supports_compressed_writes(filtered);
> +}
> +
> +return true;
> +}
> +
>   const char *bdrv_get_format_name(BlockDriverState *bs)
>   {
>   return bs->drv ? bs->drv->format_name : NULL;
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v9] ssh: switch from libssh2 to libssh

[Philippe Mathieu-Daudé]

On 6/13/19 3:20 PM, Pino Toscano wrote:
> Rewrite the implementation of the ssh block driver to use libssh instead
> of libssh2.  The libssh library has various advantages over libssh2:
> - easier API for authentication (for example for using ssh-agent)
> - easier API for known_hosts handling
> - supports newer types of keys in known_hosts
> 
> Use APIs/features available in libssh 0.8 conditionally, to support
> older versions (which are not recommended though).
> 
> Adjust the tests according to the different error message, and to the
> newer host keys (ed25519) that are used by default with OpenSSH >= 6.7
> and libssh >= 0.7.0.
> 
> Adjust the various Docker/Travis scripts to use libssh when available
> instead of libssh2. The mingw/mxe testing is dropped for now, as there
> are no packages for it.
> 
> Signed-off-by: Pino Toscano 
> Tested-by: Philippe Mathieu-Daudé 
> Acked-by: Alex Bennée 
> ---
> 
> Changes from v8:
> - use a newer key type in iotest 207
> - improve the commit message
> 
> Changes from v7:
> - #if HAVE_LIBSSH_0_8 -> #ifdef HAVE_LIBSSH_0_8
> - ptrdiff_t -> size_t
> 
> Changes from v6:
> - fixed few checkpatch style issues
> - detect libssh 0.8 via symbol detection
> - adjust travis/docker test material
> - remove dead "default" case in a switch
> - use variables for storing MIN() results
> - adapt a documentation bit
> 
> Changes from v5:
> - adapt to newer tracing APIs
> - disable ssh compression (mimic what libssh2 does by default)
> - use build time checks for libssh 0.8, and use newer APIs directly
> 
> Changes from v4:
> - fix wrong usages of error_setg/session_error_setg/sftp_error_setg
> - fix few return code checks
> - remove now-unused parameters in few internal functions
> - allow authentication with "none" method
> - switch to unsigned int for the port number
> - enable TCP_NODELAY on the socket
> - fix one reference error message in iotest 207
> 
> Changes from v3:
> - fix socket cleanup in connect_to_ssh()
> - add comments about the socket cleanup
> - improve the error reporting (closer to what was with libssh2)
> - improve EOF detection on sftp_read()
> 
> Changes from v2:
> - used again an own fd
> - fixed co_yield() implementation
> 
> Changes from v1:
> - fixed jumbo packets writing
> - fixed missing 'err' assignment
> - fixed commit message
> 
>  .travis.yml   |   4 +-
>  block/Makefile.objs   |   6 +-
>  block/ssh.c   | 622 +-
>  block/trace-events|  14 +-
>  configure |  65 +-
>  docs/qemu-block-drivers.texi  |   2 +-
>  .../dockerfiles/debian-win32-cross.docker |   1 -
>  .../dockerfiles/debian-win64-cross.docker |   1 -
>  tests/docker/dockerfiles/fedora.docker|   4 +-
>  tests/docker/dockerfiles/ubuntu.docker|   2 +-
>  tests/docker/dockerfiles/ubuntu1804.docker|   2 +-
>  tests/qemu-iotests/207|   4 +-
>  tests/qemu-iotests/207.out|   2 +-
>  13 files changed, 376 insertions(+), 353 deletions(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 08502c0aa2..75f47df3d2 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -31,7 +31,7 @@ addons:
>- libseccomp-dev
>- libspice-protocol-dev
>- libspice-server-dev
> -  - libssh2-1-dev
> +  - libssh-dev
>- liburcu-dev
>- libusb-1.0-0-dev
>- libvte-2.91-dev
> @@ -268,7 +268,7 @@ matrix:
>  - libseccomp-dev
>  - libspice-protocol-dev
>  - libspice-server-dev
> -- libssh2-1-dev
> +- libssh-dev
>  - liburcu-dev
>  - libusb-1.0-0-dev
>  - libvte-2.91-dev
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index ae11605c9f..bf01429dd5 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -31,7 +31,7 @@ block-obj-$(CONFIG_CURL) += curl.o
>  block-obj-$(CONFIG_RBD) += rbd.o
>  block-obj-$(CONFIG_GLUSTERFS) += gluster.o
>  block-obj-$(CONFIG_VXHS) += vxhs.o
> -block-obj-$(CONFIG_LIBSSH2) += ssh.o
> +block-obj-$(CONFIG_LIBSSH) += ssh.o
>  block-obj-y += accounting.o dirty-bitmap.o
>  block-obj-y += write-threshold.o
>  block-obj-y += backup.o
> @@ -52,8 +52,8 @@ rbd.o-libs := $(RBD_LIBS)
>  gluster.o-cflags   := $(GLUSTERFS_CFLAGS)
>  gluster.o-libs := $(GLUSTERFS_LIBS)
>  vxhs.o-libs:= $(VXHS_LIBS)
> -ssh.o-cflags   := $(LIBSSH2_CFLAGS)
> -ssh.o-libs := $(LIBSSH2_LIBS)
> +ssh.o-cflags   := $(LIBSSH_CFLAGS)
> +ssh.o-libs := $(LIBSSH_LIBS)
>  block-obj-dmg-bz2-$(CONFIG_BZIP2) += dmg-bz2.o
>  block-obj-$(if $(CONFIG_DMG),m,n) += $(block-obj-dmg-bz2-y)
>  dmg-bz2.o-libs := $(BZIP2_LIBS)
> diff --git a/block/ssh.c b/block/ssh.c
> index 6da7b9cbfe..fb458d4548 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -24,8 +24,8 @@
>  
>  #include "qemu/osdep.h"
>  
> 

Re: [Qemu-block] [PATCH v6] ssh: switch from libssh2 to libssh

[Alex Bennée]

Philippe Mathieu-Daudé  writes:

> Cc'ing Alex (Docker, Travis) and Stefan (MinGW)
>
> On 6/5/19 11:36 PM, Pino Toscano wrote:
>> Rewrite the implementation of the ssh block driver to use libssh instead
>> of libssh2.  The libssh library has various advantages over libssh2:
>> - easier API for authentication (for example for using ssh-agent)
>> - easier API for known_hosts handling
>> - supports newer types of keys in known_hosts
>>
>> Use APIs/features available in libssh 0.8 conditionally, to support
>> older versions (which are not recommended though).
>>
>> Signed-off-by: Pino Toscano 
>> ---
>>
>> Changes from v5:
>> - adapt to newer tracing APIs
>> - disable ssh compression (mimic what libssh2 does by default)
>> - use build time checks for libssh 0.8, and use newer APIs directly
>>
>> Changes from v4:
>> - fix wrong usages of error_setg/session_error_setg/sftp_error_setg
>> - fix few return code checks
>> - remove now-unused parameters in few internal functions
>> - allow authentication with "none" method
>> - switch to unsigned int for the port number
>> - enable TCP_NODELAY on the socket
>> - fix one reference error message in iotest 207
>>
>> Changes from v3:
>> - fix socket cleanup in connect_to_ssh()
>> - add comments about the socket cleanup
>> - improve the error reporting (closer to what was with libssh2)
>> - improve EOF detection on sftp_read()
>>
>> Changes from v2:
>> - used again an own fd
>> - fixed co_yield() implementation
>>
>> Changes from v1:
>> - fixed jumbo packets writing
>> - fixed missing 'err' assignment
>> - fixed commit message
>>
>>  block/Makefile.objs|   6 +-
>>  block/ssh.c| 610 +++--
>>  block/trace-events |  14 +-
>>  configure  |  62 ++--
>>  tests/qemu-iotests/207.out |   2 +-
>>  5 files changed, 351 insertions(+), 343 deletions(-)
>>
>> diff --git a/block/Makefile.objs b/block/Makefile.objs
>> index ae11605c9f..bf01429dd5 100644
>> --- a/block/Makefile.objs
>> +++ b/block/Makefile.objs
>> @@ -31,7 +31,7 @@ block-obj-$(CONFIG_CURL) += curl.o
>>  block-obj-$(CONFIG_RBD) += rbd.o
>>  block-obj-$(CONFIG_GLUSTERFS) += gluster.o
>>  block-obj-$(CONFIG_VXHS) += vxhs.o
>> -block-obj-$(CONFIG_LIBSSH2) += ssh.o
>> +block-obj-$(CONFIG_LIBSSH) += ssh.o
>>  block-obj-y += accounting.o dirty-bitmap.o
>>  block-obj-y += write-threshold.o
>>  block-obj-y += backup.o
>> @@ -52,8 +52,8 @@ rbd.o-libs := $(RBD_LIBS)
>>  gluster.o-cflags   := $(GLUSTERFS_CFLAGS)
>>  gluster.o-libs := $(GLUSTERFS_LIBS)
>>  vxhs.o-libs:= $(VXHS_LIBS)
>> -ssh.o-cflags   := $(LIBSSH2_CFLAGS)
>> -ssh.o-libs := $(LIBSSH2_LIBS)
>> +ssh.o-cflags   := $(LIBSSH_CFLAGS)
>> +ssh.o-libs := $(LIBSSH_LIBS)
>>  block-obj-dmg-bz2-$(CONFIG_BZIP2) += dmg-bz2.o
>>  block-obj-$(if $(CONFIG_DMG),m,n) += $(block-obj-dmg-bz2-y)
>>  dmg-bz2.o-libs := $(BZIP2_LIBS)
>> diff --git a/block/ssh.c b/block/ssh.c
>> index 12fd4f39e8..ce2363a471 100644
>> --- a/block/ssh.c
>> +++ b/block/ssh.c
>> @@ -24,8 +24,8 @@
>>
>>  #include "qemu/osdep.h"
>>
>> -#include 
>> -#include 
>> +#include 
>> +#include 
>>
>>  #include "block/block_int.h"
>>  #include "block/qdict.h"
>> @@ -43,14 +43,13 @@
>>  #include "qapi/qobject-output-visitor.h"
>>  #include "trace.h"
>>
>> -/*
>> - * TRACE_LIBSSH2= enables tracing in libssh2 itself.  Note
>> - * that this requires that libssh2 was specially compiled with the
>> - * `./configure --enable-debug' option, so most likely you will have
>> - * to compile it yourself.  The meaning of  is described
>> - * here: http://www.libssh2.org/libssh2_trace.html
>> +/* TRACE_LIBSSH= enables tracing in libssh itself.
>> + * The meaning of  is described here:
>> + * http://api.libssh.org/master/group__libssh__log.html
>>   */
>> -#define TRACE_LIBSSH2 0 /* or try: LIBSSH2_TRACE_SFTP */
>> +#define TRACE_LIBSSH  0 /* see: SSH_LOG_* */
>> +
>> +#define HAVE_LIBSSH_0_8 (LIBSSH_VERSION_INT >= SSH_VERSION_INT(0, 8, 0))
>
> As I noticed with ssh_get_publickey() and reading
> https://www.redhat.com/archives/libvir-list/2018-May/msg00597.html, I'm
> not convinced this definition is accurate. Used in [1].
>
>>
>>  typedef struct BDRVSSHState {
>>  /* Coroutine. */
>> @@ -58,18 +57,14 @@ typedef struct BDRVSSHState {
>>
>>  /* SSH connection. */
>>  int sock; /* socket */
>> -LIBSSH2_SESSION *session; /* ssh session */
>> -LIBSSH2_SFTP *sftp;   /* sftp session */
>> -LIBSSH2_SFTP_HANDLE *sftp_handle; /* sftp remote file handle */
>> +ssh_session session;  /* ssh session */
>> +sftp_session sftp;/* sftp session */
>> +sftp_file sftp_handle;/* sftp remote file handle */
>>
>> -/* See ssh_seek() function below. */
>> -int64_t offset;
>> -bool offset_op_read;
>> -
>> -/* File attributes at open.  We try to keep the .filesize field
>> +/* File attributes at open.  

Re: [Qemu-block] [PATCH v5 09/42] block: Include filters when freezing backing chain

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> In order to make filters work in backing chains, the associated
> functions must be able to deal with them and freeze all filter links, be
> they COW or R/W filter links.
> 
> While at it, add some comments that note which functions require their
> caller to ensure that a given child link is not frozen, and how the
> callers do so.
> 
> Signed-off-by: Max Reitz 
> ---
>   block.c | 45 -
>   1 file changed, 32 insertions(+), 13 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 8438b0699e..45882a3470 100644
> --- a/block.c
> +++ b/block.c
> @@ -2214,12 +2214,15 @@ static void bdrv_replace_child_noperm(BdrvChild 
> *child,
>* If @new_bs is not NULL, bdrv_check_perm() must be called beforehand, as 
> this
>* function uses bdrv_set_perm() to update the permissions according to the 
> new
>* reference that @new_bs gets.
> + *
> + * Callers must ensure that child->frozen is false.
>*/
>   static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
>   {
>   BlockDriverState *old_bs = child->bs;
>   uint64_t perm, shared_perm;
>   
> +/* Asserts that child->frozen == false */
>   bdrv_replace_child_noperm(child, new_bs);
>   
>   if (old_bs) {
> @@ -2360,6 +2363,7 @@ static void bdrv_detach_child(BdrvChild *child)
>   g_free(child);
>   }
>   
> +/* Callers must ensure that child->frozen is false. */

Is such a comment better than one-line extra assertion at start of the function 
body?

>   void bdrv_root_unref_child(BdrvChild *child)
>   {
>   BlockDriverState *child_bs;
> @@ -2369,6 +2373,7 @@ void bdrv_root_unref_child(BdrvChild *child)
>   bdrv_unref(child_bs);
>   }
>   
> +/* Callers must ensure that child->frozen is false. */
>   void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child)
>   {
>   if (child == NULL) {
> @@ -2435,6 +2440,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
> BlockDriverState *backing_hd,
>   }
>   
>   if (bs->backing) {
> +/* Cannot be frozen, we checked that above */
>   bdrv_unref_child(bs, bs->backing);
>   }
>   
> @@ -3908,6 +3914,7 @@ static void bdrv_close(BlockDriverState *bs)
>   
>   if (bs->drv) {
>   if (bs->drv->bdrv_close) {
> +/* Must unfreeze all children, so bdrv_unref_child() works */
>   bs->drv->bdrv_close(bs);
>   }
>   bs->drv = NULL;
> @@ -4281,17 +4288,20 @@ BlockDriverState *bdrv_find_base(BlockDriverState *bs)
>* Return true if at least one of the backing links between @bs and
>* @base is frozen. @errp is set if that's the case.
>* @base must be reachable from @bs, or NULL.
> + * (Filters are treated as normal elements of the backing chain.)
>*/
>   bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState 
> *base,
> Error **errp)
>   {
>   BlockDriverState *i;
> +BdrvChild *child;
>   
> -for (i = bs; i != base; i = backing_bs(i)) {
> -if (i->backing && i->backing->frozen) {
> +for (i = bs; i != base; i = child_bs(child)) {
> +child = bdrv_filtered_child(i);
> +
> +if (child && child->frozen) {
>   error_setg(errp, "Cannot change '%s' link from '%s' to '%s'",
> -   i->backing->name, i->node_name,
> -   backing_bs(i)->node_name);
> +   child->name, i->node_name, child->bs->node_name);
>   return true;
>   }
>   }
> @@ -4305,19 +4315,22 @@ bool bdrv_is_backing_chain_frozen(BlockDriverState 
> *bs, BlockDriverState *base,
>* none of the links are modified.
>* @base must be reachable from @bs, or NULL.
>* Returns 0 on success. On failure returns < 0 and sets @errp.
> + * (Filters are treated as normal elements of the backing chain.)
>*/
>   int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
> Error **errp)
>   {
>   BlockDriverState *i;
> +BdrvChild *child;
>   
>   if (bdrv_is_backing_chain_frozen(bs, base, errp)) {
>   return -EPERM;
>   }
>   
> -for (i = bs; i != base; i = backing_bs(i)) {
> -if (i->backing) {
> -i->backing->frozen = true;
> +for (i = bs; i != base; i = child_bs(child)) {
> +child = bdrv_filtered_child(i);
> +if (child) {
> +child->frozen = true;
>   }
>   }
>   
> @@ -4328,15 +4341,18 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, 
> BlockDriverState *base,
>* Unfreeze all backing links between @bs and @base. The caller must
>* ensure that all links are frozen before using this function.
>* @base must be reachable from @bs, or NULL.
> + * (Filters are treated as normal elements of the backing chain.)
>*/
>   void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState 
> *base)
>   {
>   

Re: [Qemu-block] [PATCH v8] ssh: switch from libssh2 to libssh

[Philippe Mathieu-Daudé]

Hi Pino,

Please Cc the different maintainers (doing that for you).

On 6/13/19 8:11 AM, Pino Toscano wrote:
> Rewrite the implementation of the ssh block driver to use libssh instead
> of libssh2.  The libssh library has various advantages over libssh2:
> - easier API for authentication (for example for using ssh-agent)
> - easier API for known_hosts handling
> - supports newer types of keys in known_hosts
> 
> Use APIs/features available in libssh 0.8 conditionally, to support
> older versions (which are not recommended though).
> 
> Adjust the various Docker/Travis scripts to use libssh when available
> instead of libssh2.

Please add a comment this patch remove the possibility to use the ssh
block driver on MinGW. Stefan only read QEMU emails during the week-end.

> Signed-off-by: Pino Toscano 
> ---
> 
> Changes from v7:
> - #if HAVE_LIBSSH_0_8 -> #ifdef HAVE_LIBSSH_0_8
> - ptrdiff_t -> size_t
> 
> Changes from v6:
> - fixed few checkpatch style issues
> - detect libssh 0.8 via symbol detection
> - adjust travis/docker test material
> - remove dead "default" case in a switch
> - use variables for storing MIN() results
> - adapt a documentation bit
> 
> Changes from v5:
> - adapt to newer tracing APIs
> - disable ssh compression (mimic what libssh2 does by default)
> - use build time checks for libssh 0.8, and use newer APIs directly
> 
> Changes from v4:
> - fix wrong usages of error_setg/session_error_setg/sftp_error_setg
> - fix few return code checks
> - remove now-unused parameters in few internal functions
> - allow authentication with "none" method
> - switch to unsigned int for the port number
> - enable TCP_NODELAY on the socket
> - fix one reference error message in iotest 207
> 
> Changes from v3:
> - fix socket cleanup in connect_to_ssh()
> - add comments about the socket cleanup
> - improve the error reporting (closer to what was with libssh2)
> - improve EOF detection on sftp_read()
> 
> Changes from v2:
> - used again an own fd
> - fixed co_yield() implementation
> 
> Changes from v1:
> - fixed jumbo packets writing
> - fixed missing 'err' assignment
> - fixed commit message
> 
>  .travis.yml   |   4 +-
>  block/Makefile.objs   |   6 +-
>  block/ssh.c   | 622 +-
>  block/trace-events|  14 +-
>  configure |  65 +-
>  docs/qemu-block-drivers.texi  |   2 +-
>  .../dockerfiles/debian-win32-cross.docker |   1 -
>  .../dockerfiles/debian-win64-cross.docker |   1 -
>  tests/docker/dockerfiles/fedora.docker|   4 +-
>  tests/docker/dockerfiles/ubuntu.docker|   2 +-
>  tests/docker/dockerfiles/ubuntu1804.docker|   2 +-
>  tests/qemu-iotests/207.out|   2 +-
>  12 files changed, 374 insertions(+), 351 deletions(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index a08a7b7278..c70dd055ed 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -31,7 +31,7 @@ addons:
>- libseccomp-dev
>- libspice-protocol-dev
>- libspice-server-dev
> -  - libssh2-1-dev
> +  - libssh-dev
>- liburcu-dev
>- libusb-1.0-0-dev
>- libvte-2.91-dev
> @@ -261,7 +261,7 @@ matrix:
>  - libseccomp-dev
>  - libspice-protocol-dev
>  - libspice-server-dev
> -- libssh2-1-dev
> +- libssh-dev
>  - liburcu-dev
>  - libusb-1.0-0-dev
>  - libvte-2.91-dev
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index ae11605c9f..bf01429dd5 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -31,7 +31,7 @@ block-obj-$(CONFIG_CURL) += curl.o
>  block-obj-$(CONFIG_RBD) += rbd.o
>  block-obj-$(CONFIG_GLUSTERFS) += gluster.o
>  block-obj-$(CONFIG_VXHS) += vxhs.o
> -block-obj-$(CONFIG_LIBSSH2) += ssh.o
> +block-obj-$(CONFIG_LIBSSH) += ssh.o
>  block-obj-y += accounting.o dirty-bitmap.o
>  block-obj-y += write-threshold.o
>  block-obj-y += backup.o
> @@ -52,8 +52,8 @@ rbd.o-libs := $(RBD_LIBS)
>  gluster.o-cflags   := $(GLUSTERFS_CFLAGS)
>  gluster.o-libs := $(GLUSTERFS_LIBS)
>  vxhs.o-libs:= $(VXHS_LIBS)
> -ssh.o-cflags   := $(LIBSSH2_CFLAGS)
> -ssh.o-libs := $(LIBSSH2_LIBS)
> +ssh.o-cflags   := $(LIBSSH_CFLAGS)
> +ssh.o-libs := $(LIBSSH_LIBS)
>  block-obj-dmg-bz2-$(CONFIG_BZIP2) += dmg-bz2.o
>  block-obj-$(if $(CONFIG_DMG),m,n) += $(block-obj-dmg-bz2-y)
>  dmg-bz2.o-libs := $(BZIP2_LIBS)
> diff --git a/block/ssh.c b/block/ssh.c
> index 6da7b9cbfe..fb458d4548 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -24,8 +24,8 @@
>  
>  #include "qemu/osdep.h"
>  
> -#include 
> -#include 
> +#include 
> +#include 
>  
>  #include "block/block_int.h"
>  #include "block/qdict.h"
> @@ -46,13 +46,11 @@
>  #include "trace.h"
>  
>  /*
> - * TRACE_LIBSSH2= enables tracing in libssh2 itself.  Note
> - * that this 

[Qemu-block] [PATCH v9] ssh: switch from libssh2 to libssh

[Pino Toscano]

Rewrite the implementation of the ssh block driver to use libssh instead
of libssh2.  The libssh library has various advantages over libssh2:
- easier API for authentication (for example for using ssh-agent)
- easier API for known_hosts handling
- supports newer types of keys in known_hosts

Use APIs/features available in libssh 0.8 conditionally, to support
older versions (which are not recommended though).

Adjust the tests according to the different error message, and to the
newer host keys (ed25519) that are used by default with OpenSSH >= 6.7
and libssh >= 0.7.0.

Adjust the various Docker/Travis scripts to use libssh when available
instead of libssh2. The mingw/mxe testing is dropped for now, as there
are no packages for it.

Signed-off-by: Pino Toscano 
Tested-by: Philippe Mathieu-Daudé 
Acked-by: Alex Bennée 
---

Changes from v8:
- use a newer key type in iotest 207
- improve the commit message

Changes from v7:
- #if HAVE_LIBSSH_0_8 -> #ifdef HAVE_LIBSSH_0_8
- ptrdiff_t -> size_t

Changes from v6:
- fixed few checkpatch style issues
- detect libssh 0.8 via symbol detection
- adjust travis/docker test material
- remove dead "default" case in a switch
- use variables for storing MIN() results
- adapt a documentation bit

Changes from v5:
- adapt to newer tracing APIs
- disable ssh compression (mimic what libssh2 does by default)
- use build time checks for libssh 0.8, and use newer APIs directly

Changes from v4:
- fix wrong usages of error_setg/session_error_setg/sftp_error_setg
- fix few return code checks
- remove now-unused parameters in few internal functions
- allow authentication with "none" method
- switch to unsigned int for the port number
- enable TCP_NODELAY on the socket
- fix one reference error message in iotest 207

Changes from v3:
- fix socket cleanup in connect_to_ssh()
- add comments about the socket cleanup
- improve the error reporting (closer to what was with libssh2)
- improve EOF detection on sftp_read()

Changes from v2:
- used again an own fd
- fixed co_yield() implementation

Changes from v1:
- fixed jumbo packets writing
- fixed missing 'err' assignment
- fixed commit message

 .travis.yml   |   4 +-
 block/Makefile.objs   |   6 +-
 block/ssh.c   | 622 +-
 block/trace-events|  14 +-
 configure |  65 +-
 docs/qemu-block-drivers.texi  |   2 +-
 .../dockerfiles/debian-win32-cross.docker |   1 -
 .../dockerfiles/debian-win64-cross.docker |   1 -
 tests/docker/dockerfiles/fedora.docker|   4 +-
 tests/docker/dockerfiles/ubuntu.docker|   2 +-
 tests/docker/dockerfiles/ubuntu1804.docker|   2 +-
 tests/qemu-iotests/207|   4 +-
 tests/qemu-iotests/207.out|   2 +-
 13 files changed, 376 insertions(+), 353 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 08502c0aa2..75f47df3d2 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -31,7 +31,7 @@ addons:
   - libseccomp-dev
   - libspice-protocol-dev
   - libspice-server-dev
-  - libssh2-1-dev
+  - libssh-dev
   - liburcu-dev
   - libusb-1.0-0-dev
   - libvte-2.91-dev
@@ -268,7 +268,7 @@ matrix:
 - libseccomp-dev
 - libspice-protocol-dev
 - libspice-server-dev
-- libssh2-1-dev
+- libssh-dev
 - liburcu-dev
 - libusb-1.0-0-dev
 - libvte-2.91-dev
diff --git a/block/Makefile.objs b/block/Makefile.objs
index ae11605c9f..bf01429dd5 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -31,7 +31,7 @@ block-obj-$(CONFIG_CURL) += curl.o
 block-obj-$(CONFIG_RBD) += rbd.o
 block-obj-$(CONFIG_GLUSTERFS) += gluster.o
 block-obj-$(CONFIG_VXHS) += vxhs.o
-block-obj-$(CONFIG_LIBSSH2) += ssh.o
+block-obj-$(CONFIG_LIBSSH) += ssh.o
 block-obj-y += accounting.o dirty-bitmap.o
 block-obj-y += write-threshold.o
 block-obj-y += backup.o
@@ -52,8 +52,8 @@ rbd.o-libs := $(RBD_LIBS)
 gluster.o-cflags   := $(GLUSTERFS_CFLAGS)
 gluster.o-libs := $(GLUSTERFS_LIBS)
 vxhs.o-libs:= $(VXHS_LIBS)
-ssh.o-cflags   := $(LIBSSH2_CFLAGS)
-ssh.o-libs := $(LIBSSH2_LIBS)
+ssh.o-cflags   := $(LIBSSH_CFLAGS)
+ssh.o-libs := $(LIBSSH_LIBS)
 block-obj-dmg-bz2-$(CONFIG_BZIP2) += dmg-bz2.o
 block-obj-$(if $(CONFIG_DMG),m,n) += $(block-obj-dmg-bz2-y)
 dmg-bz2.o-libs := $(BZIP2_LIBS)
diff --git a/block/ssh.c b/block/ssh.c
index 6da7b9cbfe..fb458d4548 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -24,8 +24,8 @@
 
 #include "qemu/osdep.h"
 
-#include 
-#include 
+#include 
+#include 
 
 #include "block/block_int.h"
 #include "block/qdict.h"
@@ -46,13 +46,11 @@
 #include "trace.h"
 
 /*
- * TRACE_LIBSSH2= enables tracing in libssh2 itself.  Note
- * that this requires that libssh2 was specially compiled with the
- * `./configure --enable-debug' option, 

Re: [Qemu-block] [PATCH v5 08/42] block: bdrv_set_backing_hd() is about bs->backing

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> bdrv_set_backing_hd() is a function that explicitly cares about the
> bs->backing child.  Highlight that in its description and use
> child_bs(bs->backing) instead of backing_bs(bs) to make it more obvious.
> 
> Signed-off-by: Max Reitz 

Reviewed-by: Vladimir Sementsov-Ogievskiy ]

> ---
>   block.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 64d6190984..8438b0699e 100644
> --- a/block.c
> +++ b/block.c
> @@ -2417,7 +2417,7 @@ static bool 
> bdrv_inherits_from_recursive(BlockDriverState *child,
>   }
>   
>   /*
> - * Sets the backing file link of a BDS. A new reference is created; callers
> + * Sets the bs->backing link of a BDS. A new reference is created; callers
>* which don't need their own reference any more must call bdrv_unref().
>*/
>   void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
> @@ -2426,7 +2426,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
> BlockDriverState *backing_hd,
>   bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
>   bdrv_inherits_from_recursive(backing_hd, bs);
>   
> -if (bdrv_is_backing_chain_frozen(bs, backing_bs(bs), errp)) {
> +if (bdrv_is_backing_chain_frozen(bs, child_bs(bs->backing), errp)) {
>   return;
>   }
>   
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v5 07/42] block: *filtered_cow_child() for *has_zero_init()

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> bdrv_has_zero_init() and the related bdrv_unallocated_blocks_are_zero()
> should use bdrv_filtered_cow_child() if they want to check whether the
> given BDS has a COW backing file.
> 
> Signed-off-by: Max Reitz 
> ---
>   block.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index be18130944..64d6190984 100644
> --- a/block.c
> +++ b/block.c
> @@ -4933,7 +4933,7 @@ int bdrv_has_zero_init(BlockDriverState *bs)
>   
>   /* If BS is a copy on write image, it is initialized to
>  the contents of the base image, which may not be zeroes.  */
> -if (bs->backing) {
> +if (bdrv_filtered_cow_child(bs)) {
>   return 0;
>   }

Hmm, if you are fixing bdrv_has_zero_init around filters, I'd prefere to fix 
the whole
function, converting the following here too:
 if (bs->file && bs->drv->is_filter) {
 return bdrv_has_zero_init(bs->file->bs);
 }


But it's not a real problem:

Reviewed-by: Vladimir Sementsov-Ogievskiy 

>   if (bs->drv->bdrv_has_zero_init) {
> @@ -4951,7 +4951,7 @@ bool bdrv_unallocated_blocks_are_zero(BlockDriverState 
> *bs)
>   {
>   BlockDriverInfo bdi;
>   
> -if (bs->backing) {
> +if (bdrv_filtered_cow_child(bs)) {
>   return false;
>   }
>   
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v5 05/42] block: Add chain helper functions

[Vladimir Sementsov-Ogievskiy]

13.06.2019 15:33, Max Reitz wrote:
> On 13.06.19 14:26, Vladimir Sementsov-Ogievskiy wrote:
>> 13.06.2019 1:09, Max Reitz wrote:
>>> Add some helper functions for skipping filters in a chain of block
>>> nodes.
>>>
>>> Signed-off-by: Max Reitz 
>>> ---
>>>include/block/block_int.h |  3 +++
>>>block.c   | 55 +++
>>>2 files changed, 58 insertions(+)
>>>
>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>>> index 7ce71623f8..875a33f255 100644
>>> --- a/include/block/block_int.h
>>> +++ b/include/block/block_int.h
>>> @@ -1264,6 +1264,9 @@ BdrvChild *bdrv_filtered_child(BlockDriverState *bs);
>>>BdrvChild *bdrv_metadata_child(BlockDriverState *bs);
>>>BdrvChild *bdrv_storage_child(BlockDriverState *bs);
>>>BdrvChild *bdrv_primary_child(BlockDriverState *bs);
>>> +BlockDriverState *bdrv_skip_implicit_filters(BlockDriverState *bs);
>>> +BlockDriverState *bdrv_skip_rw_filters(BlockDriverState *bs);
>>> +BlockDriverState *bdrv_backing_chain_next(BlockDriverState *bs);
>>>
>>>static inline BlockDriverState *child_bs(BdrvChild *child)
>>>{
>>> diff --git a/block.c b/block.c
>>> index 724d8889a6..be18130944 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -6494,3 +6494,58 @@ BdrvChild *bdrv_primary_child(BlockDriverState *bs)
>>>{
>>>return bdrv_filtered_rw_child(bs) ?: bs->file;
>>>}
>>> +
>>> +static BlockDriverState *bdrv_skip_filters(BlockDriverState *bs,
>>> +   bool stop_on_explicit_filter)
>>> +{
>>> +BdrvChild *filtered;
>>> +
>>> +if (!bs) {
>>> +return NULL;
>>> +}
>>> +
>>> +while (!(stop_on_explicit_filter && !bs->implicit)) {
>>> +filtered = bdrv_filtered_rw_child(bs);
>>> +if (!filtered) {
>>> +break;
>>> +}
>>> +bs = filtered->bs;
>>> +}
>>> +/*
>>> + * Note that this treats nodes with bs->drv == NULL
>>
>> as well as filters without filtered_rw child
> 
> A filter always must have a filtered_rw child, though.  So I don’t quite
> understand what you mean here...
> 
> Max
> 
>>as not being
>>> + * R/W filters (bs->drv == NULL should be replaced by something
>>> + * else anyway).
>>> + * The advantage of this behavior is that this function will thus
>>> + * always return a non-NULL value (given a non-NULL @bs).
>>
>> and this is the advantage of what I've written, not about bs->drv.

I mean, that advantage seems unrelated to the reason about bs->drv == NULL,
as even with bs->drv == NULL we can go to bs->backing or bs->file..

But I don't  really care, my r-b stays here anyway

>>
>>> + */
>>> +
>>> +return bs;
>>> +}
>>> +
>>> +/*
>>> + * Return the first BDS that has not been added implicitly or that
>>> + * does not have an RW-filtered child down the chain starting from @bs
>>> + * (including @bs itself).
>>> + */
>>> +BlockDriverState *bdrv_skip_implicit_filters(BlockDriverState *bs)
>>> +{
>>> +return bdrv_skip_filters(bs, true);
>>> +}
>>> +
>>> +/*
>>> + * Return the first BDS that does not have an RW-filtered child down
>>> + * the chain starting from @bs (including @bs itself).
>>> + */
>>> +BlockDriverState *bdrv_skip_rw_filters(BlockDriverState *bs)
>>> +{
>>> +return bdrv_skip_filters(bs, false);
>>> +}
>>> +
>>> +/*
>>> + * For a backing chain, return the first non-filter backing image of
>>> + * the first non-filter image.
>>> + */
>>> +BlockDriverState *bdrv_backing_chain_next(BlockDriverState *bs)
>>> +{
>>> +return 
>>> bdrv_skip_rw_filters(bdrv_filtered_cow_bs(bdrv_skip_rw_filters(bs)));
>>> +}
>>>
>>
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy 
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v5 05/42] block: Add chain helper functions

[Max Reitz]

On 13.06.19 14:26, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 1:09, Max Reitz wrote:
>> Add some helper functions for skipping filters in a chain of block
>> nodes.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>   include/block/block_int.h |  3 +++
>>   block.c   | 55 +++
>>   2 files changed, 58 insertions(+)
>>
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 7ce71623f8..875a33f255 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -1264,6 +1264,9 @@ BdrvChild *bdrv_filtered_child(BlockDriverState *bs);
>>   BdrvChild *bdrv_metadata_child(BlockDriverState *bs);
>>   BdrvChild *bdrv_storage_child(BlockDriverState *bs);
>>   BdrvChild *bdrv_primary_child(BlockDriverState *bs);
>> +BlockDriverState *bdrv_skip_implicit_filters(BlockDriverState *bs);
>> +BlockDriverState *bdrv_skip_rw_filters(BlockDriverState *bs);
>> +BlockDriverState *bdrv_backing_chain_next(BlockDriverState *bs);
>>   
>>   static inline BlockDriverState *child_bs(BdrvChild *child)
>>   {
>> diff --git a/block.c b/block.c
>> index 724d8889a6..be18130944 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -6494,3 +6494,58 @@ BdrvChild *bdrv_primary_child(BlockDriverState *bs)
>>   {
>>   return bdrv_filtered_rw_child(bs) ?: bs->file;
>>   }
>> +
>> +static BlockDriverState *bdrv_skip_filters(BlockDriverState *bs,
>> +   bool stop_on_explicit_filter)
>> +{
>> +BdrvChild *filtered;
>> +
>> +if (!bs) {
>> +return NULL;
>> +}
>> +
>> +while (!(stop_on_explicit_filter && !bs->implicit)) {
>> +filtered = bdrv_filtered_rw_child(bs);
>> +if (!filtered) {
>> +break;
>> +}
>> +bs = filtered->bs;
>> +}
>> +/*
>> + * Note that this treats nodes with bs->drv == NULL
> 
> as well as filters without filtered_rw child

A filter always must have a filtered_rw child, though.  So I don’t quite
understand what you mean here...

Max

>   as not being
>> + * R/W filters (bs->drv == NULL should be replaced by something
>> + * else anyway).
>> + * The advantage of this behavior is that this function will thus
>> + * always return a non-NULL value (given a non-NULL @bs).
> 
> and this is the advantage of what I've written, not about bs->drv.
> 
>> + */
>> +
>> +return bs;
>> +}
>> +
>> +/*
>> + * Return the first BDS that has not been added implicitly or that
>> + * does not have an RW-filtered child down the chain starting from @bs
>> + * (including @bs itself).
>> + */
>> +BlockDriverState *bdrv_skip_implicit_filters(BlockDriverState *bs)
>> +{
>> +return bdrv_skip_filters(bs, true);
>> +}
>> +
>> +/*
>> + * Return the first BDS that does not have an RW-filtered child down
>> + * the chain starting from @bs (including @bs itself).
>> + */
>> +BlockDriverState *bdrv_skip_rw_filters(BlockDriverState *bs)
>> +{
>> +return bdrv_skip_filters(bs, false);
>> +}
>> +
>> +/*
>> + * For a backing chain, return the first non-filter backing image of
>> + * the first non-filter image.
>> + */
>> +BlockDriverState *bdrv_backing_chain_next(BlockDriverState *bs)
>> +{
>> +return 
>> bdrv_skip_rw_filters(bdrv_filtered_cow_bs(bdrv_skip_rw_filters(bs)));
>> +}
>>
> 
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 06/42] qcow2: Implement .bdrv_storage_child()

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> Signed-off-by: Max Reitz 


Reviewed-by: Vladimir Sementsov-Ogievskiy 

> ---
>   block/qcow2.c | 9 +
>   1 file changed, 9 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 9396d490d5..57675c9416 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -5085,6 +5085,13 @@ void qcow2_signal_corruption(BlockDriverState *bs, 
> bool fatal, int64_t offset,
>   s->signaled_corruption = true;
>   }
>   
> +static BdrvChild *qcow2_storage_child(BlockDriverState *bs)
> +{
> +BDRVQcow2State *s = bs->opaque;
> +
> +return s->data_file;
> +}
> +
>   static QemuOptsList qcow2_create_opts = {
>   .name = "qcow2-create-opts",
>   .head = QTAILQ_HEAD_INITIALIZER(qcow2_create_opts.head),
> @@ -5231,6 +5238,8 @@ BlockDriver bdrv_qcow2 = {
>   .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw,
>   .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap,
>   .bdrv_remove_persistent_dirty_bitmap = 
> qcow2_remove_persistent_dirty_bitmap,
> +
> +.bdrv_storage_child = qcow2_storage_child,
>   };
>   
>   static void bdrv_qcow2_init(void)
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v5 05/42] block: Add chain helper functions

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> Add some helper functions for skipping filters in a chain of block
> nodes.
> 
> Signed-off-by: Max Reitz 
> ---
>   include/block/block_int.h |  3 +++
>   block.c   | 55 +++
>   2 files changed, 58 insertions(+)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 7ce71623f8..875a33f255 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1264,6 +1264,9 @@ BdrvChild *bdrv_filtered_child(BlockDriverState *bs);
>   BdrvChild *bdrv_metadata_child(BlockDriverState *bs);
>   BdrvChild *bdrv_storage_child(BlockDriverState *bs);
>   BdrvChild *bdrv_primary_child(BlockDriverState *bs);
> +BlockDriverState *bdrv_skip_implicit_filters(BlockDriverState *bs);
> +BlockDriverState *bdrv_skip_rw_filters(BlockDriverState *bs);
> +BlockDriverState *bdrv_backing_chain_next(BlockDriverState *bs);
>   
>   static inline BlockDriverState *child_bs(BdrvChild *child)
>   {
> diff --git a/block.c b/block.c
> index 724d8889a6..be18130944 100644
> --- a/block.c
> +++ b/block.c
> @@ -6494,3 +6494,58 @@ BdrvChild *bdrv_primary_child(BlockDriverState *bs)
>   {
>   return bdrv_filtered_rw_child(bs) ?: bs->file;
>   }
> +
> +static BlockDriverState *bdrv_skip_filters(BlockDriverState *bs,
> +   bool stop_on_explicit_filter)
> +{
> +BdrvChild *filtered;
> +
> +if (!bs) {
> +return NULL;
> +}
> +
> +while (!(stop_on_explicit_filter && !bs->implicit)) {
> +filtered = bdrv_filtered_rw_child(bs);
> +if (!filtered) {
> +break;
> +}
> +bs = filtered->bs;
> +}
> +/*
> + * Note that this treats nodes with bs->drv == NULL

as well as filters without filtered_rw child

  as not being
> + * R/W filters (bs->drv == NULL should be replaced by something
> + * else anyway).
> + * The advantage of this behavior is that this function will thus
> + * always return a non-NULL value (given a non-NULL @bs).

and this is the advantage of what I've written, not about bs->drv.

> + */
> +
> +return bs;
> +}
> +
> +/*
> + * Return the first BDS that has not been added implicitly or that
> + * does not have an RW-filtered child down the chain starting from @bs
> + * (including @bs itself).
> + */
> +BlockDriverState *bdrv_skip_implicit_filters(BlockDriverState *bs)
> +{
> +return bdrv_skip_filters(bs, true);
> +}
> +
> +/*
> + * Return the first BDS that does not have an RW-filtered child down
> + * the chain starting from @bs (including @bs itself).
> + */
> +BlockDriverState *bdrv_skip_rw_filters(BlockDriverState *bs)
> +{
> +return bdrv_skip_filters(bs, false);
> +}
> +
> +/*
> + * For a backing chain, return the first non-filter backing image of
> + * the first non-filter image.
> + */
> +BlockDriverState *bdrv_backing_chain_next(BlockDriverState *bs)
> +{
> +return 
> bdrv_skip_rw_filters(bdrv_filtered_cow_bs(bdrv_skip_rw_filters(bs)));
> +}
> 


Reviewed-by: Vladimir Sementsov-Ogievskiy 

-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v5 04/42] block: Add child access functions

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> There are BDS children that the general block layer code can access,
> namely bs->file and bs->backing.  Since the introduction of filters and
> external data files, their meaning is not quite clear.  bs->backing can
> be a COW source, or it can be an R/W-filtered child; bs->file can be an
> R/W-filtered child, it can be data and metadata storage, or it can be
> just metadata storage.
> 
> This overloading really is not helpful.  This patch adds function that
> retrieve the correct child for each exact purpose.  Later patches in
> this series will make use of them.  Doing so will allow us to handle
> filter nodes and external data files in a meaningful way.
> 
> Signed-off-by: Max Reitz 

Reviewed-by: Vladimir Sementsov-Ogievskiy 

> ---
>   include/block/block_int.h | 57 --
>   block.c   | 99 +++
>   2 files changed, 153 insertions(+), 3 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 58fca37ba3..7ce71623f8 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h

[..]

>   
>   typedef struct BlockLimits {
> @@ -1249,4 +1258,46 @@ int coroutine_fn bdrv_co_copy_range_to(BdrvChild *src, 
> uint64_t src_offset,
>   
>   int refresh_total_sectors(BlockDriverState *bs, int64_t hint);
>   
> +BdrvChild *bdrv_filtered_cow_child(BlockDriverState *bs);
> +BdrvChild *bdrv_filtered_rw_child(BlockDriverState *bs);
> +BdrvChild *bdrv_filtered_child(BlockDriverState *bs);
> +BdrvChild *bdrv_metadata_child(BlockDriverState *bs);
> +BdrvChild *bdrv_storage_child(BlockDriverState *bs);
> +BdrvChild *bdrv_primary_child(BlockDriverState *bs);
> +

Wow! Such a big family :)

I'd like to put them into a table, just for me to make it easier to keep it all 
in mind.
But if you want, you may include it here as a comment.. But it's difficult to 
keep it less than 80 columns.
I think, I'll modify it after reviewing following patches.

+++---+---+
| child  | description| filter node 
  | format node   |
+++---+---+
| filtered_cow_child | for COW/COR| NULL
  | bs->backing   |
+++---+---+
| filtered_rw_child  | for IO pass-through| bs->backing or bs->file 
  | NULL  |
||| (only one may exist)
  |   |
+++---+---+
| filtered_child | one of the previous| 
  |   |
|| for extended backing   | filtered_rw_child   
  | filtered_cow_child|
|| chain  | 
  |   |
+++---+---+
| metadata_child | where metadata is stored   | NULL
  | bs->file  |
+++---+---+
| storage_child  | where actual guest visible | 
bs->drv->bdrv_storage_child() | bs->drv->bdrv_storage_child() |
|| data is stored | or filtered_rw_child
  | or bs->file   |
+++---+---+
| primary_child  | don't know yet | filtered_rw_child   
  | bs->file  |
+++---+---+




-- 
Best regards,
Vladimir

Re: [Qemu-block] [QEMU] [PATCH v2 0/8] Add Qemu to SeaBIOS LCHS interface

[Sam Eiderman]

> On 13 Jun 2019, at 12:38, Gerd Hoffmann  wrote:
> 
>  Hi,
> 
>> Yes they are pretty rare.
>> Windows 2000 and Windows XP guests migrated from VMware to Qemu/KVM
>> would not boot due to incorrect disk geometries (some had 32/56 spt instead 
>> of
>> 56. Also number of heads was not entirely correct)
> 
> Ok.
> 
>>> Why?  Asking the user to deal with the mess is pretty lame if there are
>>> better options.  And IMO doing this fully automatic in seabios is
>>> better.
>> 
>> I’m not against an automatic approach, however I do think that doing this
>> in SeaBIOS might break compatibility for already existing guests that will
>> suddenly see different LCHS values. (Explanation below)
> 
>>> I can't see how this can break guests.  It should either have no effect
>>> (guests using LBA) or unbreak guests due to LCHS changing from "wrong"
>>> to "correct”.
>> 
>> I’m not sure what do you mean by "unbreak guests” if you change an existing
>> guest that uses LCHS from 56 spt to LBA (63 spt) it will stop booting.
> 
> Well, that LCHS change happens because you move the guest from vmware to
> qemu and seabios uses 63 spt no matter what if the disk is too big for
> chs addressing.
> 
> When seabios is changed to look at the MBR to figure what the lchs of
> the disk is that will make your guest boot.

See below

> 
>> Your guessing algorithm will have to guess 56, if it will fail guessing 56 
>> correctly,
>> the user can not perform any action beside downgrading SeaBIOS in order to 
>> run
>> the guest.
> 
> Sure, if the guess is wrong then the guest will not boot.  That isn't
> worse than the situation we have today where seabios will not even try
> to figure what the lchs of the disk is.
> 
> And, no, downgrading seabios will not make your vmware guest with 56 spt
> boot.

I’m not talking about the vmware case here.
If you introduce MBR guessing into SeaBIOS and change its default behaviour you
risk making operating systems such as Windows XP / 2003 / 2000 created on
QEMU to not work anymore.

Example:

Consider a Windows XP that works with the following geometries on 
standard
QEMU/SeaBIOS today:

Disk is very large, therefore INT13 AH=02:

255 heads, 63 spt

Now you change SeaBIOS to guess from the MBR.
In some cases the MBR guess can be incorrect so now SeaBIOS will guess:

255 heads, 62 spt

The guest no longer boots with these geometries and you broke 
compatibility.

Can there be a guest that will fail the MBR in such a way? Yes.
Look at the following MBR partition table of a Windows XP guest in our 
production
environment:

Disk size in sectors: 16777216

Binary (only one partition 16 bytes): 80 01 01 00 07 fe ff ff 3f 00 00 00 d5 ea 
ff 00
Start: (0, 1, 1, 63)
End: (1023, 254, 63, 16771859)

As can be easily seen, any MBR guessing algorithm should guess:

255 heads (since a value of 254 appears), 63 spt (since a value of 63 
appears)

Turns out that this image does not work with 255, 63 but actually requires

16 heads, 63 spt

to boot.

So relying on MBR partitions alone is not always enough and sometimes manual 
intervention
is required.

(VMware solves this by specifying 16 heads, 63 spt in the descriptor file and 
overrides its
default guessing algorithm which also fails here)

(By the way this is not a VMware specific problem, the disk itself was imported 
to VMware in
a P2V scenario, so that probably explains why the ddb.geometry.bios* values 
appear in the
VMDK in the first place)


> 
> cheers,
>  Gerd
> 

Re: [Qemu-block] [PATCH v5 02/42] copy-on-read: Support compressed writes

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> Signed-off-by: Max Reitz 

Reviewed-by: Vladimir Sementsov-Ogievskiy 

> ---
>   block/copy-on-read.c | 11 +++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
> index 53972b1da3..88e1c1f538 100644
> --- a/block/copy-on-read.c
> +++ b/block/copy-on-read.c
> @@ -114,6 +114,16 @@ static int coroutine_fn cor_co_pdiscard(BlockDriverState 
> *bs,
>   }
>   
>   
> +static int coroutine_fn cor_co_pwritev_compressed(BlockDriverState *bs,
> +  uint64_t offset,
> +  uint64_t bytes,
> +  QEMUIOVector *qiov)
> +{
> +return bdrv_co_pwritev(bs->file, offset, bytes, qiov,
> +   BDRV_REQ_WRITE_COMPRESSED);
> +}

Hmm, possibly it's better to handle support of compression by checking supported
flags

> +
> +
>   static void cor_eject(BlockDriverState *bs, bool eject_flag)
>   {
>   bdrv_eject(bs->file->bs, eject_flag);
> @@ -146,6 +156,7 @@ static BlockDriver bdrv_copy_on_read = {
>   .bdrv_co_pwritev= cor_co_pwritev,
>   .bdrv_co_pwrite_zeroes  = cor_co_pwrite_zeroes,
>   .bdrv_co_pdiscard   = cor_co_pdiscard,
> +.bdrv_co_pwritev_compressed = cor_co_pwritev_compressed,
>   
>   .bdrv_eject = cor_eject,
>   .bdrv_lock_medium   = cor_lock_medium,
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v5 01/42] block: Mark commit and mirror as filter drivers

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> The commit and mirror block nodes are filters, so they should be marked
> as such.  (Strictly speaking, BDS.is_filter's documentation states that
> a filter's child must be bs->file.  The following patch will relax this
> restriction, however.)
> 
> Signed-off-by: Max Reitz 
> Reviewed-by: Alberto Garcia 
> Reviewed-by: Eric Blake 

Reviewed-by: Vladimir Sementsov-Ogievskiy 

> ---
>   block/commit.c | 2 ++
>   block/mirror.c | 2 ++
>   2 files changed, 4 insertions(+)
> 
> diff --git a/block/commit.c b/block/commit.c
> index c815def89a..f20a26fecd 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -256,6 +256,8 @@ static BlockDriver bdrv_commit_top = {
>   .bdrv_co_block_status   = bdrv_co_block_status_from_backing,
>   .bdrv_refresh_filename  = bdrv_commit_top_refresh_filename,
>   .bdrv_child_perm= bdrv_commit_top_child_perm,
> +
> +.is_filter  = true,
>   };
>   
>   void commit_start(const char *job_id, BlockDriverState *bs,
> diff --git a/block/mirror.c b/block/mirror.c
> index f8bdb5b21b..4fa8f57c80 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1480,6 +1480,8 @@ static BlockDriver bdrv_mirror_top = {
>   .bdrv_co_block_status   = bdrv_co_block_status_from_backing,
>   .bdrv_refresh_filename  = bdrv_mirror_top_refresh_filename,
>   .bdrv_child_perm= bdrv_mirror_top_child_perm,
> +
> +.is_filter  = true,
>   };
>   
>   static void mirror_start_job(const char *job_id, BlockDriverState *bs,
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v5 03/42] throttle: Support compressed writes

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> Signed-off-by: Max Reitz 

Reviewed-by: Vladimir Sementsov-Ogievskiy 

> ---
>   block/throttle.c | 10 ++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/block/throttle.c b/block/throttle.c
> index f64dcc27b9..de1b6bd7e8 100644
> --- a/block/throttle.c
> +++ b/block/throttle.c
> @@ -152,6 +152,15 @@ static int coroutine_fn 
> throttle_co_pdiscard(BlockDriverState *bs,
>   return bdrv_co_pdiscard(bs->file, offset, bytes);
>   }
>   
> +static int coroutine_fn throttle_co_pwritev_compressed(BlockDriverState *bs,
> +   uint64_t offset,
> +   uint64_t bytes,
> +   QEMUIOVector *qiov)
> +{
> +return throttle_co_pwritev(bs, offset, bytes, qiov,
> +   BDRV_REQ_WRITE_COMPRESSED);
> +}
> +
>   static int throttle_co_flush(BlockDriverState *bs)
>   {
>   return bdrv_co_flush(bs->file->bs);
> @@ -250,6 +259,7 @@ static BlockDriver bdrv_throttle = {
>   
>   .bdrv_co_pwrite_zeroes  =   throttle_co_pwrite_zeroes,
>   .bdrv_co_pdiscard   =   throttle_co_pdiscard,
> +.bdrv_co_pwritev_compressed =   throttle_co_pwritev_compressed,
>   
>   .bdrv_recurse_is_first_non_filter   =   
> throttle_recurse_is_first_non_filter,
>   
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v2 6/7] iotests: amend QEMU NBD process synchronization

[Vladimir Sementsov-Ogievskiy]

11.06.2019 21:02, Andrey Shinkevich wrote:
> Processes are dying harder under the Valgring. It results in counting
> the dying process as a newborn one. Make it sure that old NBD job get
> finished before starting a new one.
> 
> Suggested-by: Roman Kagan 
> Signed-off-by: Andrey Shinkevich 
> ---
>   tests/qemu-iotests/common.nbd | 6 ++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
> index 25fc9ff..e3dcc60 100644
> --- a/tests/qemu-iotests/common.nbd
> +++ b/tests/qemu-iotests/common.nbd
> @@ -22,6 +22,7 @@
>   nbd_unix_socket="${TEST_DIR}/qemu-nbd.sock"
>   nbd_tcp_addr="127.0.0.1"
>   nbd_pid_file="${TEST_DIR}/qemu-nbd.pid"
> +nbd_job_pid=""
>   
>   nbd_server_stop()
>   {
> @@ -33,6 +34,9 @@ nbd_server_stop()
>   kill "$NBD_PID"
>   fi
>   fi

Honestly, I don't understand the problem from commit message, but anyway comment
should be added here, to mark that this is for valgrind... But you don't check 
for
VALGRIND enabled.. I it intentional?

> +if [ -n "$nbd_job_pid" ] && kill -s 0 "$nbd_job_pid" 2>/dev/null; then
> +wait "$nbd_job_pid"
> +fi
>   rm -f "$nbd_unix_socket"
>   }
>   
> @@ -61,6 +65,7 @@ nbd_server_start_unix_socket()
>   {
>   nbd_server_stop
>   $QEMU_NBD -v -t -k "$nbd_unix_socket" "$@" &
> +nbd_job_pid=$!
>   nbd_server_wait_for_unix_socket $!
>   }
>   
> @@ -105,5 +110,6 @@ nbd_server_start_tcp_socket()
>   {
>   nbd_server_stop
>   $QEMU_NBD -v -t -b $nbd_tcp_addr -p $nbd_tcp_port "$@" &
> +nbd_job_pid=$!
>   nbd_server_wait_for_tcp_socket $!
>   }
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v2 7/7] iotests: new file to suppress Valgrind errors

[Vladimir Sementsov-Ogievskiy]

11.06.2019 21:02, Andrey Shinkevich wrote:
> The Valgrind tool reports about an uninitialised memory usage when the
> initialization is actually not needed. For example, the buffer 'buf'
> instantiated on a stack of the function guess_disk_lchs().

for convinience, you may add: "of the function guess_disk_lchs(), which
is then actually initialized by blk_pread_unthrottled()"

> Let's use the Valgrind technology to suppress the unwanted reports by
> adding the Valgrind specific format file valgrind.supp to the QEMU
> project. The file content is extendable for future needs.
> 
> Signed-off-by: Andrey Shinkevich 
> ---
>   tests/qemu-iotests/common.rc |  5 -
>   tests/qemu-iotests/valgrind.supp | 31 +++
>   2 files changed, 35 insertions(+), 1 deletion(-)
>   create mode 100644 tests/qemu-iotests/valgrind.supp
> 
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 3caaca4..9b74890 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -17,6 +17,8 @@
>   # along with this program.  If not, see .
>   #
>   
> +readonly VALGRIND_SUPPRESS_ERRORS=./valgrind.supp

Why readonly?

I think it should be defined near and in same manner as VALGRIND_LOGFILE,
with use of TEST_DIR

> +
>   SED=
>   for sed in sed gsed; do
>   ($sed --version | grep 'GNU sed') > /dev/null 2>&1
> @@ -65,7 +67,8 @@ _qemu_proc_wrapper()
>   local VALGRIND_LOGFILE="$1"
>   shift
>   if [ "${VALGRIND_QEMU}" == "y" ]; then
> -exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 
> "$@"
> +exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 \
> +--suppressions="${VALGRIND_SUPPRESS_ERRORS}" "$@"
>   else
>   exec "$@"
>   fi
> diff --git a/tests/qemu-iotests/valgrind.supp 
> b/tests/qemu-iotests/valgrind.supp
> new file mode 100644
> index 000..78215b6
> --- /dev/null
> +++ b/tests/qemu-iotests/valgrind.supp
> @@ -0,0 +1,31 @@
> +#
> +# Valgrind errors suppression file for QEMU iotests
> +#
> +# Copyright (c) 2019 Virtuozzo International GmbH
> +#
> +# 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 .
> +#
> +{
> +   hw/block/hd-geometry.c
> +   Memcheck:Cond
> +   fun:guess_disk_lchs
> +   fun:hd_geometry_guess
> +   fun:blkconf_geometry
> +   ...
> +   fun:device_set_realized
> +   fun:property_set_bool
> +   fun:object_property_set
> +   fun:object_property_set_qobject
> +   fun:object_property_set_bool
> +}
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v2 5/7] iotests: extend sleeping time under Valgrind

[Vladimir Sementsov-Ogievskiy]

11.06.2019 21:02, Andrey Shinkevich wrote:
> To synchronize the time when QEMU is running longer under the Valgrind,
> increase the sleeping time int the test 247.

in the test

> 
> Signed-off-by: Andrey Shinkevich 

Reviewed-by: Vladimir Sementsov-Ogievskiy 

> ---
>   tests/qemu-iotests/247 | 6 +-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/247 b/tests/qemu-iotests/247
> index 546a794..1036a17 100755
> --- a/tests/qemu-iotests/247
> +++ b/tests/qemu-iotests/247
> @@ -57,7 +57,11 @@ TEST_IMG="$TEST_IMG.4" _make_test_img $size
>   {"execute":"block-commit",
>"arguments":{"device":"format-4", "top-node": "format-2", 
> "base-node":"format-0", "job-id":"job0"}}
>   EOF
> -sleep 1
> +if [ "${VALGRIND_QEMU}" == "y" ]; then
> +sleep 5
> +else
> +sleep 1
> +fi
>   echo '{"execute":"quit"}'
>   ) | $QEMU -qmp stdio -nographic -nodefaults \
>   -blockdev file,node-name=file-0,filename=$TEST_IMG.0,auto-read-only=on \
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v2 4/7] iotests: extended timeout under Valgrind

[Vladimir Sementsov-Ogievskiy]

11.06.2019 21:02, Andrey Shinkevich wrote:
> As the iotests run longer under the Valgrind, the QEMU_COMM_TIMEOUT is
> to be increased in the test cases 028, 183 and 192 when running under
> the Valgrind.
> 
> Suggested-by: Roman Kagan 
> Signed-off-by: Andrey Shinkevich 



Reviewed-by: Vladimir Sementsov-Ogievskiy 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v2 3/7] iotests: Valgrind fails to work with nonexistent directory

[Vladimir Sementsov-Ogievskiy]

11.06.2019 21:02, Andrey Shinkevich wrote:
> The Valgrind uses the exported variable TMPDIR and fails if the
> directory does not exist. Let us exclude such a test case from
> being run under the Valgrind.
> 
> Signed-off-by: Andrey Shinkevich 

Hmm, isn't it preferable to skip unsupported by
valgrind iotests, instead silently disable valgrind in them?

> ---
>   tests/qemu-iotests/051 | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
> index 200660f..ccc5bc2 100755
> --- a/tests/qemu-iotests/051
> +++ b/tests/qemu-iotests/051
> @@ -377,6 +377,7 @@ printf %b "qemu-io $device_id \"write -P 0x33 0 
> 4k\"\ncommit $device_id\n" |
>   $QEMU_IO -c "read -P 0x33 0 4k" "$TEST_IMG" | _filter_qemu_io
>   
>   # Using snapshot=on with a non-existent TMPDIR

(you can add into comment: "Valgrind needs TMPDIR, so disable it"

> +VALGRIND_QEMU="" \
>   TMPDIR=/nonexistent run_qemu -drive driver=null-co,snapshot=on
>   
>   # Using snapshot=on together with read-only=on
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v2 2/7] iotests: exclude killed processes from running under Valgrind

[Vladimir Sementsov-Ogievskiy]

11.06.2019 21:02, Andrey Shinkevich wrote:
> The Valgrind tool fails to manage its termination when QEMU raises the
> signal SIGKILL. Lets exclude such test cases from running under the
> Valgrind because there is no sense to check memory issues that way.
> 
> Signed-off-by: Andrey Shinkevich 
> ---
>   tests/qemu-iotests/039 | 5 +
>   tests/qemu-iotests/061 | 2 ++
>   tests/qemu-iotests/137 | 1 +
>   3 files changed, 8 insertions(+)
> 
> diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
> index 0d4e963..95115e2 100755
> --- a/tests/qemu-iotests/039
> +++ b/tests/qemu-iotests/039
> @@ -65,6 +65,7 @@ echo "== Creating a dirty image file =="
>   IMGOPTS="compat=1.1,lazy_refcounts=on"
>   _make_test_img $size
>   
> +VALGRIND_QEMU="" \


Shouldn't it be written once per test, just without "\" ?

>   $QEMU_IO -c "write -P 0x5a 0 512" \
>-c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
>   | _filter_qemu_io
> @@ -100,6 +101,7 @@ echo "== Opening a dirty image read/write should repair 
> it =="
>   IMGOPTS="compat=1.1,lazy_refcounts=on"
>   _make_test_img $size
>   
> +VALGRIND_QEMU="" \
>   $QEMU_IO -c "write -P 0x5a 0 512" \
>-c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
>   | _filter_qemu_io
> @@ -118,6 +120,7 @@ echo "== Creating an image file with lazy_refcounts=off 
> =="
>   IMGOPTS="compat=1.1,lazy_refcounts=off"
>   _make_test_img $size
>   
> +VALGRIND_QEMU="" \
>   $QEMU_IO -c "write -P 0x5a 0 512" \
>-c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
>   | _filter_qemu_io
> @@ -151,6 +154,7 @@ echo "== Changing lazy_refcounts setting at runtime =="
>   IMGOPTS="compat=1.1,lazy_refcounts=off"
>   _make_test_img $size
>   
> +VALGRIND_QEMU="" \
>   $QEMU_IO -c "reopen -o lazy-refcounts=on" \
>-c "write -P 0x5a 0 512" \
>-c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
> @@ -163,6 +167,7 @@ _check_test_img
>   IMGOPTS="compat=1.1,lazy_refcounts=on"
>   _make_test_img $size
>   
> +VALGRIND_QEMU="" \
>   $QEMU_IO -c "reopen -o lazy-refcounts=off" \
>-c "write -P 0x5a 0 512" \
>-c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
> diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
> index d7dbd7e..5d0724c 100755
> --- a/tests/qemu-iotests/061
> +++ b/tests/qemu-iotests/061
> @@ -73,6 +73,7 @@ echo
>   echo "=== Testing dirty version downgrade ==="
>   echo
>   IMGOPTS="compat=1.1,lazy_refcounts=on" _make_test_img 64M
> +VALGRIND_QEMU="" \
>   $QEMU_IO -c "write -P 0x2a 0 128k" -c flush \
>-c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 | _filter_qemu_io
>   $PYTHON qcow2.py "$TEST_IMG" dump-header
> @@ -107,6 +108,7 @@ echo
>   echo "=== Testing dirty lazy_refcounts=off ==="
>   echo
>   IMGOPTS="compat=1.1,lazy_refcounts=on" _make_test_img 64M
> +VALGRIND_QEMU="" \
>   $QEMU_IO -c "write -P 0x2a 0 128k" -c flush \
>-c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 | _filter_qemu_io
>   $PYTHON qcow2.py "$TEST_IMG" dump-header
> diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137
> index 0c3d2a1..a442fc8 100755
> --- a/tests/qemu-iotests/137
> +++ b/tests/qemu-iotests/137
> @@ -130,6 +130,7 @@ echo
>   
>   # Whether lazy-refcounts was actually enabled can easily be tested: Check if
>   # the dirty bit is set after a crash
> +VALGRIND_QEMU="" \
>   $QEMU_IO \
>   -c "reopen -o lazy-refcounts=on,overlap-check=blubb" \
>   -c "write -P 0x5a 0 512" \
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [QEMU] [PATCH v2 0/8] Add Qemu to SeaBIOS LCHS interface

[Gerd Hoffmann]

  Hi,

> Yes they are pretty rare.
> Windows 2000 and Windows XP guests migrated from VMware to Qemu/KVM
> would not boot due to incorrect disk geometries (some had 32/56 spt instead of
> 56. Also number of heads was not entirely correct)

Ok.

> > Why?  Asking the user to deal with the mess is pretty lame if there are
> > better options.  And IMO doing this fully automatic in seabios is
> > better.
> 
> I’m not against an automatic approach, however I do think that doing this
> in SeaBIOS might break compatibility for already existing guests that will
> suddenly see different LCHS values. (Explanation below)

> > I can't see how this can break guests.  It should either have no effect
> > (guests using LBA) or unbreak guests due to LCHS changing from "wrong"
> > to "correct”.
> 
> I’m not sure what do you mean by "unbreak guests” if you change an existing
> guest that uses LCHS from 56 spt to LBA (63 spt) it will stop booting.

Well, that LCHS change happens because you move the guest from vmware to
qemu and seabios uses 63 spt no matter what if the disk is too big for
chs addressing.

When seabios is changed to look at the MBR to figure what the lchs of
the disk is that will make your guest boot.

> Your guessing algorithm will have to guess 56, if it will fail guessing 56 
> correctly,
> the user can not perform any action beside downgrading SeaBIOS in order to run
> the guest.

Sure, if the guess is wrong then the guest will not boot.  That isn't
worse than the situation we have today where seabios will not even try
to figure what the lchs of the disk is.

And, no, downgrading seabios will not make your vmware guest with 56 spt
boot.

cheers,
  Gerd

Re: [Qemu-block] [PATCH v2 1/7] iotests: allow Valgrind checking all QEMU processes

[Vladimir Sementsov-Ogievskiy]

11.06.2019 21:02, Andrey Shinkevich wrote:
> With the '-valgrind' option, let all the QEMU processes be run under
> the Valgrind tool. The Valgrind own parameters may be set with its
> environment variable VALGRIND_OPTS, e.g.
> VALGRIND_OPTS="--leak-check=yes" ./check -qcow2 -valgrind 
> or they may be listed in the Valgrind checked file ./.valgrindrc or
> ~/.valgrindrc like
> --memcheck:leak-check=no
> --memcheck:track-origins=yes
> After including the Valgrind into the QEMU processes wrappers in the
> common.rc script, the benchmark output for the tests 039 061 137 is to
> be amended.

If tests outputs changed, please describe here in short: why?


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v2 1/7] iotests: allow Valgrind checking all QEMU processes

[Vladimir Sementsov-Ogievskiy]

11.06.2019 21:02, Andrey Shinkevich wrote:
> With the '-valgrind' option, let all the QEMU processes be run under
> the Valgrind tool. The Valgrind own parameters may be set with its
> environment variable VALGRIND_OPTS, e.g.
> VALGRIND_OPTS="--leak-check=yes" ./check -qcow2 -valgrind 
> or they may be listed in the Valgrind checked file ./.valgrindrc or
> ~/.valgrindrc like
> --memcheck:leak-check=no
> --memcheck:track-origins=yes
> After including the Valgrind into the QEMU processes wrappers in the
> common.rc script, the benchmark output for the tests 039 061 137 is to
> be amended.
> 
> Signed-off-by: Andrey Shinkevich 
> ---
>   tests/qemu-iotests/039.out   | 30 
>   tests/qemu-iotests/061.out   | 12 ++--
>   tests/qemu-iotests/137.out   |  6 +---
>   tests/qemu-iotests/common.rc | 65 
> 
>   4 files changed, 56 insertions(+), 57 deletions(-)
> 
> diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
> index 724d7b2..972c6c0 100644
> --- a/tests/qemu-iotests/039.out
> +++ b/tests/qemu-iotests/039.out
> @@ -11,11 +11,7 @@ No errors were found on the image.
>   Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>   wrote 512/512 bytes at offset 0
>   512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -./common.rc: Killed  ( if [ "${VALGRIND_QEMU}" == "y" ]; then
> -exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 
> "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -else
> -exec "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -fi )
> +./common.rc: Killed  ( _qemu_proc_wrapper 
> "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" )
>   incompatible_features 0x1
>   ERROR cluster 5 refcount=0 reference=1
>   ERROR OFLAG_COPIED data cluster: l2_entry=8005 refcount=0
> @@ -50,11 +46,7 @@ read 512/512 bytes at offset 0
>   Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>   wrote 512/512 bytes at offset 0
>   512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -./common.rc: Killed  ( if [ "${VALGRIND_QEMU}" == "y" ]; then
> -exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 
> "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -else
> -exec "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -fi )
> +./common.rc: Killed  ( _qemu_proc_wrapper 
> "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" )
>   incompatible_features 0x1
>   ERROR cluster 5 refcount=0 reference=1
>   Rebuilding refcount structure
> @@ -68,11 +60,7 @@ incompatible_features 0x0
>   Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>   wrote 512/512 bytes at offset 0
>   512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -./common.rc: Killed  ( if [ "${VALGRIND_QEMU}" == "y" ]; then
> -exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 
> "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -else
> -exec "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -fi )
> +./common.rc: Killed  ( _qemu_proc_wrapper 
> "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" )
>   incompatible_features 0x0
>   No errors were found on the image.
>   
> @@ -91,11 +79,7 @@ No errors were found on the image.
>   Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>   wrote 512/512 bytes at offset 0
>   512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -./common.rc: Killed  ( if [ "${VALGRIND_QEMU}" == "y" ]; then
> -exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 
> "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -else
> -exec "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -fi )
> +./common.rc: Killed  ( _qemu_proc_wrapper 
> "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" )
>   incompatible_features 0x1
>   ERROR cluster 5 refcount=0 reference=1
>   ERROR OFLAG_COPIED data cluster: l2_entry=8005 refcount=0
> @@ -105,11 +89,7 @@ Data may be corrupted, or further writes to the image may 
> corrupt it.
>   Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>   wrote 512/512 bytes at offset 0
>   512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -./common.rc: Killed  ( if [ "${VALGRIND_QEMU}" == "y" ]; then
> -exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 
> "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -else
> -exec "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -fi )
> +./common.rc: Killed  ( _qemu_proc_wrapper 
> "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" )
>   incompatible_features 0x0
>   No errors were found on the image.
>   *** done
> diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
> index 1aa7d37..8cb57eb 100644
> --- a/tests/qemu-iotests/061.out
> +++ b/tests/qemu-iotests/061.out
> @@ -118,11 +118,7 @@ No errors were found on the image.
>   Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>   

Re: [Qemu-block] [QEMU] [PATCH v2 0/8] Add Qemu to SeaBIOS LCHS interface

[Sam Eiderman]

typo:
ddb.geometry.biosCylinders = “83257” *

Sam

> On 13 Jun 2019, at 10:41, Sam Eiderman  wrote:
> 
> 
> 
>> On 12 Jun 2019, at 22:18, Gerd Hoffmann > > wrote:
>> 
>> On Wed, Jun 12, 2019 at 04:30:03PM +0300, Sam Eiderman wrote:
>>> 
>>> 
 On 12 Jun 2019, at 16:06, Gerd Hoffmann >>> > wrote:
 
 On Wed, Jun 12, 2019 at 02:59:31PM +0300, Sam Eiderman wrote:
> v1:
> 
> Non-standard logical geometries break under QEMU.
> 
> A virtual disk which contains an operating system which depends on
> logical geometries (consistent values being reported from BIOS INT13
> AH=08) will most likely break under QEMU/SeaBIOS if it has non-standard
> logical geometries - for example 56 SPT (sectors per track).
> No matter what QEMU will guess - SeaBIOS, for large enough disks - will
> use LBA translation, which will report 63 SPT instead.
 
 --verbose please.
 
 As far I know seabios switches to LBA mode when the disk is simply too
 big for LCHS addressing.  So I fail to see which problem is solved by
 this.  If your guest needs LCHS, why do you assign a disk which can't
 be fully accessed using LCHS addressing?
>>> 
>>> The scenario is as follows:
>>> 
>>> A user has a disk with 56 spts.
>>> This disk has been already created under a bios that reported 56 spts.
>>> When migrating this disk to QEMU/SeaBIOS, SeaBIOS will report 63 spts
>>> (under LBA translation) - this will break the boot for this guest.
>> 
>> You sayed so already.  I was looking for a real world example.  Guests
>> which can't deal with LBA should be pretty rare these days.  What kind
>> of guest?  What other bios?  Or is this a purely theoretical issue?
> 
> Yes they are pretty rare.
> Windows 2000 and Windows XP guests migrated from VMware to Qemu/KVM
> would not boot due to incorrect disk geometries (some had 32/56 spt instead of
> 56. Also number of heads was not entirely correct)
> 
>> 
> In addition we can not enforce SeaBIOS to rely on phyiscal geometries at
> all. A virtio-blk-pci virtual disk with 255 phyiscal heads can not
> report more than 16 physical heads when moved to an IDE controller, the
> ATA spec allows a maximum of 16 heads - this is an artifact of
> virtualization.
 
 Well, not really.  Moving disks from one controller to another when the
 OS depends on LHCS addressing never is a good idea.  That already caused
 problems in the 90-ies, when moving scsi disks from one scsi host
 adapter to another type, *way* before virtualization became a thing.
>>> 
>>> I agree, but this is easily solvable in virtualized environments where the
>>> hypervisor can guess the correct LCHS values by inspecting the MBR,
>> 
>> Yes.  This is exactly what the more clever scsi host adapter int13 rom
>> implementations ended up doing too.  Look at MBR to figure which LCHS
>> they should use.
>> 
>>> or letting the user set these values manually.
>> 
>> Why?  Asking the user to deal with the mess is pretty lame if there are
>> better options.  And IMO doing this fully automatic in seabios is
>> better.
> 
> I’m not against an automatic approach, however I do think that doing this
> in SeaBIOS might break compatibility for already existing guests that will
> suddenly see different LCHS values. (Explanation below)
> 
> Notice that already today it is possible to pass “cyls", “heads", “sectors” 
> and
> even “chs-trans” (IDE only) for devices in QEMU, but these are only the
> physical geometries of the disks which later on SeaBIOS might use to
> determine the logical geometries.
> "chs-trans" is an already existing PV interface between QEMU and
> SeaBIOS for that matter (although it only supports 4 IDE disks).
> 
> I believe that the steps to bring this issue to a more stable state are:
> Create a PV interface between QEMU and SeaBIOS to pass LCHS (Implemented here)
> Allow users to manually set values for LCHS values in QEMU (Implemented here)
> (Up until here, we do not break any existing functionality)
> Implement a better LCHS guessing algorithm in QEMU - the existing ones 
> contains some issues
> On new machine versions - pass guessed LCHS directly to SeaBIOS
> At the moment QEMU does not propagate its MBR guessed LCHS values, but only 
> uses them to set PCHS values for disks - so SeaBIOS has to guess again
> (Also here we will not break compatibility for older machine versions)
> 
> In addition, QEMU allows the use of VMDKs, some VMDK descriptors contain the 
> following values:
> ddb.geometry.biosHeads = “16”
> ddb.geometry.biosHeads = “83257”
> Which override the guessing algorithm in VMware and request the following 
> values to be set.
> 
> Providing such PV interface will allow to support these VMDKs too.
> 
>> 
 BTW:  One possible way to figure which LCHS layout a disk uses is to
 check the MBR partition table.  With that we (a) don't need a new
 interface 

Re: [Qemu-block] [QEMU] [PATCH v2 0/8] Add Qemu to SeaBIOS LCHS interface

[Sam Eiderman]

> On 12 Jun 2019, at 22:18, Gerd Hoffmann  wrote:
> 
> On Wed, Jun 12, 2019 at 04:30:03PM +0300, Sam Eiderman wrote:
>> 
>> 
>>> On 12 Jun 2019, at 16:06, Gerd Hoffmann  wrote:
>>> 
>>> On Wed, Jun 12, 2019 at 02:59:31PM +0300, Sam Eiderman wrote:
 v1:
 
 Non-standard logical geometries break under QEMU.
 
 A virtual disk which contains an operating system which depends on
 logical geometries (consistent values being reported from BIOS INT13
 AH=08) will most likely break under QEMU/SeaBIOS if it has non-standard
 logical geometries - for example 56 SPT (sectors per track).
 No matter what QEMU will guess - SeaBIOS, for large enough disks - will
 use LBA translation, which will report 63 SPT instead.
>>> 
>>> --verbose please.
>>> 
>>> As far I know seabios switches to LBA mode when the disk is simply too
>>> big for LCHS addressing.  So I fail to see which problem is solved by
>>> this.  If your guest needs LCHS, why do you assign a disk which can't
>>> be fully accessed using LCHS addressing?
>> 
>> The scenario is as follows:
>> 
>> A user has a disk with 56 spts.
>> This disk has been already created under a bios that reported 56 spts.
>> When migrating this disk to QEMU/SeaBIOS, SeaBIOS will report 63 spts
>> (under LBA translation) - this will break the boot for this guest.
> 
> You sayed so already.  I was looking for a real world example.  Guests
> which can't deal with LBA should be pretty rare these days.  What kind
> of guest?  What other bios?  Or is this a purely theoretical issue?

Yes they are pretty rare.
Windows 2000 and Windows XP guests migrated from VMware to Qemu/KVM
would not boot due to incorrect disk geometries (some had 32/56 spt instead of
56. Also number of heads was not entirely correct)

> 
 In addition we can not enforce SeaBIOS to rely on phyiscal geometries at
 all. A virtio-blk-pci virtual disk with 255 phyiscal heads can not
 report more than 16 physical heads when moved to an IDE controller, the
 ATA spec allows a maximum of 16 heads - this is an artifact of
 virtualization.
>>> 
>>> Well, not really.  Moving disks from one controller to another when the
>>> OS depends on LHCS addressing never is a good idea.  That already caused
>>> problems in the 90-ies, when moving scsi disks from one scsi host
>>> adapter to another type, *way* before virtualization became a thing.
>> 
>> I agree, but this is easily solvable in virtualized environments where the
>> hypervisor can guess the correct LCHS values by inspecting the MBR,
> 
> Yes.  This is exactly what the more clever scsi host adapter int13 rom
> implementations ended up doing too.  Look at MBR to figure which LCHS
> they should use.
> 
>> or letting the user set these values manually.
> 
> Why?  Asking the user to deal with the mess is pretty lame if there are
> better options.  And IMO doing this fully automatic in seabios is
> better.

I’m not against an automatic approach, however I do think that doing this
in SeaBIOS might break compatibility for already existing guests that will
suddenly see different LCHS values. (Explanation below)

Notice that already today it is possible to pass “cyls", “heads", “sectors” and
even “chs-trans” (IDE only) for devices in QEMU, but these are only the
physical geometries of the disks which later on SeaBIOS might use to
determine the logical geometries.
"chs-trans" is an already existing PV interface between QEMU and
SeaBIOS for that matter (although it only supports 4 IDE disks).

I believe that the steps to bring this issue to a more stable state are:
Create a PV interface between QEMU and SeaBIOS to pass LCHS (Implemented here)
Allow users to manually set values for LCHS values in QEMU (Implemented here)
(Up until here, we do not break any existing functionality)
Implement a better LCHS guessing algorithm in QEMU - the existing ones contains 
some issues
On new machine versions - pass guessed LCHS directly to SeaBIOS
At the moment QEMU does not propagate its MBR guessed LCHS values, but only 
uses them to set PCHS values for disks - so SeaBIOS has to guess again
(Also here we will not break compatibility for older machine versions)

In addition, QEMU allows the use of VMDKs, some VMDK descriptors contain the 
following values:
ddb.geometry.biosHeads = “16”
ddb.geometry.biosHeads = “83257”
Which override the guessing algorithm in VMware and request the following 
values to be set.

Providing such PV interface will allow to support these VMDKs too.

> 
>>> BTW:  One possible way to figure which LCHS layout a disk uses is to
>>> check the MBR partition table.  With that we (a) don't need a new
>>> interface between qemu and seabios and (b) it is not needed to manually
>>> specify the geometry.
>> 
>> In my opinion SeaBIOS is not the correct place for this change since
>> “enhancing” the detection of LCHS values in SeaBIOS may cause it to
>> suddenly report different values for already existing guests which rely on

[Qemu-block] [PATCH v8] ssh: switch from libssh2 to libssh

[Pino Toscano]

Rewrite the implementation of the ssh block driver to use libssh instead
of libssh2.  The libssh library has various advantages over libssh2:
- easier API for authentication (for example for using ssh-agent)
- easier API for known_hosts handling
- supports newer types of keys in known_hosts

Use APIs/features available in libssh 0.8 conditionally, to support
older versions (which are not recommended though).

Adjust the various Docker/Travis scripts to use libssh when available
instead of libssh2.

Signed-off-by: Pino Toscano 
---

Changes from v7:
- #if HAVE_LIBSSH_0_8 -> #ifdef HAVE_LIBSSH_0_8
- ptrdiff_t -> size_t

Changes from v6:
- fixed few checkpatch style issues
- detect libssh 0.8 via symbol detection
- adjust travis/docker test material
- remove dead "default" case in a switch
- use variables for storing MIN() results
- adapt a documentation bit

Changes from v5:
- adapt to newer tracing APIs
- disable ssh compression (mimic what libssh2 does by default)
- use build time checks for libssh 0.8, and use newer APIs directly

Changes from v4:
- fix wrong usages of error_setg/session_error_setg/sftp_error_setg
- fix few return code checks
- remove now-unused parameters in few internal functions
- allow authentication with "none" method
- switch to unsigned int for the port number
- enable TCP_NODELAY on the socket
- fix one reference error message in iotest 207

Changes from v3:
- fix socket cleanup in connect_to_ssh()
- add comments about the socket cleanup
- improve the error reporting (closer to what was with libssh2)
- improve EOF detection on sftp_read()

Changes from v2:
- used again an own fd
- fixed co_yield() implementation

Changes from v1:
- fixed jumbo packets writing
- fixed missing 'err' assignment
- fixed commit message

 .travis.yml   |   4 +-
 block/Makefile.objs   |   6 +-
 block/ssh.c   | 622 +-
 block/trace-events|  14 +-
 configure |  65 +-
 docs/qemu-block-drivers.texi  |   2 +-
 .../dockerfiles/debian-win32-cross.docker |   1 -
 .../dockerfiles/debian-win64-cross.docker |   1 -
 tests/docker/dockerfiles/fedora.docker|   4 +-
 tests/docker/dockerfiles/ubuntu.docker|   2 +-
 tests/docker/dockerfiles/ubuntu1804.docker|   2 +-
 tests/qemu-iotests/207.out|   2 +-
 12 files changed, 374 insertions(+), 351 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index a08a7b7278..c70dd055ed 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -31,7 +31,7 @@ addons:
   - libseccomp-dev
   - libspice-protocol-dev
   - libspice-server-dev
-  - libssh2-1-dev
+  - libssh-dev
   - liburcu-dev
   - libusb-1.0-0-dev
   - libvte-2.91-dev
@@ -261,7 +261,7 @@ matrix:
 - libseccomp-dev
 - libspice-protocol-dev
 - libspice-server-dev
-- libssh2-1-dev
+- libssh-dev
 - liburcu-dev
 - libusb-1.0-0-dev
 - libvte-2.91-dev
diff --git a/block/Makefile.objs b/block/Makefile.objs
index ae11605c9f..bf01429dd5 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -31,7 +31,7 @@ block-obj-$(CONFIG_CURL) += curl.o
 block-obj-$(CONFIG_RBD) += rbd.o
 block-obj-$(CONFIG_GLUSTERFS) += gluster.o
 block-obj-$(CONFIG_VXHS) += vxhs.o
-block-obj-$(CONFIG_LIBSSH2) += ssh.o
+block-obj-$(CONFIG_LIBSSH) += ssh.o
 block-obj-y += accounting.o dirty-bitmap.o
 block-obj-y += write-threshold.o
 block-obj-y += backup.o
@@ -52,8 +52,8 @@ rbd.o-libs := $(RBD_LIBS)
 gluster.o-cflags   := $(GLUSTERFS_CFLAGS)
 gluster.o-libs := $(GLUSTERFS_LIBS)
 vxhs.o-libs:= $(VXHS_LIBS)
-ssh.o-cflags   := $(LIBSSH2_CFLAGS)
-ssh.o-libs := $(LIBSSH2_LIBS)
+ssh.o-cflags   := $(LIBSSH_CFLAGS)
+ssh.o-libs := $(LIBSSH_LIBS)
 block-obj-dmg-bz2-$(CONFIG_BZIP2) += dmg-bz2.o
 block-obj-$(if $(CONFIG_DMG),m,n) += $(block-obj-dmg-bz2-y)
 dmg-bz2.o-libs := $(BZIP2_LIBS)
diff --git a/block/ssh.c b/block/ssh.c
index 6da7b9cbfe..fb458d4548 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -24,8 +24,8 @@
 
 #include "qemu/osdep.h"
 
-#include 
-#include 
+#include 
+#include 
 
 #include "block/block_int.h"
 #include "block/qdict.h"
@@ -46,13 +46,11 @@
 #include "trace.h"
 
 /*
- * TRACE_LIBSSH2= enables tracing in libssh2 itself.  Note
- * that this requires that libssh2 was specially compiled with the
- * `./configure --enable-debug' option, so most likely you will have
- * to compile it yourself.  The meaning of  is described
- * here: http://www.libssh2.org/libssh2_trace.html
+ * TRACE_LIBSSH= enables tracing in libssh itself.
+ * The meaning of  is described here:
+ * http://api.libssh.org/master/group__libssh__log.html
  */
-#define TRACE_LIBSSH2 0 /* or try: LIBSSH2_TRACE_SFTP */
+#define TRACE_LIBSSH  0 /* see: SSH_LOG_* */
 
 typedef struct BDRVSSHState {