[Qemu-block] [PULL 12/12] iotests: Drop use of bash keyword 'function'

[Eric Blake]

Bash allows functions to be declared with or without the leading
keyword 'function'; but including the keyword does not comply with
POSIX syntax, and is confusing to ksh users where the use of the
keyword changes the scoping rules for functions.  Stick to the
POSIX form through iotests.

Done mechanically with:
  sed -i 's/^function //' $(git ls-files tests/qemu-iotests)

Signed-off-by: Eric Blake 
Message-Id: <20181116215002.2124581-1-ebl...@redhat.com>
Reviewed-by: Daniel P. Berrangé 
Reviewed-by: Philippe Mathieu-Daudé 
---
 tests/qemu-iotests/common.nbd | 12 ++--
 tests/qemu-iotests/common.pattern | 16 
 tests/qemu-iotests/common.qemu|  8 
 tests/qemu-iotests/common.tls | 10 +-
 tests/qemu-iotests/035|  2 +-
 tests/qemu-iotests/037|  2 +-
 tests/qemu-iotests/038|  6 +++---
 tests/qemu-iotests/046|  6 +++---
 tests/qemu-iotests/047|  2 +-
 tests/qemu-iotests/049|  4 ++--
 tests/qemu-iotests/051|  4 ++--
 tests/qemu-iotests/067|  4 ++--
 tests/qemu-iotests/071|  4 ++--
 tests/qemu-iotests/077|  4 ++--
 tests/qemu-iotests/081|  4 ++--
 tests/qemu-iotests/082|  2 +-
 tests/qemu-iotests/085| 10 +-
 tests/qemu-iotests/086|  2 +-
 tests/qemu-iotests/087|  6 +++---
 tests/qemu-iotests/099|  6 +++---
 tests/qemu-iotests/109|  2 +-
 tests/qemu-iotests/112|  2 +-
 tests/qemu-iotests/142|  8 
 tests/qemu-iotests/153|  4 ++--
 tests/qemu-iotests/157|  4 ++--
 tests/qemu-iotests/172|  6 +++---
 tests/qemu-iotests/176|  2 +-
 tests/qemu-iotests/177|  2 +-
 tests/qemu-iotests/184|  4 ++--
 tests/qemu-iotests/186|  4 ++--
 tests/qemu-iotests/195|  4 ++--
 tests/qemu-iotests/204|  2 +-
 tests/qemu-iotests/223|  4 ++--
 tests/qemu-iotests/227|  4 ++--
 tests/qemu-iotests/232|  6 +++---
 35 files changed, 86 insertions(+), 86 deletions(-)

diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
index 0f4497a7ea5..233187a25cc 100644
--- a/tests/qemu-iotests/common.nbd
+++ b/tests/qemu-iotests/common.nbd
@@ -23,7 +23,7 @@ nbd_unix_socket="${TEST_DIR}/qemu-nbd.sock"
 nbd_tcp_addr="127.0.0.1"
 nbd_pid_file="${TEST_DIR}/qemu-nbd.pid"

-function nbd_server_stop()
+nbd_server_stop()
 {
 local NBD_PID
 if [ -f "$nbd_pid_file" ]; then
@@ -36,7 +36,7 @@ function nbd_server_stop()
 rm -f "$nbd_unix_socket"
 }

-function nbd_server_wait_for_unix_socket()
+nbd_server_wait_for_unix_socket()
 {
 pid=$1

@@ -57,14 +57,14 @@ function nbd_server_wait_for_unix_socket()
 exit 1
 }

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

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

@@ -80,7 +80,7 @@ function nbd_server_set_tcp_port()
 exit 1
 }

-function nbd_server_wait_for_tcp_socket()
+nbd_server_wait_for_tcp_socket()
 {
 pid=$1

@@ -101,7 +101,7 @@ function nbd_server_wait_for_tcp_socket()
 exit 1
 }

-function nbd_server_start_tcp_socket()
+nbd_server_start_tcp_socket()
 {
 nbd_server_stop
 $QEMU_NBD -v -t -b $nbd_tcp_addr -p $nbd_tcp_port "$@" &
diff --git a/tests/qemu-iotests/common.pattern 
b/tests/qemu-iotests/common.pattern
index 34f4a8dc9b4..b67bb341360 100644
--- a/tests/qemu-iotests/common.pattern
+++ b/tests/qemu-iotests/common.pattern
@@ -16,7 +16,7 @@
 # along with this program.  If not, see .
 #

-function do_is_allocated() {
+do_is_allocated() {
 local start=$1
 local size=$2
 local step=$3
@@ -27,11 +27,11 @@ function do_is_allocated() {
 done
 }

-function is_allocated() {
+is_allocated() {
 do_is_allocated "$@" | $QEMU_IO "$TEST_IMG" | _filter_qemu_io
 }

-function do_io() {
+do_io() {
 local op=$1
 local start=$2
 local size=$3
@@ -45,22 +45,22 @@ function do_io() {
 done
 }

-function io_pattern() {
+io_pattern() {
 do_io "$@" | $QEMU_IO "$TEST_IMG" | _filter_qemu_io
 }

-function io() {
+io() {
 local start=$2
 local pattern=$(( (start >> 9) % 256 ))

 do_io "$@" $pattern | $QEMU_IO "$TEST_IMG" | _filter_qemu_io
 }

-function io_zero() {
+io_zero() {
 do_io "$@" 0 | $QEMU_IO "$TEST_IMG" | _filter_qemu_io
 }

-function io_test() {
+io_test() {
 local op=$1
 local offset=$2
 local cluster_size=$3
@@ -100,7 +100,7 @@ function io_test() {
 offset=$((offset + num_large * ( l2_size + half_cluster )))
 }

-function io_test2() {
+io_test2() {
 local orig_offset=$1
 local cluster_size=$2
 

[Qemu-block] [PULL 11/12] iotests: Also test I/O over NBD TLS

[Eric Blake]

Enhance test 233 to also perform I/O beyond the initial handshake.

Signed-off-by: Eric Blake 
Message-Id: <20181118022403.2211483-1-ebl...@redhat.com>
Reviewed-by: Daniel P. Berrangé 
---
 tests/qemu-iotests/233 | 12 +++-
 tests/qemu-iotests/233.out | 10 ++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/233 b/tests/qemu-iotests/233
index 46013cefdcd..a4da60d0ade 100755
--- a/tests/qemu-iotests/233
+++ b/tests/qemu-iotests/233
@@ -62,7 +62,7 @@ tls_x509_create_client "ca2" "client2"
 echo
 echo "== preparing image =="
 _make_test_img 64M
-
+$QEMU_IO -c 'w -P 0x11 1m 1m' "$TEST_IMG" | _filter_qemu_io

 echo
 echo "== check TLS client to plain server fails =="
@@ -96,6 +96,16 @@ $QEMU_IMG info --image-opts \
 driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0 \
 2>&1 | sed "s/$nbd_tcp_port/PORT/g"

+echo
+echo "== perform I/O over TLS =="
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
+$QEMU_IO -c 'r -P 0x11 1m 1m' -c 'w -P 0x22 1m 1m' --image-opts \
+--object tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0 \
+driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0 \
+2>&1 | _filter_qemu_io
+
+$QEMU_IO -f qcow2 -r -U -c 'r -P 0x22 1m 1m' "$TEST_IMG" | _filter_qemu_io
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/233.out b/tests/qemu-iotests/233.out
index 616e9238c89..94acd9b9479 100644
--- a/tests/qemu-iotests/233.out
+++ b/tests/qemu-iotests/233.out
@@ -9,6 +9,8 @@ Generating a signed certificate...

 == preparing image ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

 == check TLS client to plain server fails ==
 qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': 
Denied by server for option 5 (starttls)
@@ -27,4 +29,12 @@ disk size: unavailable
 == check TLS with different CA fails ==
 option negotiation failed: Verify failed: No certificate was found.
 qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': 
The certificate hasn't got a known issuer
+
+== perform I/O over TLS ==
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done
-- 
2.17.2

[Qemu-block] [PULL 10/12] tests: exercise NBD server in TLS mode

[Eric Blake]

From: Daniel P. Berrangé 

Add tests that validate it is possible to connect to an NBD server
running TLS mode. Also test mis-matched TLS vs non-TLS connections
correctly fail.

Signed-off-by: Daniel P. Berrangé 
Message-Id: <20181116155325.22428-7-berra...@redhat.com>
Reviewed-by: Eric Blake 
Tested-by: Eric Blake 
[eblake: rebase to iotests shell cleanups, use ss instead of socat for
port probing, sanitize port number in expected output]
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/common.nbd |  45 +++
 tests/qemu-iotests/233| 102 ++
 tests/qemu-iotests/233.out|  30 ++
 tests/qemu-iotests/group  |   1 +
 4 files changed, 178 insertions(+)
 create mode 100755 tests/qemu-iotests/233
 create mode 100644 tests/qemu-iotests/233.out

diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
index 9f841ab4029..0f4497a7ea5 100644
--- a/tests/qemu-iotests/common.nbd
+++ b/tests/qemu-iotests/common.nbd
@@ -20,6 +20,7 @@
 #

 nbd_unix_socket="${TEST_DIR}/qemu-nbd.sock"
+nbd_tcp_addr="127.0.0.1"
 nbd_pid_file="${TEST_DIR}/qemu-nbd.pid"

 function nbd_server_stop()
@@ -62,3 +63,47 @@ function nbd_server_start_unix_socket()
 $QEMU_NBD -v -t -k "$nbd_unix_socket" "$@" &
 nbd_server_wait_for_unix_socket $!
 }
+
+function nbd_server_set_tcp_port()
+{
+(ss --help) >/dev/null 2>&1 || _notrun "ss utility not found, skipping 
test"
+
+for ((port = 10809; port <= 10909; port++))
+do
+if ! ss -tln | grep -sqE ":$port\b"; then
+nbd_tcp_port=$port
+return
+fi
+done
+
+echo "Cannot find free TCP port for nbd in range 10809-10909"
+exit 1
+}
+
+function 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 by qemu-nbd"
+exit 1
+}
+
+function nbd_server_start_tcp_socket()
+{
+nbd_server_stop
+$QEMU_NBD -v -t -b $nbd_tcp_addr -p $nbd_tcp_port "$@" &
+nbd_server_wait_for_tcp_socket $!
+}
diff --git a/tests/qemu-iotests/233 b/tests/qemu-iotests/233
new file mode 100755
index 000..46013cefdcd
--- /dev/null
+++ b/tests/qemu-iotests/233
@@ -0,0 +1,102 @@
+#!/bin/bash
+#
+# Test NBD TLS certificate / authorization integration
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=berra...@redhat.com
+
+seq=$(basename $0)
+echo "QA output created by $seq"
+
+status=1   # failure is the default!
+
+_cleanup()
+{
+nbd_server_stop
+_cleanup_test_img
+tls_x509_cleanup
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.pattern
+. ./common.tls
+. ./common.nbd
+
+_supported_fmt raw qcow2
+_supported_proto file
+# If porting to non-Linux, consider using socat instead of ss in common.nbd
+_supported_os Linux
+_require_command QEMU_NBD
+
+nbd_server_set_tcp_port
+tls_x509_init
+
+echo
+echo "== preparing TLS creds =="
+
+tls_x509_create_root_ca "ca1"
+tls_x509_create_root_ca "ca2"
+tls_x509_create_server "ca1" "server1"
+tls_x509_create_client "ca1" "client1"
+tls_x509_create_client "ca2" "client2"
+
+echo
+echo "== preparing image =="
+_make_test_img 64M
+
+
+echo
+echo "== check TLS client to plain server fails =="
+nbd_server_start_tcp_socket "$TEST_IMG"
+
+$QEMU_IMG info --image-opts \
+--object tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0 \
+driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0 \
+2>&1 | sed "s/$nbd_tcp_port/PORT/g"
+
+nbd_server_stop
+
+echo
+echo "== check plain client to TLS server fails =="
+
+nbd_server_start_tcp_socket --object 
tls-creds-x509,dir=${tls_dir}/server1,endpoint=server,id=tls0,verify-peer=yes 
--tls-creds tls0 "$TEST_IMG"
+
+$QEMU_IMG info nbd://localhost:$nbd_tcp_port 2>&1 | sed 
"s/$nbd_tcp_port/PORT/g"
+
+echo
+echo "== check TLS works =="
+$QEMU_IMG info --image-opts \
+--object tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0 \
+

[Qemu-block] [PULL 09/12] tests: add iotests helpers for dealing with TLS certificates

[Eric Blake]

From: Daniel P. Berrangé 

Add helpers to common.tls for creating TLS certificates for a CA,
server and client.

Signed-off-by: Daniel P. Berrangé 
Message-Id: <20181116155325.22428-6-berra...@redhat.com>
Reviewed-by: Eric Blake 
[eblake: spelling and quoting touchups]
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/common.tls | 137 ++
 1 file changed, 137 insertions(+)
 create mode 100644 tests/qemu-iotests/common.tls

diff --git a/tests/qemu-iotests/common.tls b/tests/qemu-iotests/common.tls
new file mode 100644
index 000..cecab269ec6
--- /dev/null
+++ b/tests/qemu-iotests/common.tls
@@ -0,0 +1,137 @@
+#!/bin/bash
+#
+# Helpers for TLS related config
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+tls_dir="${TEST_DIR}/tls"
+
+function tls_x509_cleanup()
+{
+rm -f "${tls_dir}"/*.pem
+rm -f "${tls_dir}"/*/*.pem
+rmdir "${tls_dir}"/*
+rmdir "${tls_dir}"
+}
+
+
+function tls_x509_init()
+{
+mkdir -p "${tls_dir}"
+
+# use a fixed key so we don't waste system entropy on
+# each test run
+cat > "${tls_dir}/key.pem" < "${tls_dir}/ca.info" <&1 | head -1
+
+rm -f "${tls_dir}/ca.info"
+}
+
+
+function tls_x509_create_server()
+{
+caname=$1
+name=$2
+
+mkdir -p "${tls_dir}/$name"
+cat > "${tls_dir}/cert.info" <&1 | head -1
+ln -s "${tls_dir}/$caname-cert.pem" "${tls_dir}/$name/ca-cert.pem"
+ln -s "${tls_dir}/key.pem" "${tls_dir}/$name/server-key.pem"
+
+rm -f "${tls_dir}/cert.info"
+}
+
+
+function tls_x509_create_client()
+{
+caname=$1
+name=$2
+
+mkdir -p "${tls_dir}/$name"
+cat > "${tls_dir}/cert.info" <&1 | head -1
+ln -s "${tls_dir}/$caname-cert.pem" "${tls_dir}/$name/ca-cert.pem"
+ln -s "${tls_dir}/key.pem" "${tls_dir}/$name/client-key.pem"
+
+rm -f "${tls_dir}/cert.info"
+}
-- 
2.17.2

[Qemu-block] [PULL 01/12] qemu-iotests: remove unused variable 'here'

[Eric Blake]

From: Mao Zhongyi 

Running
git grep '\$here' tests/qemu-iotests

has 0 hits, which means we are setting a variable that has
no use.  It appears that commit e8f8624d removed the last
use.  So execute the following cmd to remove all of
the 'here=...' lines as dead code.

sed -i '/^here=/d' $(git grep -l '^here=' tests/qemu-iotests)

Cc: kw...@redhat.com
Cc: mre...@redhat.com
Cc: ebl...@redhat.com
Suggested-by: Eric Blake 
Signed-off-by: Mao Zhongyi 
Message-Id: <20181024094051.4470-3-maozhon...@cmss.chinamobile.com>
Reviewed-by: Eric Blake 
[eblake: touch up commit message, reorder series, rebase to master]
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/001 | 1 -
 tests/qemu-iotests/002 | 1 -
 tests/qemu-iotests/003 | 1 -
 tests/qemu-iotests/004 | 1 -
 tests/qemu-iotests/005 | 1 -
 tests/qemu-iotests/007 | 1 -
 tests/qemu-iotests/008 | 1 -
 tests/qemu-iotests/009 | 1 -
 tests/qemu-iotests/010 | 1 -
 tests/qemu-iotests/011 | 1 -
 tests/qemu-iotests/012 | 1 -
 tests/qemu-iotests/013 | 1 -
 tests/qemu-iotests/014 | 1 -
 tests/qemu-iotests/015 | 1 -
 tests/qemu-iotests/017 | 1 -
 tests/qemu-iotests/018 | 1 -
 tests/qemu-iotests/019 | 1 -
 tests/qemu-iotests/020 | 1 -
 tests/qemu-iotests/021 | 1 -
 tests/qemu-iotests/022 | 1 -
 tests/qemu-iotests/023 | 1 -
 tests/qemu-iotests/024 | 1 -
 tests/qemu-iotests/025 | 1 -
 tests/qemu-iotests/026 | 1 -
 tests/qemu-iotests/027 | 1 -
 tests/qemu-iotests/028 | 1 -
 tests/qemu-iotests/029 | 1 -
 tests/qemu-iotests/031 | 1 -
 tests/qemu-iotests/032 | 1 -
 tests/qemu-iotests/033 | 1 -
 tests/qemu-iotests/034 | 1 -
 tests/qemu-iotests/035 | 1 -
 tests/qemu-iotests/036 | 1 -
 tests/qemu-iotests/037 | 1 -
 tests/qemu-iotests/038 | 1 -
 tests/qemu-iotests/039 | 1 -
 tests/qemu-iotests/042 | 1 -
 tests/qemu-iotests/043 | 1 -
 tests/qemu-iotests/046 | 1 -
 tests/qemu-iotests/047 | 1 -
 tests/qemu-iotests/049 | 1 -
 tests/qemu-iotests/050 | 1 -
 tests/qemu-iotests/051 | 1 -
 tests/qemu-iotests/052 | 1 -
 tests/qemu-iotests/053 | 1 -
 tests/qemu-iotests/054 | 1 -
 tests/qemu-iotests/058 | 1 -
 tests/qemu-iotests/059 | 1 -
 tests/qemu-iotests/060 | 1 -
 tests/qemu-iotests/061 | 1 -
 tests/qemu-iotests/062 | 1 -
 tests/qemu-iotests/063 | 1 -
 tests/qemu-iotests/064 | 1 -
 tests/qemu-iotests/066 | 1 -
 tests/qemu-iotests/067 | 1 -
 tests/qemu-iotests/068 | 1 -
 tests/qemu-iotests/069 | 1 -
 tests/qemu-iotests/070 | 1 -
 tests/qemu-iotests/071 | 1 -
 tests/qemu-iotests/072 | 1 -
 tests/qemu-iotests/073 | 1 -
 tests/qemu-iotests/075 | 1 -
 tests/qemu-iotests/076 | 1 -
 tests/qemu-iotests/077 | 1 -
 tests/qemu-iotests/078 | 1 -
 tests/qemu-iotests/079 | 1 -
 tests/qemu-iotests/080 | 1 -
 tests/qemu-iotests/081 | 1 -
 tests/qemu-iotests/082 | 1 -
 tests/qemu-iotests/083 | 1 -
 tests/qemu-iotests/084 | 1 -
 tests/qemu-iotests/085 | 1 -
 tests/qemu-iotests/086 | 1 -
 tests/qemu-iotests/087 | 1 -
 tests/qemu-iotests/088 | 1 -
 tests/qemu-iotests/089 | 1 -
 tests/qemu-iotests/090 | 1 -
 tests/qemu-iotests/091 | 1 -
 tests/qemu-iotests/092 | 1 -
 tests/qemu-iotests/094 | 1 -
 tests/qemu-iotests/095 | 1 -
 tests/qemu-iotests/097 | 1 -
 tests/qemu-iotests/098 | 1 -
 tests/qemu-iotests/099 | 1 -
 tests/qemu-iotests/101 | 1 -
 tests/qemu-iotests/102 | 1 -
 tests/qemu-iotests/103 | 1 -
 tests/qemu-iotests/104 | 1 -
 tests/qemu-iotests/105 | 1 -
 tests/qemu-iotests/106 | 1 -
 tests/qemu-iotests/107 | 1 -
 tests/qemu-iotests/108 | 1 -
 tests/qemu-iotests/109 | 1 -
 tests/qemu-iotests/110 | 1 -
 tests/qemu-iotests/111 | 1 -
 tests/qemu-iotests/112 | 1 -
 tests/qemu-iotests/113 | 1 -
 tests/qemu-iotests/114 | 1 -
 tests/qemu-iotests/115 | 1 -
 tests/qemu-iotests/116 | 1 -
 tests/qemu-iotests/117 | 1 -
 tests/qemu-iotests/119 | 1 -
 tests/qemu-iotests/120 | 1 -
 tests/qemu-iotests/121 | 1 -
 tests/qemu-iotests/122 | 1 -
 tests/qemu-iotests/123 | 1 -
 tests/qemu-iotests/125 | 1 -
 tests/qemu-iotests/126 | 1 -
 tests/qemu-iotests/127 | 1 -
 tests/qemu-iotests/128 | 1 -
 tests/qemu-iotests/130 | 1 -
 tests/qemu-iotests/131 | 1 -
 tests/qemu-iotests/133 | 1 -
 tests/qemu-iotests/134 | 1 -
 tests/qemu-iotests/135 | 1 -
 tests/qemu-iotests/137 | 1 -
 tests/qemu-iotests/138 | 1 -
 tests/qemu-iotests/140 | 1 -
 tests/qemu-iotests/141 | 1 -
 tests/qemu-iotests/142 | 1 -
 tests/qemu-iotests/143 | 1 -
 tests/qemu-iotests/144 | 1 -
 tests/qemu-iotests/145 | 1 -
 tests/qemu-iotests/146 | 1 -
 tests/qemu-iotests/150 | 1 -
 tests/qemu-iotests/153 | 1 -
 tests/qemu-iotests/154 | 1 -
 tests/qemu-iotests/156 | 1 -
 tests/qemu-iotests/157 | 1 -
 tests/qemu-iotests/158 | 1 -
 tests/qemu-iotests/159 | 1 -
 tests/qemu-iotests/160 | 1 -
 tests/qemu-iotests/162 | 1 -
 tests/qemu-iotests/170 | 1 -
 tests/qemu-iotests/171 | 1 -
 tests/qemu-iotests/172 | 1 -
 tests/qemu-iotests/173 | 1 -
 tests/qemu-iotests/174 | 1 -
 tests/qemu-iotests/175 | 1 -
 tests/qemu-iotests/176 | 1 -
 tests/qemu-iotests/177 | 1 -
 tests/qemu-iotests/178 | 1 -
 tests/qemu-iotests/179 | 1 -
 tests/qemu-iotests/181 | 1 -
 tests/qemu-iotests/182 | 1 -
 

[Qemu-block] [PULL 08/12] tests: check if qemu-nbd is still alive before waiting

[Eric Blake]

From: Daniel P. Berrangé 

If the qemu-nbd UNIX socket has not shown up, the tests will sleep a bit
and then check again repeatedly for up to 30 seconds. This is pointless
if the qemu-nbd process has quit due to an error, so check whether the
pid is still alive before waiting and retrying.

Signed-off-by: Daniel P. Berrangé 
Message-Id: <20181116155325.22428-5-berra...@redhat.com>
Reviewed-by: Eric Blake 
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/common.nbd | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
index 27357f3151d..9f841ab4029 100644
--- a/tests/qemu-iotests/common.nbd
+++ b/tests/qemu-iotests/common.nbd
@@ -37,11 +37,19 @@ function nbd_server_stop()

 function 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"
@@ -52,5 +60,5 @@ function nbd_server_start_unix_socket()
 {
 nbd_server_stop
 $QEMU_NBD -v -t -k "$nbd_unix_socket" "$@" &
-nbd_server_wait_for_unix_socket
+nbd_server_wait_for_unix_socket $!
 }
-- 
2.17.2

[Qemu-block] [PULL 07/12] tests: pull qemu-nbd iotest helpers into common.nbd file

[Eric Blake]

From: Daniel P. Berrangé 

The helpers for starting/stopping qemu-nbd in 058 will be useful in
other test cases, so move them into a common.nbd file.

Signed-off-by: Daniel P. Berrangé 
Message-Id: <20181116155325.22428-4-berra...@redhat.com>
Reviewed-by: Eric Blake 
[eblake: fix shell quoting]
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/common.nbd | 56 +++
 tests/qemu-iotests/058| 47 +
 2 files changed, 64 insertions(+), 39 deletions(-)
 create mode 100644 tests/qemu-iotests/common.nbd

diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
new file mode 100644
index 000..27357f3151d
--- /dev/null
+++ b/tests/qemu-iotests/common.nbd
@@ -0,0 +1,56 @@
+#!/bin/bash
+# -*- shell-script-mode -*-
+#
+# Helpers for NBD server related config
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+nbd_unix_socket="${TEST_DIR}/qemu-nbd.sock"
+nbd_pid_file="${TEST_DIR}/qemu-nbd.pid"
+
+function nbd_server_stop()
+{
+local NBD_PID
+if [ -f "$nbd_pid_file" ]; then
+read NBD_PID < "$nbd_pid_file"
+rm -f "$nbd_pid_file"
+if [ -n "$NBD_PID" ]; then
+kill "$NBD_PID"
+fi
+fi
+rm -f "$nbd_unix_socket"
+}
+
+function nbd_server_wait_for_unix_socket()
+{
+for ((i = 0; i < 300; i++))
+do
+if [ -r "$nbd_unix_socket" ]; then
+return
+fi
+sleep 0.1
+done
+echo "Failed in check of unix socket created by qemu-nbd"
+exit 1
+}
+
+function nbd_server_start_unix_socket()
+{
+nbd_server_stop
+$QEMU_NBD -v -t -k "$nbd_unix_socket" "$@" &
+nbd_server_wait_for_unix_socket
+}
diff --git a/tests/qemu-iotests/058 b/tests/qemu-iotests/058
index 0d741a7cac2..d6d4f94d5d2 100755
--- a/tests/qemu-iotests/058
+++ b/tests/qemu-iotests/058
@@ -28,55 +28,19 @@ echo "QA output created by $seq"

 status=1   # failure is the default!

-nbd_unix_socket=$TEST_DIR/test_qemu_nbd_socket
-nbd_snapshot_img="nbd:unix:$nbd_unix_socket"
-rm -f "${TEST_DIR}/qemu-nbd.pid"
-
-_cleanup_nbd()
-{
-local NBD_SNAPSHOT_PID
-if [ -f "${TEST_DIR}/qemu-nbd.pid" ]; then
-read NBD_SNAPSHOT_PID < "${TEST_DIR}/qemu-nbd.pid"
-rm -f "${TEST_DIR}/qemu-nbd.pid"
-if [ -n "$NBD_SNAPSHOT_PID" ]; then
-kill "$NBD_SNAPSHOT_PID"
-fi
-fi
-rm -f "$nbd_unix_socket"
-}
-
-_wait_for_nbd()
-{
-for ((i = 0; i < 300; i++))
-do
-if [ -r "$nbd_unix_socket" ]; then
-return
-fi
-sleep 0.1
-done
-echo "Failed in check of unix socket created by qemu-nbd"
-exit 1
-}
-
-converted_image=$TEST_IMG.converted
-
 _export_nbd_snapshot()
 {
-_cleanup_nbd
-$QEMU_NBD -v -t -k "$nbd_unix_socket" "$TEST_IMG" -l $1 &
-_wait_for_nbd
+nbd_server_start_unix_socket "$TEST_IMG" -l $1
 }

 _export_nbd_snapshot1()
 {
-_cleanup_nbd
-$QEMU_NBD -v -t -k "$nbd_unix_socket" "$TEST_IMG" -l snapshot.name=$1 &
-_wait_for_nbd
+nbd_server_start_unix_socket "$TEST_IMG" -l snapshot.name=$1
 }

 _cleanup()
 {
-_cleanup_nbd
+nbd_server_stop
 _cleanup_test_img
 rm -f "$converted_image"
 }
@@ -86,6 +50,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.rc
 . ./common.filter
 . ./common.pattern
+. ./common.nbd

 _supported_fmt qcow2
 _supported_proto file
@@ -94,6 +59,10 @@ _require_command QEMU_NBD
 # Internal snapshots are (currently) impossible with refcount_bits=1
 _unsupported_imgopts 'refcount_bits=1[^0-9]'

+nbd_snapshot_img="nbd:unix:$nbd_unix_socket"
+
+converted_image=$TEST_IMG.converted
+
 # Use -f raw instead of -f $IMGFMT for the NBD connection
 QEMU_IO_NBD="$QEMU_IO -f raw --cache=$CACHEMODE"

-- 
2.17.2

[Qemu-block] [PULL 05/12] nbd/server: Ignore write errors when replying to NBD_OPT_ABORT

[Eric Blake]

Commit 37ec36f6 intentionally ignores errors when trying to reply
to an NBD_OPT_ABORT request for plaintext clients, but did not make
the same change for a TLS server.  Since NBD_OPT_ABORT is
documented as being a potential for an EPIPE when the client hangs
up without waiting for our reply, we don't need to pollute the
server's output with that failure.

Signed-off-by: Eric Blake 
Message-Id: <20181117223221.2198751-1-ebl...@redhat.com>
Reviewed-by: Daniel P. Berrangé 
---
 nbd/server.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 056cfa5ad47..dc04513de70 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1134,12 +1134,16 @@ static int nbd_negotiate_options(NBDClient *client, 
uint16_t myflags,
 return -EINVAL;

 default:
-ret = nbd_opt_drop(client, NBD_REP_ERR_TLS_REQD, errp,
+/* Let the client keep trying, unless they asked to
+ * quit. Always try to give an error back to the
+ * client; but when replying to OPT_ABORT, be aware
+ * that the client may hang up before receiving the
+ * error, in which case we are fine ignoring the
+ * resulting EPIPE. */
+ret = nbd_opt_drop(client, NBD_REP_ERR_TLS_REQD,
+   option == NBD_OPT_ABORT ? NULL : errp,
"Option 0x%" PRIx32
" not permitted before TLS", option);
-/* Let the client keep trying, unless they asked to
- * quit. In this mode, we've already sent an error, so
- * we can't ack the abort.  */
 if (option == NBD_OPT_ABORT) {
 return 1;
 }
-- 
2.17.2

[Qemu-block] [PULL 03/12] qemu-iotests: Modern shell scripting (use $() instead of ``)

[Eric Blake]

From: Mao Zhongyi 

Various shell files contain a mix between obsolete ``
and modern $(); It would be nice to convert to using
$() everywhere.  For now, just do the qemu-iotests directory.

Cc: kw...@redhat.com
Cc: mre...@redhat.com
Cc: ebl...@redhat.com
Suggested-by: Eric Blake 
Signed-off-by: Mao Zhongyi 
Message-Id: <20181024094051.4470-4-maozhon...@cmss.chinamobile.com>
Reviewed-by: Eric Blake 
[eblake: tweak commit message]
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/common.config |  4 +--
 tests/qemu-iotests/check | 60 
 2 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
index 3cda0fe5696..9f460f203da 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -21,8 +21,8 @@ export LANG=C

 PATH=".:$PATH"

-HOSTOS=`uname -s`
-arch=`uname -m`
+HOSTOS=$(uname -s)
+arch=$(uname -m)
 [[ "$arch" =~ "ppc64" ]] && qemu_arch=ppc64 || qemu_arch="$arch"

 # make sure we have a standard umask
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index b37713277d1..89ed275988a 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -80,17 +80,17 @@ _full_imgfmt_details()

 _full_platform_details()
 {
-os=`uname -s`
-host=`hostname -s`
-kernel=`uname -r`
-platform=`uname -m`
+os=$(uname -s)
+host=$(hostname -s)
+kernel=$(uname -r)
+platform=$(uname -m)
 echo "$os/$platform $host $kernel"
 }

 # $1 = prog to look for
 set_prog_path()
 {
-p=`command -v $1 2> /dev/null`
+p=$(command -v $1 2> /dev/null)
 if [ -n "$p" -a -x "$p" ]; then
 type -p "$p"
 else
@@ -147,9 +147,9 @@ do
 if $group
 then
 # arg after -g
-group_list=`sed -n <"$source_iotests/group" -e 's/$/ /' -e 
"/^[0-9][0-9][0-9].* $r /"'{
+group_list=$(sed -n <"$source_iotests/group" -e 's/$/ /' -e 
"/^[0-9][0-9][0-9].* $r /"'{
 s/ .*//p
-}'`
+}')
 if [ -z "$group_list" ]
 then
 echo "Group \"$r\" is empty or not defined?"
@@ -173,9 +173,9 @@ s/ .*//p
 # arg after -x
 # Populate $tmp.list with all tests
 awk '/^[0-9]{3,}/ {print $1}' "${source_iotests}/group" > $tmp.list 
2>/dev/null
-group_list=`sed -n <"$source_iotests/group" -e 's/$/ /' -e 
"/^[0-9][0-9][0-9].* $r /"'{
+group_list=$(sed -n <"$source_iotests/group" -e 's/$/ /' -e 
"/^[0-9][0-9][0-9].* $r /"'{
 s/ .*//p
-}'`
+}')
 if [ -z "$group_list" ]
 then
 echo "Group \"$r\" is empty or not defined?"
@@ -193,7 +193,7 @@ s/ .*//p
 rm -f $tmp.sed
 fi
 echo "/^$t\$/d" >>$tmp.sed
-numsed=`expr $numsed + 1`
+numsed=$(expr $numsed + 1)
 done
 sed -f $tmp.sed <$tmp.list >$tmp.tmp
 mv $tmp.tmp $tmp.list
@@ -433,12 +433,12 @@ testlist options
 ;;

 [0-9]*-[0-9]*)
-eval `echo $r | sed -e 's/^/start=/' -e 's/-/ end=/'`
+eval $(echo $r | sed -e 's/^/start=/' -e 's/-/ end=/')
 ;;

 [0-9]*-)
-eval `echo $r | sed -e 's/^/start=/' -e 's/-//'`
-end=`echo [0-9][0-9][0-9] [0-9][0-9][0-9][0-9] | sed -e 
's/\[0-9]//g' -e 's/  *$//' -e 's/.* //'`
+eval $(echo $r | sed -e 's/^/start=/' -e 's/-//')
+end=$(echo [0-9][0-9][0-9] [0-9][0-9][0-9][0-9] | sed -e 
's/\[0-9]//g' -e 's/  *$//' -e 's/.* //')
 if [ -z "$end" ]
 then
 echo "No tests in range \"$r\"?"
@@ -455,8 +455,8 @@ testlist options
 esac

 # get rid of leading 0s as can be interpreted as octal
-start=`echo $start | sed 's/^0*//'`
-end=`echo $end | sed 's/^0*//'`
+start=$(echo $start | sed 's/^0*//')
+end=$(echo $end | sed 's/^0*//')

 if $xpand
 then
@@ -531,7 +531,7 @@ fi
 # should be sort -n, but this did not work for Linux when this
 # was ported from IRIX
 #
-list=`sort $tmp.list`
+list=$(sort $tmp.list)
 rm -f $tmp.list $tmp.tmp $tmp.sed

 if [ -z "$QEMU_PROG" ]
@@ -590,7 +590,7 @@ fi
 export QEMU_NBD_PROG="$(type -p "$QEMU_NBD_PROG")"

 if [ -z "$QEMU_VXHS_PROG" ]; then
-  export QEMU_VXHS_PROG="`set_prog_path qnio_server`"
+export QEMU_VXHS_PROG="$(set_prog_path qnio_server)"
 fi

 if [ -x "$build_iotests/socket_scm_helper" ]
@@ -616,7 +616,7 @@ _wallclock()

 _timestamp()
 {
-now=`date "+%T"`
+now=$(date "+%T")
 printf %s " [$now]"
 }

@@ -642,9 +642,9 @@ END{ if (NR > 0) {

 if [ -f $tmp.expunged ]
 then
-notrun=`wc -l <$tmp.expunged | sed -e 's/  *//g'`
-try=`expr $try - $notrun`
-list=`echo "$list" | sed -f $tmp.expunged`
+notrun=$(wc -l <$tmp.expunged | sed -e 's/  *//g')
+try=$(expr $try - $notrun)
+list=$(echo "$list" | sed -f $tmp.expunged)
 fi

 echo "" >>check.log
@@ -682,8 +682,8 @@ trap 

[Qemu-block] [PULL 04/12] nbd: fix whitespace in server error message

[Eric Blake]

From: Daniel P. Berrangé 

A space was missing after the option number was printed:

  Option 0x8not permitted before TLS

becomes

  Option 0x8 not permitted before TLS

This fixes

  commit 3668328303429f3bc93ab3365c66331600b06a2d
  Author: Eric Blake 
  Date:   Fri Oct 14 13:33:09 2016 -0500

nbd: Send message along with server NBD_REP_ERR errors

Signed-off-by: Daniel P. Berrangé 
Message-Id: <20181116155325.22428-2-berra...@redhat.com>
Reviewed-by: Eric Blake 
[eblake: move lone space to next line]
Signed-off-by: Eric Blake 
---
 nbd/server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nbd/server.c b/nbd/server.c
index 4e8f5ae51b0..056cfa5ad47 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1136,7 +1136,7 @@ static int nbd_negotiate_options(NBDClient *client, 
uint16_t myflags,
 default:
 ret = nbd_opt_drop(client, NBD_REP_ERR_TLS_REQD, errp,
"Option 0x%" PRIx32
-   "not permitted before TLS", option);
+   " not permitted before TLS", option);
 /* Let the client keep trying, unless they asked to
  * quit. In this mode, we've already sent an error, so
  * we can't ack the abort.  */
-- 
2.17.2

[Qemu-block] [PULL 02/12] qemu-iotests: convert `pwd` and $(pwd) to $PWD

[Eric Blake]

From: Mao Zhongyi 

POSIX requires $PWD to be reliable, and we expect all
shells used by qemu scripts to be relatively close to
POSIX.  Thus, it is smarter to avoid forking the pwd
executable for something that is already available in
the environment.

So replace it with the following:

sed -i 's/\(`pwd`\|\$(pwd)\)/$PWD/g' $(git grep -l pwd)

Then delete a pointless line assigning PWD to itself.

Cc: kw...@redhat.com
Cc: mre...@redhat.com
Cc: ebl...@redhat.com
Suggested-by: Eric Blake 
Signed-off-by: Mao Zhongyi 
Message-Id: <20181024094051.4470-2-maozhon...@cmss.chinamobile.com>
Reviewed-by: Eric Blake 
[eblake: touch up commit message, reorder series, tweak a couple more files]
Signed-off-by: Eric Blake 
---
 configure| 2 +-
 tests/qemu-iotests/common.config | 2 --
 tests/qemu-iotests/common.rc | 2 +-
 scripts/coccinelle/tcg_gen_extract.cocci | 2 +-
 tests/check-block.sh | 6 +++---
 tests/qemu-iotests/check | 2 +-
 6 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/configure b/configure
index 5b1d83ea262..0a3c6a72c3b 100755
--- a/configure
+++ b/configure
@@ -878,7 +878,7 @@ Linux)
   vhost_crypto="yes"
   vhost_scsi="yes"
   vhost_vsock="yes"
-  QEMU_INCLUDES="-I\$(SRC_PATH)/linux-headers -I$(pwd)/linux-headers 
$QEMU_INCLUDES"
+  QEMU_INCLUDES="-I\$(SRC_PATH)/linux-headers -I$PWD/linux-headers 
$QEMU_INCLUDES"
   supported_os="yes"
   libudev="yes"
 ;;
diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
index 102aa6878a9..3cda0fe5696 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -25,8 +25,6 @@ HOSTOS=`uname -s`
 arch=`uname -m`
 [[ "$arch" =~ "ppc64" ]] && qemu_arch=ppc64 || qemu_arch="$arch"

-export PWD=`pwd`
-
 # make sure we have a standard umask
 umask 022

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 70ca65b49b3..e15e7a7c8e6 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -160,7 +160,7 @@ fi
 ORIG_TEST_IMG="$TEST_IMG"

 if [ -z "$TEST_DIR" ]; then
-TEST_DIR=`pwd`/scratch
+TEST_DIR=$PWD/scratch
 fi

 QEMU_TEST_DIR="${TEST_DIR}"
diff --git a/scripts/coccinelle/tcg_gen_extract.cocci 
b/scripts/coccinelle/tcg_gen_extract.cocci
index 81e66a35ae1..c10c8634827 100644
--- a/scripts/coccinelle/tcg_gen_extract.cocci
+++ b/scripts/coccinelle/tcg_gen_extract.cocci
@@ -17,7 +17,7 @@
 // --keep-comments --in-place \
 // --use-gitgrep --dir target
 //
-// $ docker run --rm -v `pwd`:`pwd` -w `pwd` philmd/coccinelle \
+// $ docker run --rm -v $PWD:$PWD -w $PWD philmd/coccinelle \
 // --macro-file scripts/cocci-macro-file.h \
 // --sp-file scripts/coccinelle/tcg_gen_extract.cocci \
 // --keep-comments --in-place \
diff --git a/tests/check-block.sh b/tests/check-block.sh
index c3de3789c48..f3d12fd602d 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -5,9 +5,9 @@ if [ "$#" -ne 0 ]; then
 FORMAT_LIST="$@"
 fi

-export QEMU_PROG="$(pwd)/x86_64-softmmu/qemu-system-x86_64"
-export QEMU_IMG_PROG="$(pwd)/qemu-img"
-export QEMU_IO_PROG="$(pwd)/qemu-io"
+export QEMU_PROG="$PWD/x86_64-softmmu/qemu-system-x86_64"
+export QEMU_IMG_PROG="$PWD/qemu-img"
+export QEMU_IO_PROG="$PWD/qemu-io"

 if [ ! -x $QEMU_PROG ]; then
 echo "'make check-block' requires qemu-system-x86_64"
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index aa94c6c7ea9..b37713277d1 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -99,7 +99,7 @@ set_prog_path()
 }

 if [ -z "$TEST_DIR" ]; then
-TEST_DIR=`pwd`/scratch
+TEST_DIR=$PWD/scratch
 fi

 if [ ! -e "$TEST_DIR" ]; then
-- 
2.17.2

Re: [Qemu-block] [Qemu-devel] [PATCH for-3.1] nvme: fix out-of-bounds access to the CMB

[Kevin Wolf]

Am 19.11.2018 um 18:09 hat Paolo Bonzini geschrieben:
> On 19/11/18 16:23, Mark Kanda wrote:
> > For CVE-2018-16847, I just noticed Kevin pulled in Li's previous fix (as
> > opposed to this one). Was this done in error?
> 
> Probably.  Kevin, can you revert and apply this one instead?  I don't
> care if 3.1 or 3.2, but the previous fix is pointless complication.

I was waiting for you to address Li Qiang's review comments before I
apply it. I can revert the other one once this is ready.

> > On 11/16/2018 3:31 AM, Paolo Bonzini wrote:
> >> Because the CMB BAR has a min_access_size of 2, if you read the last
> >> byte it will try to memcpy *2* bytes from n->cmbuf, causing an off-by-one
> >> error.  This is CVE-2018-16847.
> >>
> >> Another way to fix this might be to register the CMB as a RAM memory
> >> region, which would also be more efficient.  However, that might be a
> >> change for big-endian machines; I didn't think this through and I don't
> >> know how real hardware works.  Add a basic testcase for the CMB in case
> >> somebody does this change later on.
> >>
> >> Cc: Keith Busch 
> >> Cc: qemu-block@nongnu.org
> >> Cc: Li Qiang 
> >> Signed-off-by: Paolo Bonzini 
> >> ---
> >>   hw/block/nvme.c    |  2 +-
> >>   tests/Makefile.include |  2 +-
> >>   tests/nvme-test.c  | 58 +++---
> >>   3 files changed, 57 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> >> index 09d7c90259..5d92794ef7 100644
> >> --- a/hw/block/nvme.c
> >> +++ b/hw/block/nvme.c
> >> @@ -1192,7 +1192,7 @@ static const MemoryRegionOps nvme_cmb_ops = {
> >>   .write = nvme_cmb_write,
> >>   .endianness = DEVICE_LITTLE_ENDIAN,
> >>   .impl = {
> >> -    .min_access_size = 2,
> >> +    .min_access_size = 1,
> >>   .max_access_size = 8,
> >>   },
> >>   };

Anyway, that .min_access_size influences the accessible range feels
weird to me. Is this really how it is meant to work? I expected this
only to influence the allowed granularity of accesses, and that the
maximum accessible offset of the memory region is size - access_size.

Does this mean that the size parameter of memory_region_init_io() really
means we allow access to offsets from 0 to size + impl.min_access_size - 1?
If so, this is very surprising and I wonder if this is really the only
device that gets it wrong.

For nvme it doesn't matter much because it can trivially support
single-byte accesses, so this change is correct and fixes the problem,
but isn't the real bug in access_with_adjusted_size(), which should
adjust the accessed range in a way that it doesn't exceed the size of
the memory region?

I'm not sure why impl.min_access_size was set to 2 in the first place,
but was valid.min_access_size meant maybe? Though if I read the spec
correctly, that one should be 4, not 2.

Hm... But memory_region_access_valid() doesn't even check min and max
access size if an accepts function pointer isn't given as well. Yet,
there are devices that set min/max, but not accepts. Am I missing where
this actually takes effect or are they buggy?

This stuff really confuses me.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH for-3.1] iotests: Enhance 223 to cover multiple bitmap granularities

[Eric Blake]

On 11/19/18 11:29 AM, Eric Blake wrote:

Testing granularity at the same size as the cluster isn't quite
as fun as what happens when it is larger or smaller.  This
enhancement also shows that qemu's nbd server can server the


s/server/serve/


same disk over multiple exports simultaneously.

Signed-off-by: Eric Blake 
---



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

[Qemu-block] [PATCH for-3.1] iotests: Enhance 223 to cover multiple bitmap granularities

[Eric Blake]

Testing granularity at the same size as the cluster isn't quite
as fun as what happens when it is larger or smaller.  This
enhancement also shows that qemu's nbd server can server the
same disk over multiple exports simultaneously.

Signed-off-by: Eric Blake 
---

Just a testsuite enhancement, so suitable for -rc2 if it gets a
quick review. Otherwise, I will probably include it as part of
a larger series I'm working on to make qemu-nbd handle misaligned
images more sanely (that's a pre-existing problem in the 3.0 release,
so it's not fixing a regression and may be invovled enough to slip
into 4.0, depending on what the rest of my series looks like).

 tests/qemu-iotests/223 | 43 +++---
 tests/qemu-iotests/223.out | 32 +---
 2 files changed, 60 insertions(+), 15 deletions(-)

diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
index 72419e03388..397b865d347 100755
--- a/tests/qemu-iotests/223
+++ b/tests/qemu-iotests/223
@@ -57,10 +57,11 @@ run_qemu()
 }

 echo
-echo "=== Create partially sparse image, then add dirty bitmap ==="
+echo "=== Create partially sparse image, then add dirty bitmaps ==="
 echo

-_make_test_img 4M
+# Two bitmaps, to contrast granularity issues
+_make_test_img -o cluster_size=4k 4M
 $QEMU_IO -c 'w -P 0x11 1M 2M' "$TEST_IMG" | _filter_qemu_io
 run_qemu <

Re: [Qemu-block] [PATCH 7/6] iotests: Also test I/O over NBD TLS

[Eric Blake]

On 11/19/18 4:40 AM, Daniel P. Berrangé wrote:

On Sat, Nov 17, 2018 at 08:24:03PM -0600, Eric Blake wrote:

Enhance test 233 to also perform I/O beyond the initial handshake.

Signed-off-by: Eric Blake 
---

Depends on my tweak to 2/6 to suppress an EIO error message
on a failed read after NBD_CMD_DISC.

  tests/qemu-iotests/233 | 12 +++-
  tests/qemu-iotests/233.out | 10 ++
  2 files changed, 21 insertions(+), 1 deletion(-)


Reviewed-by: Daniel P. Berrangé 


Yay - after dropping patch 2/6 (both your work, and the code I was 
questioning whether to squash in), and instead using your patch for 
making qio return 0 on connection abort after shutdown, this test still 
passes without any extra lines from the server.


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

Re: [Qemu-block] [Qemu-devel] [PATCH for-3.1] nvme: fix out-of-bounds access to the CMB

[Paolo Bonzini]

On 19/11/18 16:23, Mark Kanda wrote:
> For CVE-2018-16847, I just noticed Kevin pulled in Li's previous fix (as
> opposed to this one). Was this done in error?

Probably.  Kevin, can you revert and apply this one instead?  I don't
care if 3.1 or 3.2, but the previous fix is pointless complication.

Paolo

> 
> Thanks,
> 
> -Mark
> 
> On 11/16/2018 3:31 AM, Paolo Bonzini wrote:
>> Because the CMB BAR has a min_access_size of 2, if you read the last
>> byte it will try to memcpy *2* bytes from n->cmbuf, causing an off-by-one
>> error.  This is CVE-2018-16847.
>>
>> Another way to fix this might be to register the CMB as a RAM memory
>> region, which would also be more efficient.  However, that might be a
>> change for big-endian machines; I didn't think this through and I don't
>> know how real hardware works.  Add a basic testcase for the CMB in case
>> somebody does this change later on.
>>
>> Cc: Keith Busch 
>> Cc: qemu-block@nongnu.org
>> Cc: Li Qiang 
>> Signed-off-by: Paolo Bonzini 
>> ---
>>   hw/block/nvme.c    |  2 +-
>>   tests/Makefile.include |  2 +-
>>   tests/nvme-test.c  | 58 +++---
>>   3 files changed, 57 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>> index 09d7c90259..5d92794ef7 100644
>> --- a/hw/block/nvme.c
>> +++ b/hw/block/nvme.c
>> @@ -1192,7 +1192,7 @@ static const MemoryRegionOps nvme_cmb_ops = {
>>   .write = nvme_cmb_write,
>>   .endianness = DEVICE_LITTLE_ENDIAN,
>>   .impl = {
>> -    .min_access_size = 2,
>> +    .min_access_size = 1,
>>   .max_access_size = 8,
>>   },
>>   };
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index 613242bc6e..fb0b449c02 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -730,7 +730,7 @@ tests/test-hmp$(EXESUF): tests/test-hmp.o
>>   tests/machine-none-test$(EXESUF): tests/machine-none-test.o
>>   tests/drive_del-test$(EXESUF): tests/drive_del-test.o
>> $(libqos-virtio-obj-y)
>>   tests/qdev-monitor-test$(EXESUF): tests/qdev-monitor-test.o
>> $(libqos-pc-obj-y)
>> -tests/nvme-test$(EXESUF): tests/nvme-test.o
>> +tests/nvme-test$(EXESUF): tests/nvme-test.o $(libqos-pc-obj-y)
>>   tests/pvpanic-test$(EXESUF): tests/pvpanic-test.o
>>   tests/i82801b11-test$(EXESUF): tests/i82801b11-test.o
>>   tests/ac97-test$(EXESUF): tests/ac97-test.o
>> diff --git a/tests/nvme-test.c b/tests/nvme-test.c
>> index 7674a446e4..2abb3b6d19 100644
>> --- a/tests/nvme-test.c
>> +++ b/tests/nvme-test.c
>> @@ -8,11 +8,64 @@
>>    */
>>     #include "qemu/osdep.h"
>> +#include "qemu/units.h"
>>   #include "libqtest.h"
>> +#include "libqos/libqos-pc.h"
>> +
>> +static QOSState *qnvme_start(const char *extra_opts)
>> +{
>> +    QOSState *qs;
>> +    const char *arch = qtest_get_arch();
>> +    const char *cmd = "-drive
>> id=drv0,if=none,file=null-co://,format=raw "
>> +  "-device nvme,addr=0x4.0,serial=foo,drive=drv0
>> %s";
>> +
>> +    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>> +    qs = qtest_pc_boot(cmd, extra_opts ? : "");
>> +    global_qtest = qs->qts;
>> +    return qs;
>> +    }
>> +
>> +    g_printerr("nvme tests are only available on x86\n");
>> +    exit(EXIT_FAILURE);
>> +}
>> +
>> +static void qnvme_stop(QOSState *qs)
>> +{
>> +    qtest_shutdown(qs);
>> +}
>>   -/* Tests only initialization so far. TODO: Replace with functional
>> tests */
>>   static void nop(void)
>>   {
>> +    QOSState *qs;
>> +
>> +    qs = qnvme_start(NULL);
>> +    qnvme_stop(qs);
>> +}
>> +
>> +static void nvmetest_cmb_test(void)
>> +{
>> +    const int cmb_bar_size = 2 * MiB;
>> +    QOSState *qs;
>> +    QPCIDevice *pdev;
>> +    QPCIBar bar;
>> +
>> +    qs = qnvme_start("-global nvme.cmb_size_mb=2");
>> +    pdev = qpci_device_find(qs->pcibus, QPCI_DEVFN(4,0));
>> +    g_assert(pdev != NULL);
>> +
>> +    qpci_device_enable(pdev);
>> +    bar = qpci_iomap(pdev, 2, NULL);
>> +
>> +    qpci_io_writel(pdev, bar, 0, 0xccbbaa99);
>> +    g_assert_cmpint(qpci_io_readb(pdev, bar, 0), ==, 0x99);
>> +    g_assert_cmpint(qpci_io_readw(pdev, bar, 0), ==, 0xaa99);
>> +
>> +    /* Test partially out-of-bounds accesses.  */
>> +    qpci_io_writel(pdev, bar, cmb_bar_size - 1, 0x44332211);
>> +    g_assert_cmpint(qpci_io_readb(pdev, bar, cmb_bar_size - 1), ==,
>> 0x11);
>> +    g_assert_cmpint(qpci_io_readw(pdev, bar, cmb_bar_size - 1), !=,
>> 0x2211);
>> +    g_assert_cmpint(qpci_io_readl(pdev, bar, cmb_bar_size - 1), !=,
>> 0x44332211);
>> +    qnvme_stop(qs);
>>   }
>>     int main(int argc, char **argv)
>> @@ -21,9 +74,8 @@ int main(int argc, char **argv)
>>     g_test_init(, , NULL);
>>   qtest_add_func("/nvme/nop", nop);
>> +    qtest_add_func("/nvme/cmb_test", nvmetest_cmb_test);
>>   -    qtest_start("-drive id=drv0,if=none,file=null-co://,format=raw "
>> -    "-device nvme,drive=drv0,serial=foo");
>>   ret = g_test_run();
>>     qtest_end();
>>

Re: [Qemu-block] [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Eric Blake]

On 11/19/18 4:37 AM, Daniel P. Berrangé wrote:


Actually, I tracked this message down to using socat (which actually
connects and then abruptly exits) when probing whether the socket is up and
listening.  That is, the message is being produced as a side effect of
nbd_server_wait_for_tcp_socket rather than during the actual $QEMU_IMG
command we are interested in testing.



This is the first use of socat in iotests.  Might not be the most
portable, but I don't know if I have better ideas.
nbdkit.git/tests/test-ip.sh greps the output of 'ss -ltn' to locate free
ports, but I don't know if ss is any better than socat.


So, I'm planning to squash this in, to use ss instead of socat, as follows:


Personally I prefer socat since it is more portable, per my previous
message.


socat is indeed probably more portable, but since tests 233 uses 
'_supported_os Linux', ss availability isn't a problem until a future 
user ports this test to non-Linux.  I'd like to patch qemu-nbd to NOT 
warn about a user that connects but does not consume data (the socat 
case, as well as port probes), but as that patch does not exist yet and 
-rc2 is getting close, I'll go ahead and send the pull request with ss 
instead of socat.


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

Re: [Qemu-block] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Eric Blake]

On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:

Add tests that validate it is possible to connect to an NBD server
running TLS mode. Also test mis-matched TLS vs non-TLS connections
correctly fail.
---



+++ b/tests/qemu-iotests/common.nbd



+function nbd_server_wait_for_tcp_socket()
+{



+sleep 0.1
+done
+echo "Failed in check of unix socket created by qemu-nbd"


Too much copy-and-paste.  Touching up. :)

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

Re: [Qemu-block] KVM Forum block no[td]es

[Alberto Garcia]

On Fri 16 Nov 2018 04:18:34 PM CET, Kevin Wolf wrote:
> Am 16.11.2018 um 16:03 hat Alberto Garcia geschrieben:
>> > I don't think anything needs a way to generally block graph changes
>> > around some node.  We only need to prevent changes to very specific
>> > sets of edges.  This is something that the permission system just
>> > cannot do.
>> 
>> But what would you do then?
>
> I agree with you mostly in that I think that most problems that Max
> mentioned aren't readl. The only real problem I see with GRAPH_MOD as
> a permission on the node level is this overblocking - but that's bad
> enough that I feel using permissions to block changes to a whole node
> is not a good solution.

I understand that this restriction would be too coarse, but do you have
any case in mind where this would be a problem, or are you simply
worried because it's not an elegant solution?

(not that I'm resisting the proposal of using a counter, I'm just
curious)

> So what's the alternative? Max had a possible solution in the first
> email in this thread:
>
>> - A property of BdrvChild that can be set by a non-parent seems more
>>   feasible, e.g. a counter where changing the child is possible only
>>   if the counter is 0.  This also actually makes sense in what it
>>   means.
>
> The commit job would increment BdrvChild.block_change (or whatever we
> would call it) for all bs->backing edges in the subchain.

So instead of having a permission blocking changes in all of a node's
links, this would only affect the backing links.

It should work, I can give it a try.

Does it make sense to keep the GRAPH_MOD permission at all if we're not
really using it and we don't know what it is for? We can just drop it,
or are we breaking the API or the ABI?

Berto

Re: [Qemu-block] [Qemu-devel] [PATCH for-3.1] nvme: fix out-of-bounds access to the CMB

[Mark Kanda]

For CVE-2018-16847, I just noticed Kevin pulled in Li's previous fix (as 
opposed to this one). Was this done in error?


Thanks,

-Mark

On 11/16/2018 3:31 AM, Paolo Bonzini wrote:

Because the CMB BAR has a min_access_size of 2, if you read the last
byte it will try to memcpy *2* bytes from n->cmbuf, causing an off-by-one
error.  This is CVE-2018-16847.

Another way to fix this might be to register the CMB as a RAM memory
region, which would also be more efficient.  However, that might be a
change for big-endian machines; I didn't think this through and I don't
know how real hardware works.  Add a basic testcase for the CMB in case
somebody does this change later on.

Cc: Keith Busch 
Cc: qemu-block@nongnu.org
Cc: Li Qiang 
Signed-off-by: Paolo Bonzini 
---
  hw/block/nvme.c|  2 +-
  tests/Makefile.include |  2 +-
  tests/nvme-test.c  | 58 +++---
  3 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 09d7c90259..5d92794ef7 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1192,7 +1192,7 @@ static const MemoryRegionOps nvme_cmb_ops = {
  .write = nvme_cmb_write,
  .endianness = DEVICE_LITTLE_ENDIAN,
  .impl = {
-.min_access_size = 2,
+.min_access_size = 1,
  .max_access_size = 8,
  },
  };
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 613242bc6e..fb0b449c02 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -730,7 +730,7 @@ tests/test-hmp$(EXESUF): tests/test-hmp.o
  tests/machine-none-test$(EXESUF): tests/machine-none-test.o
  tests/drive_del-test$(EXESUF): tests/drive_del-test.o $(libqos-virtio-obj-y)
  tests/qdev-monitor-test$(EXESUF): tests/qdev-monitor-test.o $(libqos-pc-obj-y)
-tests/nvme-test$(EXESUF): tests/nvme-test.o
+tests/nvme-test$(EXESUF): tests/nvme-test.o $(libqos-pc-obj-y)
  tests/pvpanic-test$(EXESUF): tests/pvpanic-test.o
  tests/i82801b11-test$(EXESUF): tests/i82801b11-test.o
  tests/ac97-test$(EXESUF): tests/ac97-test.o
diff --git a/tests/nvme-test.c b/tests/nvme-test.c
index 7674a446e4..2abb3b6d19 100644
--- a/tests/nvme-test.c
+++ b/tests/nvme-test.c
@@ -8,11 +8,64 @@
   */
  
  #include "qemu/osdep.h"

+#include "qemu/units.h"
  #include "libqtest.h"
+#include "libqos/libqos-pc.h"
+
+static QOSState *qnvme_start(const char *extra_opts)
+{
+QOSState *qs;
+const char *arch = qtest_get_arch();
+const char *cmd = "-drive id=drv0,if=none,file=null-co://,format=raw "
+  "-device nvme,addr=0x4.0,serial=foo,drive=drv0 %s";
+
+if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
+qs = qtest_pc_boot(cmd, extra_opts ? : "");
+global_qtest = qs->qts;
+return qs;
+}
+
+g_printerr("nvme tests are only available on x86\n");
+exit(EXIT_FAILURE);
+}
+
+static void qnvme_stop(QOSState *qs)
+{
+qtest_shutdown(qs);
+}
  
-/* Tests only initialization so far. TODO: Replace with functional tests */

  static void nop(void)
  {
+QOSState *qs;
+
+qs = qnvme_start(NULL);
+qnvme_stop(qs);
+}
+
+static void nvmetest_cmb_test(void)
+{
+const int cmb_bar_size = 2 * MiB;
+QOSState *qs;
+QPCIDevice *pdev;
+QPCIBar bar;
+
+qs = qnvme_start("-global nvme.cmb_size_mb=2");
+pdev = qpci_device_find(qs->pcibus, QPCI_DEVFN(4,0));
+g_assert(pdev != NULL);
+
+qpci_device_enable(pdev);
+bar = qpci_iomap(pdev, 2, NULL);
+
+qpci_io_writel(pdev, bar, 0, 0xccbbaa99);
+g_assert_cmpint(qpci_io_readb(pdev, bar, 0), ==, 0x99);
+g_assert_cmpint(qpci_io_readw(pdev, bar, 0), ==, 0xaa99);
+
+/* Test partially out-of-bounds accesses.  */
+qpci_io_writel(pdev, bar, cmb_bar_size - 1, 0x44332211);
+g_assert_cmpint(qpci_io_readb(pdev, bar, cmb_bar_size - 1), ==, 0x11);
+g_assert_cmpint(qpci_io_readw(pdev, bar, cmb_bar_size - 1), !=, 0x2211);
+g_assert_cmpint(qpci_io_readl(pdev, bar, cmb_bar_size - 1), !=, 
0x44332211);
+qnvme_stop(qs);
  }
  
  int main(int argc, char **argv)

@@ -21,9 +74,8 @@ int main(int argc, char **argv)
  
  g_test_init(, , NULL);

  qtest_add_func("/nvme/nop", nop);
+qtest_add_func("/nvme/cmb_test", nvmetest_cmb_test);
  
-qtest_start("-drive id=drv0,if=none,file=null-co://,format=raw "

-"-device nvme,drive=drv0,serial=foo");
  ret = g_test_run();
  
  qtest_end();

Re: [Qemu-block] [PULL 0/9] Block layer patches

[Peter Maydell]

On 19 November 2018 at 14:29, Kevin Wolf  wrote:
> The following changes since commit 9436e082de18b2fb2ceed2e9d1beef641ae64f23:
>
>   MAINTAINERS: clarify some of the tags (2018-11-19 11:19:23 +)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 6d0a4a0fb5c8f10c8eb68b52cfda0082b00ae963:
>
>   iotests: Test file-posix locking and reopen (2018-11-19 14:32:04 +0100)
>
> 
> Block layer patches:
>
> - file-posix: Fix shared permission locks after reopen
> - block: Fix error path for failed .bdrv_reopen_prepare
> - qcow2: Catch invalid allocations when the image becomes too large
> - vvfat/fdc/nvme: Fix segfaults and leaks
>

Applied, thanks.

-- PMM

Re: [Qemu-block] [PATCH 5/6] tests: add iotests helpers for dealing with TLS certificates

[Daniel P . Berrangé]

On Mon, Nov 19, 2018 at 08:27:56AM -0600, Eric Blake wrote:
> On 11/19/18 5:04 AM, Max Reitz wrote:
> 
> > > > > +tls_dir="${TEST_DIR}/tls"
> > > > > +
> > > > > +function tls_x509_cleanup()
> > > > > +{
> > > > > +rm -f ${tls_dir}/*.pem
> > > > > +rm -f ${tls_dir}/*/*.pem
> > > > > +rmdir ${tls_dir}/*
> > > > > +rmdir ${tls_dir}
> > > > 
> > > > Why not just:
> > > > rm -rf $tls_dir
> > > 
> > > Yeah, I guess we could do that for simplicity
> > > 
> > > > Also, the quoting is a bit inconsistent. if ${TEST_DIR} can contain 
> > > > spaces,
> > > > then all uses of ${tls_dir} need to be in "".
> > > 
> > > Hmm, yes.
> > 
> > Which by the way is a very good reason *not* to blindly use "rm -r".
> 
> If we ever revive Jeff Cody's patches to let each test run with its own
> temporary directory, then we don't need this function at all.  With that in
> place, you either run the testsuite in debug mode (all temporary files
> preserved) or in normal mode (./check itself does rm -rf on the temporary
> directory).

That would be very nice - i'm often just commenting out the rm lines to
preserve temp files.

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

[Qemu-block] [PULL 9/9] iotests: Test file-posix locking and reopen

[Kevin Wolf]

From: Max Reitz 

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/182 | 71 ++
 tests/qemu-iotests/182.out |  9 +
 2 files changed, 80 insertions(+)

diff --git a/tests/qemu-iotests/182 b/tests/qemu-iotests/182
index 4b31592fb8..3b7689c1d5 100755
--- a/tests/qemu-iotests/182
+++ b/tests/qemu-iotests/182
@@ -31,6 +31,7 @@ status=1  # failure is the default!
 _cleanup()
 {
 _cleanup_test_img
+rm -f "$TEST_IMG.overlay"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
@@ -71,6 +72,76 @@ echo 'quit' | $QEMU -nographic -monitor stdio \
 
 _cleanup_qemu
 
+echo
+echo '=== Testing reopen ==='
+echo
+
+# This tests that reopening does not unshare any permissions it should
+# not unshare
+# (There was a bug where reopening shared exactly the opposite of the
+# permissions it was supposed to share)
+
+_launch_qemu
+
+_send_qemu_cmd $QEMU_HANDLE \
+"{'execute': 'qmp_capabilities'}" \
+'return'
+
+# Open the image without any format layer (we are not going to access
+# it, so that is fine)
+# This should keep all permissions shared.
+success_or_failure=y _send_qemu_cmd $QEMU_HANDLE \
+"{'execute': 'blockdev-add',
+  'arguments': {
+  'node-name': 'node0',
+  'driver': 'file',
+  'filename': '$TEST_IMG',
+  'locking': 'on'
+  } }" \
+'return' \
+'error'
+
+# This snapshot will perform a reopen to drop R/W to RO.
+# It should still keep all permissions shared.
+success_or_failure=y _send_qemu_cmd $QEMU_HANDLE \
+"{'execute': 'blockdev-snapshot-sync',
+  'arguments': {
+  'node-name': 'node0',
+  'snapshot-file': '$TEST_IMG.overlay',
+  'snapshot-node-name': 'node1'
+  } }" \
+'return' \
+'error'
+
+# Now open the same file again
+# This does not require any permissions (and does not unshare any), so
+# this will not conflict with node0.
+success_or_failure=y _send_qemu_cmd $QEMU_HANDLE \
+"{'execute': 'blockdev-add',
+  'arguments': {
+  'node-name': 'node1',
+  'driver': 'file',
+  'filename': '$TEST_IMG',
+  'locking': 'on'
+  } }" \
+'return' \
+'error'
+
+# Now we attach the image to a virtio-blk device.  This device does
+# require some permissions (at least WRITE and READ_CONSISTENT), so if
+# reopening node0 unshared any (which it should not have), this will
+# fail (but it should not).
+success_or_failure=y _send_qemu_cmd $QEMU_HANDLE \
+"{'execute': 'device_add',
+  'arguments': {
+  'driver': 'virtio-blk',
+  'drive': 'node1'
+  } }" \
+'return' \
+'error'
+
+_cleanup_qemu
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/182.out b/tests/qemu-iotests/182.out
index f1463c8862..af501ca3f3 100644
--- a/tests/qemu-iotests/182.out
+++ b/tests/qemu-iotests/182.out
@@ -5,4 +5,13 @@ Starting QEMU
 Starting a second QEMU using the same image should fail
 QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,id=drive0,file.locking=on: 
Failed to get "write" lock
 Is another process using the image [TEST_DIR/t.qcow2]?
+
+=== Testing reopen ===
+
+{"return": {}}
+{"return": {}}
+Formatting 'TEST_DIR/t.qcow2.overlay', fmt=qcow2 size=197120 
backing_file=TEST_DIR/t.qcow2 backing_fmt=file cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+{"return": {}}
+{"return": {}}
+{"return": {}}
 *** done
-- 
2.19.1

[Qemu-block] [PULL 6/9] iotests: Add new test 220 for max compressed cluster offset

[Kevin Wolf]

From: Eric Blake 

If you have a capable file system (tmpfs is good, ext4 not so much;
run ./check with TEST_DIR pointing to a good location so as not
to skip the test), it's actually possible to create a qcow2 file
that expands to a sparse 512T image with just over 38M of content.
The test is not the world's fastest (qemu crawling through 256M
bits of refcount table to find the next cluster to allocate takes
several seconds, as does qemu-img check reporting millions of
leaked clusters); but it DOES catch the problem that the previous
patch just fixed where writing a compressed cluster to a full
image ended up overwriting the wrong cluster.

Suggested-by: Max Reitz 
Signed-off-by: Eric Blake 
Reviewed-by: Alberto Garcia 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/220 | 96 ++
 tests/qemu-iotests/220.out | 54 +
 tests/qemu-iotests/group   |  1 +
 3 files changed, 151 insertions(+)
 create mode 100755 tests/qemu-iotests/220
 create mode 100644 tests/qemu-iotests/220.out

diff --git a/tests/qemu-iotests/220 b/tests/qemu-iotests/220
new file mode 100755
index 00..0c5682bda0
--- /dev/null
+++ b/tests/qemu-iotests/220
@@ -0,0 +1,96 @@
+#!/bin/bash
+#
+# max limits on compression in huge qcow2 files
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+seq=$(basename $0)
+echo "QA output created by $seq"
+
+status=1# failure is the default!
+
+_cleanup()
+{
+_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.pattern
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+echo "== Creating huge file =="
+
+# Sanity check: We require a file system that permits the creation
+# of a HUGE (but very sparse) file.  tmpfs works, ext4 does not.
+if ! truncate --size=513T "$TEST_IMG"; then
+_notrun "file system on $TEST_DIR does not support large enough files"
+fi
+rm "$TEST_IMG"
+IMGOPTS='cluster_size=2M,refcount_bits=1' _make_test_img 513T
+
+echo "== Populating refcounts =="
+# We want an image with 256M refcounts * 2M clusters = 512T referenced.
+# Each 2M cluster holds 16M refcounts; the refcount table initially uses
+# 1 refblock, so we need to add 15 more.  The refcount table lives at 2M,
+# first refblock at 4M, L2 at 6M, so our remaining additions start at 8M.
+# Then, for each refblock, mark it as fully populated.
+to_hex() {
+printf %016x\\n $1 | sed 's/\(..\)/\\x\1/g'
+}
+truncate --size=38m "$TEST_IMG"
+entry=$((0x20))
+$QEMU_IO_PROG -f raw -c "w -P 0xff 4m 2m" "$TEST_IMG" | _filter_qemu_io
+for i in {1..15}; do
+offs=$((0x60 + i*0x20))
+poke_file "$TEST_IMG" $((i*8 + entry)) $(to_hex $offs)
+$QEMU_IO_PROG -f raw -c "w -P 0xff $offs 2m" "$TEST_IMG" | _filter_qemu_io
+done
+
+echo "== Checking file before =="
+# FIXME: 'qemu-img check' doesn't diagnose refcounts beyond the end of
+# the file as leaked clusters
+_check_test_img 2>&1 | sed '/^Leaked cluster/d'
+stat -c 'image size %s' "$TEST_IMG"
+
+echo "== Trying to write compressed cluster =="
+# Given our file size, the next available cluster at 512T lies beyond the
+# maximum offset that a compressed 2M cluster can reside in
+$QEMU_IO_PROG -c 'w -c 0 2m' "$TEST_IMG" | _filter_qemu_io
+# The attempt failed, but ended up allocating a new refblock
+stat -c 'image size %s' "$TEST_IMG"
+
+echo "== Writing normal cluster =="
+# The failed write should not corrupt the image, so a normal write succeeds
+$QEMU_IO_PROG -c 'w 0 2m' "$TEST_IMG" | _filter_qemu_io
+
+echo "== Checking file after =="
+# qemu-img now sees the millions of leaked clusters, thanks to the allocations
+# at 512T.  Undo many of our faked references to speed up the check.
+$QEMU_IO_PROG -f raw -c "w -z 5m 1m" -c "w -z 8m 30m" "$TEST_IMG" |
+_filter_qemu_io
+_check_test_img 2>&1 | sed '/^Leaked cluster/d'
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/220.out b/tests/qemu-iotests/220.out
new file mode 100644
index 00..af3021fd88
--- /dev/null
+++ b/tests/qemu-iotests/220.out
@@ -0,0 +1,54 @@
+QA output created by 220
+== Creating huge file ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=564049465049088
+== Populating refcounts ==
+wrote 2097152/2097152 bytes at offset 

[Qemu-block] [PULL 8/9] file-posix: Fix shared locks on reopen commit

[Kevin Wolf]

From: Max Reitz 

s->locked_shared_perm is the set of bits locked in the file, which is
the inverse of the permissions actually shared.  So we need to pass them
as they are to raw_apply_lock_bytes() instead of inverting them again.

Reported-by: Alberto Garcia 
Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
Signed-off-by: Kevin Wolf 
---
 block/file-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 58c86a01ea..07bbdab953 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -959,7 +959,7 @@ static void raw_reopen_commit(BDRVReopenState *state)
 
 /* Copy locks to the new fd before closing the old one. */
 raw_apply_lock_bytes(NULL, rs->fd, s->locked_perm,
- ~s->locked_shared_perm, false, _err);
+ s->locked_shared_perm, false, _err);
 if (local_err) {
 /* shouldn't fail in a sane host, but report it just in case. */
 error_report_err(local_err);
-- 
2.19.1

[Qemu-block] [PULL 7/9] block: Always abort reopen after prepare succeeded

[Kevin Wolf]

From: Max Reitz 

bdrv_reopen_multiple() does not invoke bdrv_reopen_abort() for the
element of the reopen queue for which bdrv_reopen_prepare() failed,
because it assumes that the prepare function will have rolled back all
changes already.

However, bdrv_reopen_prepare() does not do this in every case: It may
notice an error after BlockDriver.bdrv_reopen_prepare() succeeded, and
it will not invoke BlockDriver.bdrv_reopen_abort() then; and neither
will bdrv_reopen_multiple(), as explained above.

This is wrong because we must always call .bdrv_reopen_commit() or
.bdrv_reopen_abort() after .bdrv_reopen_prepare() has succeeded.
Otherwise, the block driver has no chance to undo what it has done in
its implementation of .bdrv_reopen_prepare().

To fix this, bdrv_reopen_prepare() has to call .bdrv_reopen_abort() if
it wants to return an error after .bdrv_reopen_prepare() has succeeded.

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
Signed-off-by: Kevin Wolf 
---
 block.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/block.c b/block.c
index fd67e14dfa..3feac08535 100644
--- a/block.c
+++ b/block.c
@@ -3201,6 +3201,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
BlockReopenQueue *queue,
 QDict *orig_reopen_opts;
 char *discard = NULL;
 bool read_only;
+bool drv_prepared = false;
 
 assert(reopen_state != NULL);
 assert(reopen_state->bs->drv != NULL);
@@ -3285,6 +3286,8 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
BlockReopenQueue *queue,
 goto error;
 }
 
+drv_prepared = true;
+
 /* Options that are not handled are only okay if they are unchanged
  * compared to the old state. It is expected that some options are only
  * used for the initial open, but not reopen (e.g. filename) */
@@ -3350,6 +3353,15 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
BlockReopenQueue *queue,
 reopen_state->options = qobject_ref(orig_reopen_opts);
 
 error:
+if (ret < 0 && drv_prepared) {
+/* drv->bdrv_reopen_prepare() has succeeded, so we need to
+ * call drv->bdrv_reopen_abort() before signaling an error
+ * (bdrv_reopen_multiple() will not call bdrv_reopen_abort()
+ * when the respective bdrv_reopen_prepare() has failed) */
+if (drv->bdrv_reopen_abort) {
+drv->bdrv_reopen_abort(reopen_state);
+}
+}
 qemu_opts_del(opts);
 qobject_unref(orig_reopen_opts);
 g_free(discard);
-- 
2.19.1

[Qemu-block] [PULL 5/9] qcow2: Don't allow overflow during cluster allocation

[Kevin Wolf]

From: Eric Blake 

Our code was already checking that we did not attempt to
allocate more clusters than what would fit in an INT64 (the
physical maximimum if we can access a full off_t's worth of
data).  But this does not catch smaller limits enforced by
various spots in the qcow2 image description: L1 and normal
clusters of L2 are documented as having bits 63-56 reserved
for other purposes, capping our maximum offset at 64PB (bit
55 is the maximum bit set).  And for compressed images with
2M clusters, the cap drops the maximum offset to bit 48, or
a maximum offset of 512TB.  If we overflow that offset, we
would write compressed data into one place, but try to
decompress from another, which won't work.

It's actually possible to prove that overflow can cause image
corruption without this patch; I'll add the iotests separately
in the next commit.

Signed-off-by: Eric Blake 
Reviewed-by: Alberto Garcia 
Signed-off-by: Kevin Wolf 
---
 block/qcow2.h  |  6 ++
 block/qcow2-refcount.c | 20 +---
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 29c98d87a0..8662b68575 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -42,6 +42,12 @@
 #define QCOW_MAX_CRYPT_CLUSTERS 32
 #define QCOW_MAX_SNAPSHOTS 65536
 
+/* Field widths in qcow2 mean normal cluster offsets cannot reach
+ * 64PB; depending on cluster size, compressed clusters can have a
+ * smaller limit (64PB for up to 16k clusters, then ramps down to
+ * 512TB for 2M clusters).  */
+#define QCOW_MAX_CLUSTER_OFFSET ((1ULL << 56) - 1)
+
 /* 8 MB refcount table is enough for 2 PB images at 64k cluster size
  * (128 GB for 512 byte clusters, 2 EB for 2 MB clusters) */
 #define QCOW_MAX_REFTABLE_SIZE S_8MiB
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 46082aeac1..1c63ac244a 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -31,7 +31,8 @@
 #include "qemu/bswap.h"
 #include "qemu/cutils.h"
 
-static int64_t alloc_clusters_noref(BlockDriverState *bs, uint64_t size);
+static int64_t alloc_clusters_noref(BlockDriverState *bs, uint64_t size,
+uint64_t max);
 static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
 int64_t offset, int64_t length, uint64_t addend,
 bool decrease, enum qcow2_discard_type type);
@@ -362,7 +363,7 @@ static int alloc_refcount_block(BlockDriverState *bs,
 }
 
 /* Allocate the refcount block itself and mark it as used */
-int64_t new_block = alloc_clusters_noref(bs, s->cluster_size);
+int64_t new_block = alloc_clusters_noref(bs, s->cluster_size, INT64_MAX);
 if (new_block < 0) {
 return new_block;
 }
@@ -954,7 +955,8 @@ int qcow2_update_cluster_refcount(BlockDriverState *bs,
 
 
 /* return < 0 if error */
-static int64_t alloc_clusters_noref(BlockDriverState *bs, uint64_t size)
+static int64_t alloc_clusters_noref(BlockDriverState *bs, uint64_t size,
+uint64_t max)
 {
 BDRVQcow2State *s = bs->opaque;
 uint64_t i, nb_clusters, refcount;
@@ -979,9 +981,9 @@ retry:
 }
 
 /* Make sure that all offsets in the "allocated" range are representable
- * in an int64_t */
+ * in the requested max */
 if (s->free_cluster_index > 0 &&
-s->free_cluster_index - 1 > (INT64_MAX >> s->cluster_bits))
+s->free_cluster_index - 1 > (max >> s->cluster_bits))
 {
 return -EFBIG;
 }
@@ -1001,7 +1003,7 @@ int64_t qcow2_alloc_clusters(BlockDriverState *bs, 
uint64_t size)
 
 BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC);
 do {
-offset = alloc_clusters_noref(bs, size);
+offset = alloc_clusters_noref(bs, size, QCOW_MAX_CLUSTER_OFFSET);
 if (offset < 0) {
 return offset;
 }
@@ -1083,7 +1085,11 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
 free_in_cluster = s->cluster_size - offset_into_cluster(s, offset);
 do {
 if (!offset || free_in_cluster < size) {
-int64_t new_cluster = alloc_clusters_noref(bs, s->cluster_size);
+int64_t new_cluster;
+
+new_cluster = alloc_clusters_noref(bs, s->cluster_size,
+   MIN(s->cluster_offset_mask,
+   QCOW_MAX_CLUSTER_OFFSET));
 if (new_cluster < 0) {
 return new_cluster;
 }
-- 
2.19.1

[Qemu-block] [PULL 4/9] qcow2: Document some maximum size constraints

[Kevin Wolf]

From: Eric Blake 

Although off_t permits up to 63 bits (8EB) of file offsets, in
practice, we're going to hit other limits first.  Document some
of those limits in the qcow2 spec (some are inherent, others are
implementation choices of qemu), and how choice of cluster size
can influence some of the limits.

While we cannot map any uncompressed virtual cluster to any
address higher than 64 PB (56 bits) (due to the current L1/L2
field encoding stopping at bit 55), qemu's cap of 8M for the
refcount table can still access larger host addresses for some
combinations of large clusters and small refcount_order.  For
comparison, ext4 with 4k blocks caps files at 16PB.

Another interesting limit: for compressed clusters, the L2 layout
requires an ever-smaller maximum host offset as cluster size gets
larger, down to a 512 TB maximum with 2M clusters.  In particular,
note that with a cluster size of 8k or smaller, the L2 entry for
a compressed cluster could technically point beyond the 64PB mark,
but when you consider that with 8k clusters and refcount_order = 0,
you cannot access beyond 512T without exceeding qemu's limit of an
8M cap on the refcount table, it is unlikely that any image in the
wild has attempted to do so.  To be safe, let's document that bits
beyond 55 in a compressed cluster must be 0.

Signed-off-by: Eric Blake 
Signed-off-by: Kevin Wolf 
---
 docs/interop/qcow2.txt | 38 --
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 845d40a086..fb5cb47245 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -40,7 +40,18 @@ The first cluster of a qcow2 image contains the file header:
 with larger cluster sizes.
 
  24 - 31:   size
-Virtual disk size in bytes
+Virtual disk size in bytes.
+
+Note: qemu has an implementation limit of 32 MB as
+the maximum L1 table size.  With a 2 MB cluster
+size, it is unable to populate a virtual cluster
+beyond 2 EB (61 bits); with a 512 byte cluster
+size, it is unable to populate a virtual size
+larger than 128 GB (37 bits).  Meanwhile, L1/L2
+table layouts limit an image to no more than 64 PB
+(56 bits) of populated clusters, and an image may
+hit other limits first (such as a file system's
+maximum size).
 
  32 - 35:   crypt_method
 0 for no encryption
@@ -326,6 +337,17 @@ in the image file.
 It contains pointers to the second level structures which are called refcount
 blocks and are exactly one cluster in size.
 
+Although a large enough refcount table can reserve clusters past 64 PB
+(56 bits) (assuming the underlying protocol can even be sized that
+large), note that some qcow2 metadata such as L1/L2 tables must point
+to clusters prior to that point.
+
+Note: qemu has an implementation limit of 8 MB as the maximum refcount
+table size.  With a 2 MB cluster size and a default refcount_order of
+4, it is unable to reference host resources beyond 2 EB (61 bits); in
+the worst case, with a 512 cluster size and refcount_order of 6, it is
+unable to access beyond 32 GB (35 bits).
+
 Given an offset into the image file, the refcount of its cluster can be
 obtained as follows:
 
@@ -365,6 +387,16 @@ The L1 table has a variable size (stored in the header) 
and may use multiple
 clusters, however it must be contiguous in the image file. L2 tables are
 exactly one cluster in size.
 
+The L1 and L2 tables have implications on the maximum virtual file
+size; for a given L1 table size, a larger cluster size is required for
+the guest to have access to more space.  Furthermore, a virtual
+cluster must currently map to a host offset below 64 PB (56 bits)
+(although this limit could be relaxed by putting reserved bits into
+use).  Additionally, as cluster size increases, the maximum host
+offset for a compressed cluster is reduced (a 2M cluster size requires
+compressed clusters to reside below 512 TB (49 bits), and this limit
+cannot be relaxed without an incompatible layout change).
+
 Given an offset into the virtual disk, the offset into the image file can be
 obtained as follows:
 
@@ -427,7 +459,9 @@ Standard Cluster Descriptor:
 Compressed Clusters Descriptor (x = 62 - (cluster_bits - 8)):
 
 Bit  0 - x-1:   Host cluster offset. This is usually _not_ aligned to a
-cluster or sector boundary!
+cluster or sector boundary!  If cluster_bits is
+small enough that this field includes bits beyond
+55, those upper bits must be set to 0.
 
  x - 61:Number of additional 512-byte sectors used for the
 compressed data, beyond the sector containing the offset
-- 

[Qemu-block] [PULL 2/9] fdc: fix segfault in fdctrl_stop_transfer() when DMA is disabled

[Kevin Wolf]

From: Mark Cave-Ayland 

Commit c8a35f1cf0f "fdc: use IsaDma interface instead of global DMA_*
functions" accidentally introduced a segfault in fdctrl_stop_transfer() for
non-DMA transfers.

If fdctrl->dma_chann has not been configured then the fdctrl->dma interface
reference isn't initialised during isabus_fdc_realize(). Unfortunately
fdctrl_stop_transfer() unconditionally references the DMA interface when
finishing the transfer causing a NULL pointer dereference.

Fix the issue by adding a check in fdctrl_stop_transfer() so that the DMA
interface reference and release method is only invoked if fdctrl->dma_chann
has been set.

(This issue was discovered by Martin testing a recent change in the NetBSD
installer under qemu-system-sparc)

Cc: qemu-sta...@nongnu.org
Reported-by: Martin Husemann 
Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Hervé Poussineau 
Reviewed-by: John Snow 
Signed-off-by: Kevin Wolf 
---
 hw/block/fdc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 2e9c1e1e2f..6f19f127a5 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1617,7 +1617,7 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t 
status0,
 fdctrl->fifo[5] = cur_drv->sect;
 fdctrl->fifo[6] = FD_SECTOR_SC;
 fdctrl->data_dir = FD_DIR_READ;
-if (!(fdctrl->msr & FD_MSR_NONDMA)) {
+if (fdctrl->dma_chann != -1 && !(fdctrl->msr & FD_MSR_NONDMA)) {
 IsaDmaClass *k = ISADMA_GET_CLASS(fdctrl->dma);
 k->release_DREQ(fdctrl->dma, fdctrl->dma_chann);
 }
-- 
2.19.1

[Qemu-block] [PULL 0/9] Block layer patches

[Kevin Wolf]

The following changes since commit 9436e082de18b2fb2ceed2e9d1beef641ae64f23:

  MAINTAINERS: clarify some of the tags (2018-11-19 11:19:23 +)

are available in the Git repository at:

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

for you to fetch changes up to 6d0a4a0fb5c8f10c8eb68b52cfda0082b00ae963:

  iotests: Test file-posix locking and reopen (2018-11-19 14:32:04 +0100)


Block layer patches:

- file-posix: Fix shared permission locks after reopen
- block: Fix error path for failed .bdrv_reopen_prepare
- qcow2: Catch invalid allocations when the image becomes too large
- vvfat/fdc/nvme: Fix segfaults and leaks


Eric Blake (3):
  qcow2: Document some maximum size constraints
  qcow2: Don't allow overflow during cluster allocation
  iotests: Add new test 220 for max compressed cluster offset

Kevin Wolf (1):
  vvfat: Fix memory leak

Li Qiang (1):
  nvme: fix oob access issue(CVE-2018-16847)

Mark Cave-Ayland (1):
  fdc: fix segfault in fdctrl_stop_transfer() when DMA is disabled

Max Reitz (3):
  block: Always abort reopen after prepare succeeded
  file-posix: Fix shared locks on reopen commit
  iotests: Test file-posix locking and reopen

 docs/interop/qcow2.txt | 38 +-
 block/qcow2.h  |  6 +++
 block.c| 12 ++
 block/file-posix.c |  2 +-
 block/qcow2-refcount.c | 20 ++
 block/vvfat.c  |  6 +--
 hw/block/fdc.c |  2 +-
 hw/block/nvme.c|  7 
 tests/qemu-iotests/182 | 71 ++
 tests/qemu-iotests/182.out |  9 +
 tests/qemu-iotests/220 | 96 ++
 tests/qemu-iotests/220.out | 54 ++
 tests/qemu-iotests/group   |  1 +
 13 files changed, 310 insertions(+), 14 deletions(-)
 create mode 100755 tests/qemu-iotests/220
 create mode 100644 tests/qemu-iotests/220.out

[Qemu-block] [PULL 1/9] nvme: fix oob access issue(CVE-2018-16847)

[Kevin Wolf]

From: Li Qiang 

Currently, the nvme_cmb_ops mr doesn't check the addr and size.
This can lead an oob access issue. This is triggerable in the guest.
Add check to avoid this issue.

Fixes CVE-2018-16847.

Reported-by: Li Qiang 
Reviewed-by: Paolo Bonzini 
Signed-off-by: Li Qiang 
Signed-off-by: Kevin Wolf 
---
 hw/block/nvme.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 09d7c90259..d0226e7fdc 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1175,6 +1175,10 @@ static void nvme_cmb_write(void *opaque, hwaddr addr, 
uint64_t data,
 unsigned size)
 {
 NvmeCtrl *n = (NvmeCtrl *)opaque;
+
+if (addr + size > NVME_CMBSZ_GETSIZE(n->bar.cmbsz)) {
+return;
+}
 memcpy(>cmbuf[addr], , size);
 }
 
@@ -1183,6 +1187,9 @@ static uint64_t nvme_cmb_read(void *opaque, hwaddr addr, 
unsigned size)
 uint64_t val;
 NvmeCtrl *n = (NvmeCtrl *)opaque;
 
+if (addr + size > NVME_CMBSZ_GETSIZE(n->bar.cmbsz)) {
+return 0;
+}
 memcpy(, >cmbuf[addr], size);
 return val;
 }
-- 
2.19.1

[Qemu-block] [PULL 3/9] vvfat: Fix memory leak

[Kevin Wolf]

Don't leak 'cluster' in the mapping == NULL case. Found by Coverity
(CID 1055918).

Fixes: 8d9401c2791ee2d2805b741b1ee3006041edcd3e
Signed-off-by: Kevin Wolf 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Liam Merwick 
Tested-by: Philippe Mathieu-Daudé 
---
 block/vvfat.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 1de5de1db4..b7b61ea8b7 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2510,7 +2510,7 @@ static int commit_one_file(BDRVVVFATState* s,
 uint32_t first_cluster = c;
 mapping_t* mapping = find_mapping_for_cluster(s, c);
 uint32_t size = filesize_of_direntry(direntry);
-char* cluster = g_malloc(s->cluster_size);
+char *cluster;
 uint32_t i;
 int fd = 0;
 
@@ -2528,17 +2528,17 @@ static int commit_one_file(BDRVVVFATState* s,
 if (fd < 0) {
 fprintf(stderr, "Could not open %s... (%s, %d)\n", mapping->path,
 strerror(errno), errno);
-g_free(cluster);
 return fd;
 }
 if (offset > 0) {
 if (lseek(fd, offset, SEEK_SET) != offset) {
 qemu_close(fd);
-g_free(cluster);
 return -3;
 }
 }
 
+cluster = g_malloc(s->cluster_size);
+
 while (offset < size) {
 uint32_t c1;
 int rest_size = (size - offset > s->cluster_size ?
-- 
2.19.1

Re: [Qemu-block] [PATCH 5/6] tests: add iotests helpers for dealing with TLS certificates

[Eric Blake]

On 11/19/18 5:04 AM, Max Reitz wrote:


+tls_dir="${TEST_DIR}/tls"
+
+function tls_x509_cleanup()
+{
+rm -f ${tls_dir}/*.pem
+rm -f ${tls_dir}/*/*.pem
+rmdir ${tls_dir}/*
+rmdir ${tls_dir}


Why not just:
rm -rf $tls_dir


Yeah, I guess we could do that for simplicity


Also, the quoting is a bit inconsistent. if ${TEST_DIR} can contain spaces,
then all uses of ${tls_dir} need to be in "".


Hmm, yes.


Which by the way is a very good reason *not* to blindly use "rm -r".


If we ever revive Jeff Cody's patches to let each test run with its own 
temporary directory, then we don't need this function at all.  With that 
in place, you either run the testsuite in debug mode (all temporary 
files preserved) or in normal mode (./check itself does rm -rf on the 
temporary directory).




So far we only seem to have one instance of "rm -r" in the iotests (and
that is on three files, so I don't even know why that has -r), and I'm
glad about that.


But until we have the global implementation of per-test temporary 
directories with cleanup relegated to the testsuite driver, I'm fine 
following your preference of explicit deletion of specific files rather 
than recursive deletion of the tree, even if it is more lines of code.


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

Re: [Qemu-block] [PATCH 2/6 for-3.1] nbd: stop waiting for a NBD response with NBD_CMD_DISC

[Eric Blake]

On 11/19/18 4:23 AM, Daniel P. Berrangé wrote:


Simply setting the 'quit' flag after sending NBD_CMD_DISC is enough to
get the coroutine to stop waiting for a reply and thus supress the error
message.


Actually, it's not quite enough - once you actually start performing I/O,
enough coroutines are kicked off that the error still happens:


Hmm, does the client not send requests synchronously  ? I expected that
any other I/O operations would have already received their reply by the
time we sent the DISC command.



During handshake phase (NBD_OPT_* messages), client sends requests 
synchronously (at most one pending read, in response to the most recent 
write). During transmission phase (NBD_CMD_* messages), the client is 
allowed to send messages asynchronously.  However, the NBD protocol 
recommends (but not requires) that the client never send NBD_CMD_DISC to 
disconnect until AFTER the client has collected all responses to any 
earlier in-flight messages. And if I'm reading the block/nbd-client.c 
code correctly, in general we don't try to shutdown a client while there 
are any pending commands with unread replies, except when we are 
shutting down due to detecting a protocol error (at which point we are 
already out of sync with the server and waiting for replies is not going 
to be helpful).




But I want to do more testing to make sure I'm not missing out on reporting
an actual error if I add that.


Yes, I'd be a little concerned about missing reporting of an error from
a command other than NBD_CMD_DISC if we did this.


But it looks like your approach of making the qio code behave more 
nicely is a smarter cleanup than my idea, so I'm willing to ditch my 
code in favor of yours.  Testing your approach now :)


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

Re: [Qemu-block] [PATCH 2/6 for-3.1] nbd: stop waiting for a NBD response with NBD_CMD_DISC

[Daniel P . Berrangé]

On Sat, Nov 17, 2018 at 08:19:10PM -0600, Eric Blake wrote:
> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> > When sending a NBD_CMD_DISC message there is no reply expected,
> > however, the nbd_read_eof() coroutine is still waiting for a reply.
> > In a plain NBD connection this doesn't matter as it will just get an
> > EOF, however, on a TLS connection it will get an interrupted TLS data
> > packet. The nbd_read_eof() will then print an error message on the
> > console due to missing reply to NBD_CMD_DISC.
> > 
> > This can be seen with qemu-img
> > 
> >$ qemu-img info \
> >--object tls-creds-x509,dir=tlsdir,id=tls0,endpoint=client \
> >--image-opts driver=nbd,host=127.0.0.1,port=9000,tls-creds=tls0
> >qemu-img: Cannot read from TLS channel: Input/output error
> >image: nbd://127.0.0.1:9000
> >file format: nbd
> >virtual size: 10M (10485760 bytes)
> >disk size: unavailable
> > 
> > Simply setting the 'quit' flag after sending NBD_CMD_DISC is enough to
> > get the coroutine to stop waiting for a reply and thus supress the error
> > message.
> 
> Actually, it's not quite enough - once you actually start performing I/O,
> enough coroutines are kicked off that the error still happens:
> 
> $  qemu-io -c 'r 1m 1m' -c 'w -P 0x22 1m 1m' --image-opts \
> --object tls-creds-x509,dir=scratch/tls/client1,endpoint=client,id=tls0\
>  driver=nbd,host=localhost,port=10809,tls-creds=tls0
> read 1048576/1048576 bytes at offset 1048576
> 1 MiB, 1 ops; 0.0430 sec (23.204 MiB/sec and 23.2040 ops/sec)
> wrote 1048576/1048576 bytes at offset 1048576
> 1 MiB, 1 ops; 0.0152 sec (65.479 MiB/sec and 65.4793 ops/sec)
> Cannot read from TLS channel: Input/output error
> 
> Squashing this in on top of your patch helps, though:
> 
> diff --git i/block/nbd-client.c w/block/nbd-client.c
> index 5f63e4b8f15..e7916c78996 100644
> --- i/block/nbd-client.c
> +++ w/block/nbd-client.c
> @@ -79,7 +79,14 @@ static coroutine_fn void nbd_read_reply_entry(void
> *opaque)
>  assert(s->reply.handle == 0);
>  ret = nbd_receive_reply(s->ioc, >reply, _err);
>  if (local_err) {
> -error_report_err(local_err);
> +/* If we are already quitting, either another error has
> + * already been reported, or we requested NBD_CMD_DISC and
> + * don't need to report anything further.  */
> +if (!s->quit) {
> +error_report_err(local_err);
> +} else {
> +error_free(local_err);
> +}
>  }
>  if (ret <= 0) {
>  break;
> 
> But I want to do more testing to make sure I'm not missing out on reporting
> an actual error if I add that.

It occurred to me that it would be nice if the TLS channel behaved the same
a normal channel when seeing EOF on the socket. Unfortunately the reason why
GNUTLS returns an error in this case does in fact make sense, so it isn't
quite as simple as just ignoring the error. Fortunately the NBD client has
decided it wants to shutdown and crucially has called qio_channel_shutdown()
at this point. Thus we can safely ignore the gnutls error code when the
channel has been shutdown for read and just return 0 for EOF as with a plain
socket.

Thus I've sent a patch which solves the problem in that way:

  https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg03616.html

With this applied, both my original patch and your extra chunk here
should be redundant.

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

Re: [Qemu-block] [PATCH for-3.1? v2 0/3] block: Fix two minor reopen issues

[Kevin Wolf]

Am 16.11.2018 um 17:45 hat Max Reitz geschrieben:
> These are fixes for issues I found when looking after something Berto
> has reported.  The second patch fixes that issue Berto found, the first
> one is only kind of related.
> 
> For the first patch:  bdrv_reopen_abort() or bdrv_reopen_commit() are
> called only if the respective bdrv_reopen_prepare() has succeeded.
> However, bdrv_reopen_prepare() can fail even if the block driver’s
> .bdrv_reopen_prepare() succeeded.  In that case, the block driver is
> expecting a .bdrv_reopen_abort() or .bdrv_reopen_commit(), but will
> never get it.
> 
> This is fixed by making bdrv_reopen_prepare() call .bdrv_reopen_abort()
> if an error occurs after .bdrv_reopen_prepare() has already succeeded.
> 
> In practice this just prevents resource leaks, so nothing too dramatic
> (fine for qemu-next).  It also means I’ve been incapable of writing a
> test, sorry.
> 
> 
> The second issue is more severe: file-posix applies the inverse share
> locks when reopening.  Now the user can only directly do reopens from
> the monitor for now, so that wouldn’t be so bad.  But of course there
> are other places in qemu that implicitly reopen nodes, like dropping
> R/W to RO or gaining R/W from RO.  Most of these places are symmetrical
> at least (they’ll get R/W on an RO image for a short period of time and
> then drop back to RO), so you’ll see the wrong shared permission locks
> only for a short duration.  But at least blockdev-snapshot and
> blockdev-snapshot-sync are one way; they drop R/W to RO and stay there.
> So if you do a blockdev-snapshot*, the shared permission bits will be
> flipped.  This is therefore very much user-visible.
> 
> It’s not catastrophic, though, so I’m not sure whether we need it in
> 3.1.  It’s definitely a bugfix, and I think we have patches queued for
> the next RC already, so I think it makes sense to include at least
> patches 2 and 3 as well.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [PATCH 00/22] QOM'ify SysBusDeviceClass->init

[Cornelia Huck]

On Mon, 19 Nov 2018 20:07:58 +0800
Mao Zhongyi  wrote:

> The SysBusDeviceClass::init() interface is considered
> as a legacy interface and there are currently some
> efforts going on to get rid of it. Thus convert 
> SysBusDeviceClass::init to DeviceClass::realize.

In case my comment to the s390 change comes off as negative: I like
getting rid of the legacy interface :)

[Qemu-block] [PATCH 02/22] block/noenand: Convert sysbus init function to realize function

[Mao Zhongyi]

Use DeviceClass rather than SysBusDeviceClass in
onenand_class_init().

Cc: kw...@redhat.com
Cc: mre...@redhat.com
Cc: qemu-block@nongnu.org

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
---
 hw/block/onenand.c | 19 ---
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/hw/block/onenand.c b/hw/block/onenand.c
index 0cb8d7fa13..6bf89aac1d 100644
--- a/hw/block/onenand.c
+++ b/hw/block/onenand.c
@@ -768,9 +768,8 @@ static const MemoryRegionOps onenand_ops = {
 .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static int onenand_initfn(SysBusDevice *sbd)
+static void onenand_realize(DeviceState *dev, Error **errp)
 {
-DeviceState *dev = DEVICE(sbd);
 OneNANDState *s = ONE_NAND(dev);
 uint32_t size = 1 << (24 + ((s->id.dev >> 4) & 7));
 void *ram;
@@ -790,14 +789,14 @@ static int onenand_initfn(SysBusDevice *sbd)
   0xff, size + (size >> 5));
 } else {
 if (blk_is_read_only(s->blk)) {
-error_report("Can't use a read-only drive");
-return -1;
+error_setg(errp, "Can't use a read-only drive");
+return;
 }
 blk_set_perm(s->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
  BLK_PERM_ALL, _err);
 if (local_err) {
-error_report_err(local_err);
-return -1;
+error_propagate(errp, local_err);
+return;
 }
 s->blk_cur = s->blk;
 }
@@ -814,15 +813,14 @@ static int onenand_initfn(SysBusDevice *sbd)
 s->data[1][0] = ram + ((0x0200 + (1 << (PAGE_SHIFT - 1))) << s->shift);
 s->data[1][1] = ram + ((0x8010 + (1 << (PAGE_SHIFT - 6))) << s->shift);
 onenand_mem_setup(s);
-sysbus_init_irq(sbd, >intr);
-sysbus_init_mmio(sbd, >container);
+sysbus_init_irq(SYS_BUS_DEVICE(dev), >intr);
+sysbus_init_mmio(SYS_BUS_DEVICE(dev), >container);
 vmstate_register(dev,
  ((s->shift & 0x7f) << 24)
  | ((s->id.man & 0xff) << 16)
  | ((s->id.dev & 0xff) << 8)
  | (s->id.ver & 0xff),
  _onenand, s);
-return 0;
 }
 
 static Property onenand_properties[] = {
@@ -837,9 +835,8 @@ static Property onenand_properties[] = {
 static void onenand_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
-SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
 
-k->init = onenand_initfn;
+dc->realize = onenand_realize;
 dc->reset = onenand_system_reset;
 dc->props = onenand_properties;
 }
-- 
2.17.1

[Qemu-block] [PATCH 00/22] QOM'ify SysBusDeviceClass->init

[Mao Zhongyi]

The SysBusDeviceClass::init() interface is considered
as a legacy interface and there are currently some
efforts going on to get rid of it. Thus convert 
SysBusDeviceClass::init to DeviceClass::realize.

Cc: alistair.fran...@wdc.com
Cc: anthony.per...@citrix.com
Cc: arm...@redhat.com
Cc: borntrae...@de.ibm.com
Cc: chout...@adacore.com
Cc: coh...@redhat.com
Cc: da...@gibson.dropbear.id.au
Cc: da...@redhat.com
Cc: edgar.igles...@gmail.com
Cc: ehabk...@redhat.com
Cc: f4...@amsat.org
Cc: g...@mprc.pku.edu.cn
Cc: jan.kis...@web.de
Cc: kra...@redhat.com
Cc: kw...@redhat.com
Cc: marcandre.lur...@redhat.com
Cc: marcel.apfelb...@gmail.com
Cc: mich...@walle.cc
Cc: mre...@redhat.com
Cc: m...@redhat.com
Cc: pbonz...@redhat.com
Cc: peter.mayd...@linaro.org
Cc: peter.mayd...@linaro.org

  
Cc: qemu-...@nongnu.org
Cc: qemu-block@nongnu.org
Cc: qemu-...@nongnu.org
Cc: qemu-s3...@nongnu.org
Cc: richard.hender...@linaro.org
Cc: r...@twiddle.net
Cc: sstabell...@kernel.org
Cc: th...@redhat.com
Cc: xen-de...@lists.xenproject.org

Mao Zhongyi (22):
  musicpal: Convert sysbus init function to realize function
  block/noenand: Convert sysbus init function to realize function
  char/grlib_apbuart: Convert sysbus init function to realize function
  core/empty_slot: Convert sysbus init function to realize function
  display/g364fb: Convert sysbus init function to realize function
  dma/puv3_dma: Convert sysbus init function to realize function
  gpio/puv3_gpio: Convert sysbus init function to realize function
  milkymist-softusb: Convert sysbus init function to realize function
  input/pl050: Convert sysbus init function to realize function
  intc/puv3_intc: Convert sysbus init function to realize function
  milkymist-hpdmc: Convert sysbus init function to realize function
  milkymist-pfpu: Convert sysbus init function to realize function
  puv3_pm.c: Convert sysbus init function to realize function
  nvram/ds1225y: Convert sysbus init function to realize function
  pci-bridge/dec: Convert sysbus init function to realize function
  timer/etraxfs_timer: Convert sysbus init function to realize function
  timer/grlib_gptimer: Convert sysbus init function to realize function
  timer/puv3_ost: Convert sysbus init function to realize function
  usb/tusb6010: Convert sysbus init function to realize function
  xen_backend: Convert sysbus init function to realize function
  event-facility: Change SysBusDeviceClass *sbdc to SysBusDeviceClass
*sbc
  core/sysbus: remove the SysBusDeviceClass::init path

 hw/arm/musicpal.c|  9 -
 hw/block/onenand.c   | 19 ---
 hw/char/grlib_apbuart.c  | 11 ---
 hw/core/empty_slot.c |  9 -
 hw/core/sysbus.c | 15 ---
 hw/display/g364fb.c  | 14 +-
 hw/dma/puv3_dma.c| 10 --
 hw/gpio/puv3_gpio.c  | 28 +---
 hw/input/milkymist-softusb.c | 15 ++-
 hw/input/pl050.c | 10 --
 hw/intc/puv3_intc.c  | 13 +
 hw/misc/milkymist-hpdmc.c|  9 +++--
 hw/misc/milkymist-pfpu.c | 11 ---
 hw/misc/puv3_pm.c| 10 --
 hw/nvram/ds1225y.c   | 12 +---
 hw/pci-bridge/dec.c  | 11 +--
 hw/s390x/event-facility.c|  4 ++--
 hw/timer/etraxfs_timer.c | 13 ++---
 hw/timer/grlib_gptimer.c | 10 --
 hw/timer/puv3_ost.c  | 12 +---
 hw/usb/tusb6010.c| 13 +
 hw/xen/xen_backend.c |  6 ++
 include/hw/sysbus.h  |  3 ---
 23 files changed, 102 insertions(+), 165 deletions(-)

-- 
2.17.1

Re: [Qemu-block] Change in qemu 2.12 causes qemu-img convert to NBD to write more data

[Kevin Wolf]

Am 17.11.2018 um 21:59 hat Nir Soffer geschrieben:
> On Fri, Nov 16, 2018 at 5:26 PM Kevin Wolf  wrote:
> 
> > Am 15.11.2018 um 23:27 hat Nir Soffer geschrieben:
> > > On Sun, Nov 11, 2018 at 6:11 PM Nir Soffer  wrote:
> > >
> > > > On Wed, Nov 7, 2018 at 7:55 PM Nir Soffer  wrote:
> > > >
> > > >> On Wed, Nov 7, 2018 at 7:27 PM Kevin Wolf  wrote:
> > > >>
> > > >>> Am 07.11.2018 um 15:56 hat Nir Soffer geschrieben:
> > > >>> > Wed, Nov 7, 2018 at 4:36 PM Richard W.M. Jones 
> > > >>> wrote:
> > > >>> >
> > > >>> > > Another thing I tried was to change the NBD server (nbdkit) so
> > that
> > > >>> it
> > > >>> > > doesn't advertise zero support to the client:
> > > >>> > >
> > > >>> > >   $ nbdkit --filter=log --filter=nozero memory size=6G
> > > >>> logfile=/tmp/log \
> > > >>> > >   --run './qemu-img convert ./fedora-28.img -n $nbd'
> > > >>> > >   $ grep '\.\.\.$' /tmp/log | sed 's/.*\([A-Z][a-z]*\).*/\1/' |
> > uniq
> > > >>> -c
> > > >>> > >2154 Write
> > > >>> > >
> > > >>> > > Not surprisingly no zero commands are issued.  The size of the
> > write
> > > >>> > > commands is very uneven -- it appears to be send one command per
> > > >>> block
> > > >>> > > of zeroes or data.
> > > >>> > >
> > > >>> > > Nir: If we could get information from imageio about whether
> > zeroing
> > > >>> is
> > > >>> > > implemented efficiently or not by the backend, we could change
> > > >>> > > virt-v2v / nbdkit to advertise this back to qemu.
> > > >>> >
> > > >>> > There is no way to detect the capability, ioctl(BLKZEROOUT) always
> > > >>> > succeeds, falling back to manual zeroing in the kernel silently
> > > >>> >
> > > >>> > Even if we could, sending zero on the wire from qemu may be even
> > > >>> > slower, and it looks like qemu send even more requests in this case
> > > >>> > (2154 vs ~1300).
> > > >>> >
> > > >>> > Looks like this optimization in qemu side leads to worse
> > performance,
> > > >>> > so it should not be enabled by default.
> > > >>>
> > > >>> Well, that's overgeneralising your case a bit. If the backend does
> > > >>> support efficient zero writes (which file systems, the most common
> > case,
> > > >>> generally do), doing one big write_zeroes request at the start can
> > > >>> improve performance quite a bit.
> > > >>>
> > > >>> It seems the problem is that we can't really know whether the
> > operation
> > > >>> will be efficient because the backends generally don't tell us. Maybe
> > > >>> NBD could introduce a flag for this, but in the general case it
> > appears
> > > >>> to me that we'll have to have a command line option.
> > > >>>
> > > >>> However, I'm curious what your exact use case and the backend used
> > in it
> > > >>> is? Can something be improved there to actually get efficient zero
> > > >>> writes and get even better performance than by just disabling the big
> > > >>> zero write?
> > > >>
> > > >>
> > > >> The backend is some NetApp storage connected via FC. I don't have
> > > >> more info on this. We get zero rate of about 1G/s on this storage,
> > which
> > > >> is quite slow compared with other storage we tested.
> > > >>
> > > >> One option we check now is if this is the kernel silent fallback to
> > manual
> > > >> zeroing when the server advertise wrong value of write_same_max_bytes.
> > > >>
> > > >
> > > > We eliminated this using blkdiscard. This is what we get on with this
> > > > storage
> > > > zeroing 100G LV:
> > > >
> > > > for i in 1 2 4 8 16 32; do time blkdiscard -z -p ${i}m
> > > >
> > /dev/6e1d84f9-f939-46e9-b108-0427a08c280c/2d5c06ce-6536-4b3c-a7b6-13c6d8e55ade;
> > > > done
> > > >
> > > > real 4m50.851s
> > > > user 0m0.065s
> > > > sys 0m1.482s
> > > >
> > > > real 4m30.504s
> > > > user 0m0.047s
> > > > sys 0m0.870s
> > > >
> > > > real 4m19.443s
> > > > user 0m0.029s
> > > > sys 0m0.508s
> > > >
> > > > real 4m13.016s
> > > > user 0m0.020s
> > > > sys 0m0.284s
> > > >
> > > > real 2m45.888s
> > > > user 0m0.011s
> > > > sys 0m0.162s
> > > >
> > > > real 2m10.153s
> > > > user 0m0.003s
> > > > sys 0m0.100s
> > > >
> > > > We are investigating why we get low throughput on this server, and also
> > > > will check
> > > > several other servers.
> > > >
> > > > Having a command line option to control this behavior sounds good. I
> > don't
> > > >> have enough data to tell what should be the default, but I think the
> > safe
> > > >> way would be to keep old behavior.
> > > >>
> > > >
> > > > We file this bug:
> > > > https://bugzilla.redhat.com/1648622
> > > >
> > >
> > > More data from even slower storage - zeroing 10G lv on Kaminario K2
> > >
> > > # time blkdiscard -z -p 32m /dev/test_vg/test_lv2
> > >
> > > real50m12.425s
> > > user0m0.018s
> > > sys 2m6.785s
> > >
> > > Maybe something is wrong with this storage, since we see this:
> > >
> > > # grep -s "" /sys/block/dm-29/queue/* | grep write_same_max_bytes
> > > /sys/block/dm-29/queue/write_same_max_bytes:512
> > >
> > > Since BLKZEROOUT always fallback to manual slow zeroing 

Re: [Qemu-block] [Qemu-devel] [PATCH for-3.1] fdc: fix segfault in fdctrl_stop_transfer() when DMA is disabled

[Kevin Wolf]

Am 18.11.2018 um 13:32 hat Mark Cave-Ayland geschrieben:
> On 13/11/2018 20:29, John Snow wrote:
> 
> > On 11/13/18 8:16 AM, Kevin Wolf wrote:
> >> Am 12.11.2018 um 20:58 hat John Snow geschrieben:
> >>>
> >>>
> >>> On 11/11/18 4:40 AM, Mark Cave-Ayland wrote:
>  Commit c8a35f1cf0f "fdc: use IsaDma interface instead of global DMA_*
>  functions" accidentally introduced a segfault in fdctrl_stop_transfer() 
>  for
>  non-DMA transfers.
> 
>  If fdctrl->dma_chann has not been configured then the fdctrl->dma 
>  interface
>  reference isn't initialised during isabus_fdc_realize(). Unfortunately
>  fdctrl_stop_transfer() unconditionally references the DMA interface when
>  finishing the transfer causing a NULL pointer dereference.
> 
>  Fix the issue by adding a check in fdctrl_stop_transfer() so that the DMA
>  interface reference and release method is only invoked if 
>  fdctrl->dma_chann
>  has been set.
> 
>  (This issue was discovered by Martin testing a recent change in the 
>  NetBSD
>  installer under qemu-system-sparc)
> 
>  Reported-by: Martin Husemann 
>  Signed-off-by: Mark Cave-Ayland 
>  ---
>   hw/block/fdc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
>  diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>  index 2e9c1e1e2f..6f19f127a5 100644
>  --- a/hw/block/fdc.c
>  +++ b/hw/block/fdc.c
>  @@ -1617,7 +1617,7 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, 
>  uint8_t status0,
>   fdctrl->fifo[5] = cur_drv->sect;
>   fdctrl->fifo[6] = FD_SECTOR_SC;
>   fdctrl->data_dir = FD_DIR_READ;
>  -if (!(fdctrl->msr & FD_MSR_NONDMA)) {
>  +if (fdctrl->dma_chann != -1 && !(fdctrl->msr & FD_MSR_NONDMA)) {
>   IsaDmaClass *k = ISADMA_GET_CLASS(fdctrl->dma);
>   k->release_DREQ(fdctrl->dma, fdctrl->dma_chann);
>   }
> 
> >>>
> >>> Thanks.
> >>>
> >>> Reviewed-by: John Snow 
> >>>
> >>> ... Kevin, would you mind staging this one-off for the next RC?
> >>
> >> No problem, I'm applying this to my block branch. However, my pull
> >> request for -rc1 is already merged, so this will have to wait until next
> >> week and -rc2.
> >>
> >> Kevin
> >>
> > 
> > I think that should be fine. Thank you!
> 
> Thanks everyone! Kevin, any chance you could also add a CC: qemu-stable@ tag 
> when you
> apply this to your branch? This will help ensure that when the next NetBSD 
> release
> appears the fix should already be available for most people.

Ok, done. Also actually CCed qemu-stable on this mail.

Kevin

Re: [Qemu-block] [PATCH] migration/block-dirty-bitmap: fix Coverity CID1390625

[Vladimir Sementsov-Ogievskiy]

16.11.2018 17:29, Stefan Hajnoczi wrote:
> On Tue, Oct 16, 2018 at 04:20:18PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Theoretically possible that we finish the skipping loop with bs = NULL
>> and the following code will crash trying to dereference it. Fix that.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
>>   migration/block-dirty-bitmap.c | 4 
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
>> index 477826330c..6de808f95f 100644
>> --- a/migration/block-dirty-bitmap.c
>> +++ b/migration/block-dirty-bitmap.c
>> @@ -288,6 +288,10 @@ static int init_dirty_bitmap_migration(void)
>>   bs = backing_bs(bs);
>>   }
>>   
>> +if (!bs || bs->implicit) {
> 
> Why is it necessary to check bs->implicit?
> 


to be sure, that it is not implicit, as previous loop may finish with implicit 
= true and bs!= NULL, if drv == NULL. (hmm, may be we want to check drv too?)

same thing is in bdrv_query_info, without any checks, and in 
bdrv_block_device_info with assert(bs).


-- 
Best regards,
Vladimir

Re: [Qemu-block] [Qemu-devel] [PATCH] iotests: Drop use of bash keyword 'function'

[Philippe Mathieu-Daudé]

On 16/11/18 22:50, Eric Blake wrote:

Bash allows functions to be declared with or without the leading
keyword 'function'; but including the keyword does not comply with
POSIX syntax, and is confusing to ksh users where the use of the
keyword changes the scoping rules for functions.  Stick to the
POSIX form through iotests.


I remember trying non-bash shell last year and this was not working well.
If ksh is used we should add a build test.



Done mechanically with:
   sed -i 's/^function //' $(git ls-files tests/qemu-iotests)

Signed-off-by: Eric Blake 


Reviewed-by: Philippe Mathieu-Daudé 


---

Based-on: <20181116155325.22428-1-berra...@redhat.com>
[0/6 Misc fixes to NBD]

  tests/qemu-iotests/common.nbd | 12 ++--
  tests/qemu-iotests/common.pattern | 16 
  tests/qemu-iotests/common.qemu|  8 
  tests/qemu-iotests/common.tls | 10 +-
  tests/qemu-iotests/035|  2 +-
  tests/qemu-iotests/037|  2 +-
  tests/qemu-iotests/038|  6 +++---
  tests/qemu-iotests/046|  6 +++---
  tests/qemu-iotests/047|  2 +-
  tests/qemu-iotests/049|  4 ++--
  tests/qemu-iotests/051|  4 ++--
  tests/qemu-iotests/067|  4 ++--
  tests/qemu-iotests/071|  4 ++--
  tests/qemu-iotests/077|  4 ++--
  tests/qemu-iotests/081|  4 ++--
  tests/qemu-iotests/082|  2 +-
  tests/qemu-iotests/085| 10 +-
  tests/qemu-iotests/086|  2 +-
  tests/qemu-iotests/087|  6 +++---
  tests/qemu-iotests/099|  6 +++---
  tests/qemu-iotests/109|  2 +-
  tests/qemu-iotests/112|  2 +-
  tests/qemu-iotests/142|  8 
  tests/qemu-iotests/153|  4 ++--
  tests/qemu-iotests/157|  4 ++--
  tests/qemu-iotests/172|  6 +++---
  tests/qemu-iotests/176|  2 +-
  tests/qemu-iotests/177|  2 +-
  tests/qemu-iotests/184|  4 ++--
  tests/qemu-iotests/186|  4 ++--
  tests/qemu-iotests/195|  4 ++--
  tests/qemu-iotests/204|  2 +-
  tests/qemu-iotests/223|  4 ++--
  tests/qemu-iotests/227|  4 ++--
  tests/qemu-iotests/232|  6 +++---
  35 files changed, 86 insertions(+), 86 deletions(-)

diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
index 0483ea7c55a..afcdb7ae7a0 100644
--- a/tests/qemu-iotests/common.nbd
+++ b/tests/qemu-iotests/common.nbd
@@ -23,7 +23,7 @@ nbd_unix_socket="${TEST_DIR}/qemu-nbd.sock"
  nbd_tcp_addr="127.0.0.1"
  nbd_pid_file="${TEST_DIR}/qemu-nbd.pid"

-function nbd_server_stop()
+nbd_server_stop()
  {
  local NBD_PID
  if [ -f "$nbd_pid_file" ]; then
@@ -36,7 +36,7 @@ function nbd_server_stop()
  rm -f "$nbd_unix_socket"
  }

-function nbd_server_wait_for_unix_socket()
+nbd_server_wait_for_unix_socket()
  {
  pid=$1

@@ -57,14 +57,14 @@ function nbd_server_wait_for_unix_socket()
  exit 1
  }

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

-function nbd_server_set_tcp_port()
+nbd_server_set_tcp_port()
  {
  for port in `seq 10809 10909`
  do
@@ -80,7 +80,7 @@ function nbd_server_set_tcp_port()
  exit 1
  }

-function nbd_server_wait_for_tcp_socket()
+nbd_server_wait_for_tcp_socket()
  {
  pid=$1

@@ -103,7 +103,7 @@ function nbd_server_wait_for_tcp_socket()
  exit 1
  }

-function nbd_server_start_tcp_socket()
+nbd_server_start_tcp_socket()
  {
  nbd_server_stop
  $QEMU_NBD -v -t -b $nbd_tcp_addr -p $nbd_tcp_port $@ &
diff --git a/tests/qemu-iotests/common.pattern 
b/tests/qemu-iotests/common.pattern
index 34f4a8dc9b4..b67bb341360 100644
--- a/tests/qemu-iotests/common.pattern
+++ b/tests/qemu-iotests/common.pattern
@@ -16,7 +16,7 @@
  # along with this program.  If not, see .
  #

-function do_is_allocated() {
+do_is_allocated() {
  local start=$1
  local size=$2
  local step=$3
@@ -27,11 +27,11 @@ function do_is_allocated() {
  done
  }

-function is_allocated() {
+is_allocated() {
  do_is_allocated "$@" | $QEMU_IO "$TEST_IMG" | _filter_qemu_io
  }

-function do_io() {
+do_io() {
  local op=$1
  local start=$2
  local size=$3
@@ -45,22 +45,22 @@ function do_io() {
  done
  }

-function io_pattern() {
+io_pattern() {
  do_io "$@" | $QEMU_IO "$TEST_IMG" | _filter_qemu_io
  }

-function io() {
+io() {
  local start=$2
  local pattern=$(( (start >> 9) % 256 ))

  do_io "$@" $pattern | $QEMU_IO "$TEST_IMG" | _filter_qemu_io
  }

-function io_zero() {
+io_zero() {
  do_io "$@" 0 | $QEMU_IO "$TEST_IMG" | _filter_qemu_io
  }

-function io_test() {
+io_test() {
  local op=$1
  local offset=$2
  local cluster_size=$3
@@ 

Re: [Qemu-block] [PATCH 5/6] tests: add iotests helpers for dealing with TLS certificates

[Max Reitz]

On 19.11.18 11:27, Daniel P. Berrangé wrote:
> On Fri, Nov 16, 2018 at 10:39:03AM -0600, Eric Blake wrote:
>> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
>>> Add helpers to common.tls for creating TLS certificates for a CA,
>>> server and client.
>>
>> MUCH appreciated!  We NEED this coverage, easily automated.
>>
>>>
>>> Signed-off-by: Daniel P. Berrangé 
>>> ---
>>>   tests/qemu-iotests/common.tls | 139 ++
>>>   1 file changed, 139 insertions(+)
>>>   create mode 100644 tests/qemu-iotests/common.tls
>>>
>>> diff --git a/tests/qemu-iotests/common.tls b/tests/qemu-iotests/common.tls
>>> new file mode 100644
>>
>> I was a bit surprised that this wasn't 100755, but this matches the fact
>> that none of the other common.* are executable. And after thinking more, it
>> makes sense - they aren't standalone scripts, but designed to be sourced,
>> and 'source' doesn't care about execute bits.
>>
>>> +tls_dir="${TEST_DIR}/tls"
>>> +
>>> +function tls_x509_cleanup()
>>> +{
>>> +rm -f ${tls_dir}/*.pem
>>> +rm -f ${tls_dir}/*/*.pem
>>> +rmdir ${tls_dir}/*
>>> +rmdir ${tls_dir}
>>
>> Why not just:
>> rm -rf $tls_dir
> 
> Yeah, I guess we could do that for simplicity
> 
>> Also, the quoting is a bit inconsistent. if ${TEST_DIR} can contain spaces,
>> then all uses of ${tls_dir} need to be in "".
> 
> Hmm, yes.

Which by the way is a very good reason *not* to blindly use "rm -r".

So far we only seem to have one instance of "rm -r" in the iotests (and
that is on three files, so I don't even know why that has -r), and I'm
glad about that.

I prefer for scripts to only delete what they have created and not
blindly delete something.  Wildcards are already kind of pushing it.

Maybe the user has created the tls dir beforehand, then I'd prefer for
the iotests not to just delete it and everything in it.  But the worst
of course would be if we get escaping wrong somewhere (as demonstrated
here) and suddenly we delete a completely unrelated directory by
accident.  Anyone remember Steam's 'rm -rf "$STEAMROOT"/*'?

Everyone knows they have to be careful with deleting things, but most of
the time it is a bother if you're in an interactive shell and know your
directory structure and all the arguments you're passing perfectly well.
 But a script doesn't know either, and "it's a bother" is not really an
argument if you have to write the code just once.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH] iotests: Drop use of bash keyword 'function'

[Daniel P . Berrangé]

On Fri, Nov 16, 2018 at 03:50:02PM -0600, Eric Blake wrote:
> Bash allows functions to be declared with or without the leading
> keyword 'function'; but including the keyword does not comply with
> POSIX syntax, and is confusing to ksh users where the use of the
> keyword changes the scoping rules for functions.  Stick to the
> POSIX form through iotests.
> 
> Done mechanically with:
>   sed -i 's/^function //' $(git ls-files tests/qemu-iotests)
> 
> Signed-off-by: Eric Blake 
> ---

Reviewed-by: Daniel P. Berrangé 


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

Re: [Qemu-block] [PATCH 7/6] iotests: Also test I/O over NBD TLS

[Daniel P . Berrangé]

On Sat, Nov 17, 2018 at 08:24:03PM -0600, Eric Blake wrote:
> Enhance test 233 to also perform I/O beyond the initial handshake.
> 
> Signed-off-by: Eric Blake 
> ---
> 
> Depends on my tweak to 2/6 to suppress an EIO error message
> on a failed read after NBD_CMD_DISC.
> 
>  tests/qemu-iotests/233 | 12 +++-
>  tests/qemu-iotests/233.out | 10 ++
>  2 files changed, 21 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé 


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

Re: [Qemu-block] [PATCH 1.5/6] nbd/server: Ignore write errors when replying to NBD_OPT_ABORT

[Daniel P . Berrangé]

On Sat, Nov 17, 2018 at 04:32:21PM -0600, Eric Blake wrote:
> Commit 37ec36f6 intentionally ignores errors when trying to reply
> to an NBD_OPT_ABORT request for plaintext clients, but did not make
> the same change for a TLS server.  Since NBD_OPT_ABORT is
> documented as being a potential for an EPIPE when the client hangs
> up without waiting for our reply, we don't need to pollute the
> server's output with that failure.
> 
> Signed-off-by: Eric Blake 
> ---
>  nbd/server.c | 12 
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 056cfa5ad47..dc04513de70 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1134,12 +1134,16 @@ static int nbd_negotiate_options(NBDClient *client, 
> uint16_t myflags,
>  return -EINVAL;
> 
>  default:
> -ret = nbd_opt_drop(client, NBD_REP_ERR_TLS_REQD, errp,
> +/* Let the client keep trying, unless they asked to
> + * quit. Always try to give an error back to the
> + * client; but when replying to OPT_ABORT, be aware
> + * that the client may hang up before receiving the
> + * error, in which case we are fine ignoring the
> + * resulting EPIPE. */
> +ret = nbd_opt_drop(client, NBD_REP_ERR_TLS_REQD,
> +   option == NBD_OPT_ABORT ? NULL : errp,
> "Option 0x%" PRIx32
> " not permitted before TLS", option);
> -/* Let the client keep trying, unless they asked to
> - * quit. In this mode, we've already sent an error, so
> - * we can't ack the abort.  */
>  if (option == NBD_OPT_ABORT) {
>  return 1;
>  }

Reviewed-by: Daniel P. Berrangé 


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

Re: [Qemu-block] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Daniel P . Berrangé]

On Sat, Nov 17, 2018 at 02:49:22PM -0600, Eric Blake wrote:
> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> > Add tests that validate it is possible to connect to an NBD server
> > running TLS mode. Also test mis-matched TLS vs non-TLS connections
> > correctly fail.
> > ---
> 
> Missing your Signed-off-by. Can you please supply that, so I can include
> this in my pull request?

Signed-off-by: Daniel P. Berrangé 


> 
> Also, I'm getting failures when trying to test it:
> 
> @@ -17,9 +17,9 @@
> 
>  == check plain client to TLS server fails ==
>  option negotiation failed: read failed: Unexpected end-of-file before all
> bytes were read
> +write failed (error message): Unable to write to socket: Broken pipe
>  qemu-img: Could not open 'nbd://localhost:10809': TLS negotiation required
> before option 8 (structured reply)
>  server reported: Option 0x8 not permitted before TLS
> -write failed (error message): Unable to write to socket: Broken pipe
> 
> 
> which looks like an output race. :(

Guess that shows the message is in fact coming from the server rather
than the client then.

We could just silence the qemu-nbd process to null, since we're primarily
interested in what the client displays in this test

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

Re: [Qemu-block] [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Daniel P . Berrangé]

On Sat, Nov 17, 2018 at 03:31:34PM -0600, Eric Blake wrote:
> On 11/16/18 11:20 AM, Eric Blake wrote:
> > On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> > > Add tests that validate it is possible to connect to an NBD server
> > > running TLS mode. Also test mis-matched TLS vs non-TLS connections
> > > correctly fail.
> > > ---
> 
> > > +== check TLS client to plain server fails ==
> > > +option negotiation failed: read failed: Unexpected end-of-file
> > > before all bytes were read
> > 
> > Annoying message; I wonder if we can clean that up. But not this patch's
> > problem.
> > 
> 
> Actually, I tracked this message down to using socat (which actually
> connects and then abruptly exits) when probing whether the socket is up and
> listening.  That is, the message is being produced as a side effect of
> nbd_server_wait_for_tcp_socket rather than during the actual $QEMU_IMG
> command we are interested in testing.
> 
> 
> > >   nbd_pid_file="${TEST_DIR}/qemu-nbd.pid"
> > >   function nbd_server_stop()
> > > @@ -62,3 +63,49 @@ function nbd_server_start_unix_socket()
> > >   $QEMU_NBD -v -t -k "$nbd_unix_socket" $@ &
> > >   nbd_server_wait_for_unix_socket $!
> > >   }
> > > +
> > > +function nbd_server_set_tcp_port()
> > > +{
> > > +    for port in `seq 10809 10909`
> > > +    do
> > > +    socat TCP:$nbd_tcp_addr:$port STDIO < /dev/null 1>/dev/null 2>&1
> > 
> > This is the first use of socat in iotests.  Might not be the most
> > portable, but I don't know if I have better ideas.
> > nbdkit.git/tests/test-ip.sh greps the output of 'ss -ltn' to locate free
> > ports, but I don't know if ss is any better than socat.
> 
> So, I'm planning to squash this in, to use ss instead of socat, as follows:

Personally I prefer socat since it is more portable, per my previous
message.

> diff --git i/tests/qemu-iotests/233.out w/tests/qemu-iotests/233.out
> index eaa410c2703..eb4077f9fd7 100644
> --- i/tests/qemu-iotests/233.out
> +++ w/tests/qemu-iotests/233.out
> @@ -11,12 +11,10 @@ Generating a signed certificate...
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> 
>  == check TLS client to plain server fails ==
> -option negotiation failed: read failed: Unexpected end-of-file before all
> bytes were read
>  qemu-img: Could not open
> 'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': Denied by server for
> option 5 (starttls)
>  server reported: TLS not configured
> 
>  == check plain client to TLS server fails ==
> -option negotiation failed: read failed: Unexpected end-of-file before all
> bytes were read
>  qemu-img: Could not open 'nbd://localhost:10809': TLS negotiation required
> before option 8 (structured reply)
>  server reported: Option 0x8 not permitted before TLS
>  write failed (error message): Unable to write to socket: Broken pipe
> 
> 
> Also, you have to sanitize 233.out to change 10809 into PORT, so the test
> can still pass when it picked a different port.

Opps, yes.

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

Re: [Qemu-block] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Daniel P . Berrangé]

On Fri, Nov 16, 2018 at 11:20:26AM -0600, Eric Blake wrote:
> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> > Add tests that validate it is possible to connect to an NBD server
> > running TLS mode. Also test mis-matched TLS vs non-TLS connections
> > correctly fail.
> > ---
> >   tests/qemu-iotests/233| 99 +++
> >   tests/qemu-iotests/233.out| 33 
> >   tests/qemu-iotests/common.nbd | 47 +
> >   tests/qemu-iotests/group  |  1 +
> >   4 files changed, 180 insertions(+)
> >   create mode 100755 tests/qemu-iotests/233
> >   create mode 100644 tests/qemu-iotests/233.out
> 
> Adding tests is non-invasive, so I have no objection to taking the entire
> series in 3.1.  Do you want me to touch up the nits I found earlier, or
> should I wait for a v2?

If you're happy to touch it up, that's fine with me.

> > +
> > +$QEMU_IMG info nbd://localhost:$nbd_tcp_port
> > +
> > +echo
> > +echo "== check TLS works =="
> > +$QEMU_IMG info --image-opts \
> > +--object tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0 
> > \
> > +driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0
> 
> Is it also worth reading or even writing, to ensure that more than just the
> handshake is working?

I was mostly interested in the handshake/cert stuff, but yes, we could
do some qemu-io testing too.

> > +
> > +echo
> > +echo "== check TLS with different CA fails =="
> > +$QEMU_IMG info --image-opts \
> > +--object tls-creds-x509,dir=${tls_dir}/client2,endpoint=client,id=tls0 
> > \
> > +driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0
> > +
> > +# success, all done
> 
> > +== check TLS client to plain server fails ==
> > +option negotiation failed: read failed: Unexpected end-of-file before all 
> > bytes were read
> 
> Annoying message; I wonder if we can clean that up. But not this patch's
> problem.

This is a result of using the 'socat' check to poll for qemu-nbd
readiness. When socat finally succeeds in connecting & then drops
the conenction, qemu-nbd prints this message.  Personally I don't
think we need remove it unless we want to make qemu-nbd silently
by default when clients do unusual things.

> > +qemu-img: Could not open 
> > 'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': Denied by server for 
> > option 5 (starttls)
> > +server reported: TLS not configured
> 
> This 2-line message is better (and as such, explains why I think the message
> about early EOF should be silenced in this case).
> 
> > +
> > +== check plain client to TLS server fails ==
> > +option negotiation failed: read failed: Unexpected end-of-file before all 
> > bytes were read
> > +qemu-img: Could not open 'nbd://localhost:10809': TLS negotiation required 
> > before option 8 (structured reply)
> > +server reported: Option 0x8 not permitted before TLS
> 
> Same story about a redundant message.

Again this was the socat polling.

> 
> > +write failed (error message): Unable to write to socket: Broken pipe
> 
> Hmm - is this the client complaining that it can't send more to the server
> (that's a bug if the server hung up, since the protocol allows the client to
> change its mind and try TLS after all), or the server complaining that the
> client hung up abruptly without sending NBD_OPT_ABORT? Qemu as client either
> tries TLS right away, or knows that if the server requires TLS and the
> client has no TLS credentials then the client will never succeed, so the
> client gives up - but it SHOULD be giving up with NBD_OPT_ABORT rather than
> just disconnecting.  I'll have to play with that.  Doesn't impact your
> patch, but might spur a followup fix :)

I'm unclear if this message comes from the server or the client since
they are intermingled.


> > diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
> > index 61e9e90fee..0483ea7c55 100644
> > --- a/tests/qemu-iotests/common.nbd
> > +++ b/tests/qemu-iotests/common.nbd
> > @@ -20,6 +20,7 @@
> >   #
> >   nbd_unix_socket="${TEST_DIR}/qemu-nbd.sock"
> > +nbd_tcp_addr="127.0.0.1"
> 
> Should this be in your earlier patch that created common.nbd? Maybe not, as
> it doesn't get used until the functions you add here for tcp support. Still,
> I wonder if we should split the addition of tcp support separate from the
> new test.

Yeah, I wanted the earlier patch to be a plain refactor of existing
functionality, not adding anything new.

> >   nbd_pid_file="${TEST_DIR}/qemu-nbd.pid"
> >   function nbd_server_stop()
> > @@ -62,3 +63,49 @@ function nbd_server_start_unix_socket()
> >   $QEMU_NBD -v -t -k "$nbd_unix_socket" $@ &
> >   nbd_server_wait_for_unix_socket $!
> >   }
> > +
> > +function nbd_server_set_tcp_port()
> > +{
> > +for port in `seq 10809 10909`
> > +do
> > +   socat TCP:$nbd_tcp_addr:$port STDIO < /dev/null 1>/dev/null 2>&1
> 
> This is the first use of socat in iotests.  Might not be the most portable,
> but I don't know if I have 

Re: [Qemu-block] [PATCH 3/6] tests: pull qemu-nbd iotest helpers into common.nbd file

[Daniel P . Berrangé]

On Sat, Nov 17, 2018 at 09:01:57PM -0600, Eric Blake wrote:
> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> > The helpers for starting/stopping qemu-nbd in 058 will be useful in
> > other test cases, so move them into a common.nbd file.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> 
> > +function nbd_server_start_unix_socket()
> > +{
> > +nbd_server_stop
> > +$QEMU_NBD -v -t -k "$nbd_unix_socket" $@ &
> 
> Needs to be "$@" to properly preserve whitespace and/or empty arguments (the
> latter if someone passes -x '' for a default-named export).

ok

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

Re: [Qemu-block] [PATCH 2/6 for-3.1] nbd: stop waiting for a NBD response with NBD_CMD_DISC

[Daniel P . Berrangé]

On Sat, Nov 17, 2018 at 08:19:10PM -0600, Eric Blake wrote:
> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> > When sending a NBD_CMD_DISC message there is no reply expected,
> > however, the nbd_read_eof() coroutine is still waiting for a reply.
> > In a plain NBD connection this doesn't matter as it will just get an
> > EOF, however, on a TLS connection it will get an interrupted TLS data
> > packet. The nbd_read_eof() will then print an error message on the
> > console due to missing reply to NBD_CMD_DISC.
> > 
> > This can be seen with qemu-img
> > 
> >$ qemu-img info \
> >--object tls-creds-x509,dir=tlsdir,id=tls0,endpoint=client \
> >--image-opts driver=nbd,host=127.0.0.1,port=9000,tls-creds=tls0
> >qemu-img: Cannot read from TLS channel: Input/output error
> >image: nbd://127.0.0.1:9000
> >file format: nbd
> >virtual size: 10M (10485760 bytes)
> >disk size: unavailable
> > 
> > Simply setting the 'quit' flag after sending NBD_CMD_DISC is enough to
> > get the coroutine to stop waiting for a reply and thus supress the error
> > message.
> 
> Actually, it's not quite enough - once you actually start performing I/O,
> enough coroutines are kicked off that the error still happens:

Hmm, does the client not send requests synchronously  ? I expected that
any other I/O operations would have already received their reply by the
time we sent the DISC command.

> 
> $  qemu-io -c 'r 1m 1m' -c 'w -P 0x22 1m 1m' --image-opts \
> --object tls-creds-x509,dir=scratch/tls/client1,endpoint=client,id=tls0\
>  driver=nbd,host=localhost,port=10809,tls-creds=tls0
> read 1048576/1048576 bytes at offset 1048576
> 1 MiB, 1 ops; 0.0430 sec (23.204 MiB/sec and 23.2040 ops/sec)
> wrote 1048576/1048576 bytes at offset 1048576
> 1 MiB, 1 ops; 0.0152 sec (65.479 MiB/sec and 65.4793 ops/sec)
> Cannot read from TLS channel: Input/output error
> 
> Squashing this in on top of your patch helps, though:
> 
> diff --git i/block/nbd-client.c w/block/nbd-client.c
> index 5f63e4b8f15..e7916c78996 100644
> --- i/block/nbd-client.c
> +++ w/block/nbd-client.c
> @@ -79,7 +79,14 @@ static coroutine_fn void nbd_read_reply_entry(void
> *opaque)
>  assert(s->reply.handle == 0);
>  ret = nbd_receive_reply(s->ioc, >reply, _err);
>  if (local_err) {
> -error_report_err(local_err);
> +/* If we are already quitting, either another error has
> + * already been reported, or we requested NBD_CMD_DISC and
> + * don't need to report anything further.  */
> +if (!s->quit) {
> +error_report_err(local_err);
> +} else {
> +error_free(local_err);
> +}
>  }
>  if (ret <= 0) {
>  break;
> 
> But I want to do more testing to make sure I'm not missing out on reporting
> an actual error if I add that.

Yes, I'd be a little concerned about missing reporting of an error from
a command other than NBD_CMD_DISC if we did this.

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

Re: [Qemu-block] [PATCH 5/6] tests: add iotests helpers for dealing with TLS certificates

[Daniel P . Berrangé]

On Fri, Nov 16, 2018 at 10:39:03AM -0600, Eric Blake wrote:
> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> > Add helpers to common.tls for creating TLS certificates for a CA,
> > server and client.
> 
> MUCH appreciated!  We NEED this coverage, easily automated.
> 
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >   tests/qemu-iotests/common.tls | 139 ++
> >   1 file changed, 139 insertions(+)
> >   create mode 100644 tests/qemu-iotests/common.tls
> > 
> > diff --git a/tests/qemu-iotests/common.tls b/tests/qemu-iotests/common.tls
> > new file mode 100644
> 
> I was a bit surprised that this wasn't 100755, but this matches the fact
> that none of the other common.* are executable. And after thinking more, it
> makes sense - they aren't standalone scripts, but designed to be sourced,
> and 'source' doesn't care about execute bits.
> 
> > +tls_dir="${TEST_DIR}/tls"
> > +
> > +function tls_x509_cleanup()
> > +{
> > +rm -f ${tls_dir}/*.pem
> > +rm -f ${tls_dir}/*/*.pem
> > +rmdir ${tls_dir}/*
> > +rmdir ${tls_dir}
> 
> Why not just:
> rm -rf $tls_dir

Yeah, I guess we could do that for simplicity

> Also, the quoting is a bit inconsistent. if ${TEST_DIR} can contain spaces,
> then all uses of ${tls_dir} need to be in "".

Hmm, yes.

> > +}
> > +
> > +
> > +function tls_x509_init()
> > +{
> > +mkdir "${tls_dir}"
> 
> And this just highlights the quoting inconsistency.  Should this use mkdir
> -p?

I assume $TEST_DIR would already exist, so wouldn't need -p.

> > +
> > +function tls_x509_create_root_ca()
> > +{
> > +name=$1
> > +
> > +test -z "$name" && name=ca-cert
> 
> Could also be shortened as:
> 
> name=${1:-ca-cert}

ok

> > +
> > +cat > ${tls_dir}/ca.info < > +cn = Cthulu Dark Lord Enterprises $name
> 
> s/Cthulu/Cthulhu/ - after all, we don't want him coming after us just
> because we botched the spelling of his name :)
> 
> > +ca
> > +cert_signing_key
> > +EOF
> > +
> > +certtool --generate-self-signed \
> > + --load-privkey ${tls_dir}/key.pem \
> > + --template ${tls_dir}/ca.info \
> > + --outfile ${tls_dir}/$name-cert.pem 2>&1 | head -1
> 
> More missing ""
> 
> > +
> > +rm -f ${tls_dir}/ca.info
> > +}
> > +
> > +
> > +function tls_x509_create_server()
> > +{
> > +caname=$1
> > +name=$2
> > +
> > +mkdir ${tls_dir}/$name
> > +cat > ${tls_dir}/cert.info < > +organization = Cthulu Dark Lord Enterprises $name
> 
> Matched spelling
> 
> > +function tls_x509_create_client()
> > +{
> > +caname=$1
> > +name=$2
> > +
> > +mkdir ${tls_dir}/$name
> > +cat > ${tls_dir}/cert.info < > +country = South Pacific
> > +locality =  R'lyeh
> > +organization = Cthulu Dark Lord Enterprises $name
> 
> And again
> 
> Needs several touch-ups, but the idea itself is sound.

Yes will fix

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

Re: [Qemu-block] [PATCH 4/6] tests: check if qemu-nbd is still alive before waiting

[Daniel P . Berrangé]

On Fri, Nov 16, 2018 at 10:24:54AM -0600, Eric Blake wrote:
> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> > If the qemu-nbd UNIX socket has not shown up, the tests will sleep a bit
> > and then check again repeatedly for upto 30 seconds. This is pointless
> 
> s/upto/up to/
> 
> > if the qemu-nbd process has quit due to an error, so check whether the
> > pid is still alive before waiting and retrying.
> 
> "But our tests are perfect and qemu-nbd never fails" :)

Well the key benefit for this is actually people writing new tests,
like me with this series, who continually screw up the argv syntax
to qemu-nbd and don't want to wait 30 seconds for it to fail :)

> Yes, this makes sense.  Not 3.1 material on its own (after all, our
> testsuite isn't showing such failures, so we aren't wasting that time at the
> moment) - but worth including if the later patches end up in 3.1.
> 
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >   tests/qemu-iotests/common.nbd | 10 +-
> >   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Eric Blake 
> 
> > 
> > diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
> > index f920a578f1..61e9e90fee 100644
> > --- a/tests/qemu-iotests/common.nbd
> > +++ b/tests/qemu-iotests/common.nbd
> > @@ -37,11 +37,19 @@ function nbd_server_stop()
> >   function 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
> 
> Maybe the echo should be redirected to stderr.  But we aren't consistently
> doing that in other tests (_init_error does it, but other spots in check are
> not), so I'm not changing it.

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

Re: [Qemu-block] [PATCH 3/6] tests: pull qemu-nbd iotest helpers into common.nbd file

[Daniel P . Berrangé]

On Fri, Nov 16, 2018 at 03:43:16PM -0600, Eric Blake wrote:
> On 11/16/18 3:41 PM, Eric Blake wrote:
> 
> > > +#!/bin/bash
> > 
> > I know we're using bash,
> > 
> > > +
> > > +function nbd_server_stop()
> > > +{
> > 
> > > +function nbd_server_wait_for_unix_socket()
> > 
> > and bash supports 'function', but it is an obsolete syntactic sugar
> > thing that I don't recommend using.  (In ksh, it actually makes a
> > difference in behavior whether you use 'function' or not, and using it
> > in 'bash' makes it harder to port code over to 'ksh' - and hence in bash
> > it is obsolete because here it does NOT cause the change in behavior
> > that ksh users expect)
> > 
> 
> Of course, I hit send too soon, before getting to my punchline:
> 
> Since we already have so many existing iotests that use 'function', it's
> better to clean that up as a separate patch.

Yeah, I actually thought 'function' was the preferred syntax and
omitting it was bad since so many iotests used it :-)

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

[Qemu-block] [PATCH for-next? 0/2] qemu-img: Minor fixes to an amend error path

[Max Reitz]

One of the amend error paths has two issues that are fixed by this
series.  Since they are relatively minor and have been present in 3.0
already, I think there is no need to get them into 3.1.  OTOH they are
bug fixes, so they could go into 3.1 if you, dear reader, insist.


Max Reitz (2):
  qemu-img: Fix typo
  qemu-img: Fix leak

 qemu-img.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.17.2

[Qemu-block] [PATCH for-next? 1/2] qemu-img: Fix typo

[Max Reitz]

Fixes: d402b6a21a825a5c07aac9251990860723d49f5d
Reported-by: Kevin Wolf 
Cc: qemu-sta...@nongnu.org
Signed-off-by: Max Reitz 
---
 qemu-img.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 13a6ca31b4..a9a2470e1a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -261,7 +261,7 @@ static int print_block_option_help(const char *filename, 
const char *fmt)
 return 1;
 }
 if (!proto_drv->create_opts) {
-error_report("Protocal driver '%s' does not support image 
creation",
+error_report("Protocol driver '%s' does not support image 
creation",
  proto_drv->format_name);
 return 1;
 }
-- 
2.17.2

[Qemu-block] [PATCH for-next? 2/2] qemu-img: Fix leak

[Max Reitz]

create_opts was leaked here.  This is not too bad since the process is
about to exit anyway, but relying on that does not make the code nicer
to read.

Fixes: d402b6a21a825a5c07aac9251990860723d49f5d
Reported-by: Kevin Wolf 
Cc: qemu-sta...@nongnu.org
Signed-off-by: Max Reitz 
---
 qemu-img.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qemu-img.c b/qemu-img.c
index a9a2470e1a..ad04f59565 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -263,6 +263,7 @@ static int print_block_option_help(const char *filename, 
const char *fmt)
 if (!proto_drv->create_opts) {
 error_report("Protocol driver '%s' does not support image 
creation",
  proto_drv->format_name);
+qemu_opts_free(create_opts);
 return 1;
 }
 create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
-- 
2.17.2

Re: [Qemu-block] [PATCH for-3.1? v2 2/3] file-posix: Fix shared locks on reopen commit

[Alberto Garcia]

On Fri 16 Nov 2018 05:45:25 PM CET, Max Reitz  wrote:
> s->locked_shared_perm is the set of bits locked in the file, which is
> the inverse of the permissions actually shared.  So we need to pass them
> as they are to raw_apply_lock_bytes() instead of inverting them again.
>
> Reported-by: Alberto Garcia 
> Signed-off-by: Max Reitz 

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [PATCH for-3.1? v2 3/3] iotests: Test file-posix locking and reopen

[Alberto Garcia]

On Fri 16 Nov 2018 05:45:26 PM CET, Max Reitz  wrote:
> Signed-off-by: Max Reitz 

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [PATCH for-3.1? v2 1/3] block: Always abort reopen after prepare succeeded

[Alberto Garcia]

On Fri 16 Nov 2018 05:45:24 PM CET, Max Reitz wrote:
> bdrv_reopen_multiple() does not invoke bdrv_reopen_abort() for the
> element of the reopen queue for which bdrv_reopen_prepare() failed,
> because it assumes that the prepare function will have rolled back all
> changes already.
>
> However, bdrv_reopen_prepare() does not do this in every case: It may
> notice an error after BlockDriver.bdrv_reopen_prepare() succeeded, and
> it will not invoke BlockDriver.bdrv_reopen_abort() then; and neither
> will bdrv_reopen_multiple(), as explained above.
>
> This is wrong because we must always call .bdrv_reopen_commit() or
> .bdrv_reopen_abort() after .bdrv_reopen_prepare() has succeeded.
> Otherwise, the block driver has no chance to undo what it has done in
> its implementation of .bdrv_reopen_prepare().
>
> To fix this, bdrv_reopen_prepare() has to call .bdrv_reopen_abort() if
> it wants to return an error after .bdrv_reopen_prepare() has succeeded.
>
> Signed-off-by: Max Reitz 

Reviewed-by: Alberto Garcia 

Berto