[Qemu-devel] [PATCH 0/8] nbd: Fix failed assertion on negotiation error

[Qemu-devel] [PATCH 0/8] nbd: Fix failed assertion on negotiation error

[Max Reitz]

An error during negotiation, e.g. by the client trying to open an export
that does not exist, should not lead to a crash of the server process.

The middle six patches of this series are taken from my series
"block: Rework bdrv_close_all()", so here is a git-backport-diff against
v7 of that series:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/8:[down] 'nbd: client_close on error in nbd_co_client_start'
002/8:[----] [--] 'iotests: Rename filter_nbd to _filter_nbd in 083'
003/8:[0004] [FC] 'iotests: Change coding style of _filter_nbd in 083'
004/8:[0004] [FC] 'iotests: Move _filter_nbd into common.filter'
005/8:[0004] [FC] 'iotests: Make _filter_nbd drop log lines'
006/8:[----] [-C] 'iotests: Make _filter_nbd support more URL types'
007/8:[----] [--] 'iotests: Make redirecting qemu's stderr optional'
008/8:[down] 'iotests: Add test for a nonexistent NBD export'


Max Reitz (8):
  nbd: client_close on error in nbd_co_client_start
  iotests: Rename filter_nbd to _filter_nbd in 083
  iotests: Change coding style of _filter_nbd in 083
  iotests: Move _filter_nbd into common.filter
  iotests: Make _filter_nbd drop log lines
  iotests: Make _filter_nbd support more URL types
  iotests: Make redirecting qemu's stderr optional
  iotests: Add test for a nonexistent NBD export

 nbd/server.c                     |  3 +-
 tests/qemu-iotests/083           | 13 +------
 tests/qemu-iotests/083.out       | 10 ------
 tests/qemu-iotests/143           | 73 ++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/143.out       |  7 ++++
 tests/qemu-iotests/common.filter | 13 +++++++
 tests/qemu-iotests/common.qemu   | 15 +++++++--
 tests/qemu-iotests/group         |  1 +
 8 files changed, 109 insertions(+), 26 deletions(-)
 create mode 100755 tests/qemu-iotests/143
 create mode 100644 tests/qemu-iotests/143.out

-- 
2.7.0

[Qemu-devel] [PATCH 1/8] nbd: client_close on error in nbd_co_client_start

[Max Reitz]

Use client_close() if an error in nbd_co_client_start() occurs instead
of manually inlining parts of it. This fixes an assertion error on the
server side if nbd_negotiate() fails.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 nbd/server.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 2265cb0..5169b59 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1080,8 +1080,7 @@ static coroutine_fn void nbd_co_client_start(void *opaque)
         nbd_export_get(exp);
     }
     if (nbd_negotiate(data)) {
-        shutdown(client->sock, 2);
-        client->close(client);
+        client_close(client);
         goto out;
     }
     qemu_co_mutex_init(&client->send_lock);
-- 
2.7.0

[Qemu-devel] [PATCH 2/8] iotests: Rename filter_nbd to _filter_nbd in 083

[Max Reitz]

In the patch after the next, this function is moved to common.filter.
Therefore, its name should be preceded by an underscore to signify its
global availability.

To keep the code motion patch clean, we cannot rename it in the same
patch, so we need to choose some order of renaming vs. motion. It is
better to keep a supposedly global function used by only a single test
in that test than to keep a supposedly local function in a common* file
and use it from a test, so we should rename the function before moving
it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/083 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083
index 566da99..13495bc 100755
--- a/tests/qemu-iotests/083
+++ b/tests/qemu-iotests/083
@@ -49,7 +49,7 @@ wait_for_tcp_port() {
 	done
 }
 
-filter_nbd() {
+_filter_nbd() {
 	# nbd.c error messages contain function names and line numbers that are prone
 	# to change.  Message ordering depends on timing between send and receive
 	# callbacks sometimes, making them unreliable.
@@ -84,7 +84,7 @@ EOF
 
 	$PYTHON nbd-fault-injector.py $extra_args "127.0.0.1:$port" "$TEST_DIR/nbd-fault-injector.conf" 2>&1 >/dev/null &
 	wait_for_tcp_port "127\\.0\\.0\\.1:$port"
-	$QEMU_IO -c "read 0 512" "$nbd_url" 2>&1 | _filter_qemu_io | filter_nbd
+	$QEMU_IO -c "read 0 512" "$nbd_url" 2>&1 | _filter_qemu_io | _filter_nbd
 
 	echo
 }
-- 
2.7.0

[Qemu-devel] [PATCH 3/8] iotests: Change coding style of _filter_nbd in 083

[Max Reitz]

In order to be able to move _filter_nbd to common.filter in the next
patch, its coding style needs to be adapted to that of common.filter.
That means, we have to convert tabs to four spaces, adjust the alignment
of the last line (done with spaces already, assuming one tab equals
eight spaces), fix the line length of the comment, and add a line break
before the opening brace.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/083 | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083
index 13495bc..36e6de8 100755
--- a/tests/qemu-iotests/083
+++ b/tests/qemu-iotests/083
@@ -49,15 +49,16 @@ wait_for_tcp_port() {
 	done
 }
 
-_filter_nbd() {
-	# nbd.c error messages contain function names and line numbers that are prone
-	# to change.  Message ordering depends on timing between send and receive
-	# callbacks sometimes, making them unreliable.
-	#
-	# Filter out the TCP port number since this changes between runs.
-	sed -e 's#^.*nbd/.*\.c:.*##g' \
-	    -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \
-            -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#'
+_filter_nbd()
+{
+    # nbd.c error messages contain function names and line numbers that are
+    # prone to change.  Message ordering depends on timing between send and
+    # receive callbacks sometimes, making them unreliable.
+    #
+    # Filter out the TCP port number since this changes between runs.
+    sed -e 's#^.*nbd/.*\.c:.*##g' \
+        -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \
+        -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#'
 }
 
 check_disconnect() {
-- 
2.7.0

[Qemu-devel] [PATCH 4/8] iotests: Move _filter_nbd into common.filter

[Max Reitz]

_filter_nbd can be useful for other NBD tests, too, therefore it should
reside in common.filter.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/083           | 12 ------------
 tests/qemu-iotests/common.filter | 12 ++++++++++++
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083
index 36e6de8..aa99278 100755
--- a/tests/qemu-iotests/083
+++ b/tests/qemu-iotests/083
@@ -49,18 +49,6 @@ wait_for_tcp_port() {
 	done
 }
 
-_filter_nbd()
-{
-    # nbd.c error messages contain function names and line numbers that are
-    # prone to change.  Message ordering depends on timing between send and
-    # receive callbacks sometimes, making them unreliable.
-    #
-    # Filter out the TCP port number since this changes between runs.
-    sed -e 's#^.*nbd/.*\.c:.*##g' \
-        -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \
-        -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#'
-}
-
 check_disconnect() {
 	event=$1
 	when=$2
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index cfdb633..33ed1e4 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -230,5 +230,17 @@ _filter_qemu_img_map()
         -e 's/Mapped to *//' | _filter_testdir | _filter_imgfmt
 }
 
+_filter_nbd()
+{
+    # nbd.c error messages contain function names and line numbers that are
+    # prone to change.  Message ordering depends on timing between send and
+    # receive callbacks sometimes, making them unreliable.
+    #
+    # Filter out the TCP port number since this changes between runs.
+    sed -e 's#^.*nbd/.*\.c:.*##g' \
+        -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \
+        -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#'
+}
+
 # make sure this script returns success
 true
-- 
2.7.0

[Qemu-devel] [PATCH 5/8] iotests: Make _filter_nbd drop log lines

[Max Reitz]

The NBD log lines ("/your/source/dir/nbd.c:function():line: error")
should not be converted to empty lines but removed altogether.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/083.out       | 10 ----------
 tests/qemu-iotests/common.filter |  2 +-
 2 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/tests/qemu-iotests/083.out b/tests/qemu-iotests/083.out
index 78cc49a..ef3d1e3 100644
--- a/tests/qemu-iotests/083.out
+++ b/tests/qemu-iotests/083.out
@@ -51,7 +51,6 @@ no file open, try 'help open'
 
 === Check disconnect after neg2 ===
 
-
 read failed: Input/output error
 
 === Check disconnect 8 neg2 ===
@@ -66,42 +65,34 @@ no file open, try 'help open'
 
 === Check disconnect before request ===
 
-
 read failed: Input/output error
 
 === Check disconnect after request ===
 
-
 read failed: Input/output error
 
 === Check disconnect before reply ===
 
-
 read failed: Input/output error
 
 === Check disconnect after reply ===
 
-
 read failed: Input/output error
 
 === Check disconnect 4 reply ===
 
-
 read failed: Input/output error
 
 === Check disconnect 8 reply ===
 
-
 read failed: Input/output error
 
 === Check disconnect before data ===
 
-
 read failed: Input/output error
 
 === Check disconnect after data ===
 
-
 read 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
@@ -132,7 +123,6 @@ no file open, try 'help open'
 
 === Check disconnect after neg-classic ===
 
-
 read failed: Input/output error
 
 *** done
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 33ed1e4..e1bfdb7 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -237,7 +237,7 @@ _filter_nbd()
     # receive callbacks sometimes, making them unreliable.
     #
     # Filter out the TCP port number since this changes between runs.
-    sed -e 's#^.*nbd/.*\.c:.*##g' \
+    sed -e '/nbd\/.*\.c:/d' \
         -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \
         -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#'
 }
-- 
2.7.0

[Qemu-devel] [PATCH 6/8] iotests: Make _filter_nbd support more URL types

[Max Reitz]

This function should support URLs of the "nbd://" format (without
swallowing the export name), and for "nbd:///" URLs it should replace
"?socket=$TEST_DIR" by "?socket=TEST_DIR" because putting the Unix
socket files into the test directory makes sense.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/common.filter | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index e1bfdb7..84b7434 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -238,7 +238,8 @@ _filter_nbd()
     #
     # Filter out the TCP port number since this changes between runs.
     sed -e '/nbd\/.*\.c:/d' \
-        -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \
+        -e 's#nbd:\(//\)\?127\.0\.0\.1:[0-9]*#nbd:\1127.0.0.1:PORT#g' \
+        -e "s#?socket=$TEST_DIR#?socket=TEST_DIR#g" \
         -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#'
 }
 
-- 
2.7.0

[Qemu-devel] [PATCH 7/8] iotests: Make redirecting qemu's stderr optional

[Max Reitz]

Redirecting qemu's stderr to stdout makes working with the stderr output
difficult due to the other file descriptor magic performed in
_launch_qemu ("ambiguous redirect").

Add an option which specifies whether stderr should be redirected to
stdout or not (allowing for other modes to be added in the future).

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/common.qemu | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
index 8bf3969..2548a87 100644
--- a/tests/qemu-iotests/common.qemu
+++ b/tests/qemu-iotests/common.qemu
@@ -129,6 +129,8 @@ function _send_qemu_cmd()
 # $qemu_comm_method: set this variable to 'monitor' (case insensitive)
 #                    to use the QEMU HMP monitor for communication.
 #                    Otherwise, the default of QMP is used.
+# $keep_stderr: Set this variable to 'y' to keep QEMU's stderr output on stderr.
+#               If this variable is empty, stderr will be redirected to stdout.
 # Returns:
 # $QEMU_HANDLE: set to a handle value to communicate with this QEMU instance.
 #
@@ -151,11 +153,20 @@ function _launch_qemu()
     mkfifo "${fifo_out}"
     mkfifo "${fifo_in}"
 
-    QEMU_NEED_PID='y'\
-    ${QEMU} -nographic -serial none ${comm} -machine accel=qtest "${@}" \
+    if [ -z "$keep_stderr" ]; then
+        QEMU_NEED_PID='y'\
+        ${QEMU} -nographic -serial none ${comm} -machine accel=qtest "${@}" \
                                                                 >"${fifo_out}" \
                                                                 2>&1 \
                                                                 <"${fifo_in}" &
+    elif [ "$keep_stderr" = "y" ]; then
+        QEMU_NEED_PID='y'\
+        ${QEMU} -nographic -serial none ${comm} -machine accel=qtest "${@}" \
+                                                                >"${fifo_out}" \
+                                                                <"${fifo_in}" &
+    else
+        exit 1
+    fi
 
     if [[ "${BASH_VERSINFO[0]}" -ge "5" ||
         ("${BASH_VERSINFO[0]}" -ge "4"  &&  "${BASH_VERSINFO[1]}" -ge "1") ]]
-- 
2.7.0

[Qemu-devel] [PATCH 8/8] iotests: Add test for a nonexistent NBD export

[Max Reitz]

Trying to connect to a nonexistent NBD export should not crash the
server.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/143     | 73 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/143.out |  7 +++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 81 insertions(+)
 create mode 100755 tests/qemu-iotests/143
 create mode 100644 tests/qemu-iotests/143.out

diff --git a/tests/qemu-iotests/143 b/tests/qemu-iotests/143
new file mode 100755
index 0000000..6207368
--- /dev/null
+++ b/tests/qemu-iotests/143
@@ -0,0 +1,73 @@
+#!/bin/bash
+#
+# Test case for connecting to a non-existing NBD export name
+#
+# Copyright (C) 2016 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 <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+    rm -f "$TEST_DIR/nbd"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt generic
+_supported_proto generic
+_supported_os Linux
+
+keep_stderr=y \
+_launch_qemu 2> >(_filter_nbd)
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'qmp_capabilities' }" \
+    'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'nbd-server-start',
+       'arguments': { 'addr': { 'type': 'unix',
+                                'data': { 'path': '$TEST_DIR/nbd' }}}}" \
+    'return'
+
+# This should just result in a client error, not in the server crashing
+$QEMU_IO_PROG -f raw -c quit \
+    "nbd+unix:///no_such_export?socket=$TEST_DIR/nbd" 2>&1 \
+    | _filter_qemu_io | _filter_nbd
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'quit' }" \
+    'return'
+
+wait=1 _cleanup_qemu
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/143.out b/tests/qemu-iotests/143.out
new file mode 100644
index 0000000..dad2024
--- /dev/null
+++ b/tests/qemu-iotests/143.out
@@ -0,0 +1,7 @@
+QA output created by 143
+{"return": {}}
+{"return": {}}
+can't open device nbd+unix:///no_such_export?socket=TEST_DIR/nbd: Failed to read export length
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index d6e9219..ac6a959 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -142,3 +142,4 @@
 138 rw auto quick
 139 rw auto quick
 142 auto
+143 auto quick
-- 
2.7.0

Re: [Qemu-devel] [PATCH 1/8] nbd: client_close on error in nbd_co_client_start

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 524 bytes --]

On 01/25/2016 11:41 AM, Max Reitz wrote:
> Use client_close() if an error in nbd_co_client_start() occurs instead
> of manually inlining parts of it. This fixes an assertion error on the
> server side if nbd_negotiate() fails.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  nbd/server.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 3/8] iotests: Change coding style of _filter_nbd in 083

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 764 bytes --]

On 01/25/2016 11:41 AM, Max Reitz wrote:
> In order to be able to move _filter_nbd to common.filter in the next
> patch, its coding style needs to be adapted to that of common.filter.
> That means, we have to convert tabs to four spaces, adjust the alignment
> of the last line (done with spaces already, assuming one tab equals
> eight spaces), fix the line length of the comment, and add a line break
> before the opening brace.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/083 | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 4/8] iotests: Move _filter_nbd into common.filter

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 527 bytes --]

On 01/25/2016 11:41 AM, Max Reitz wrote:
> _filter_nbd can be useful for other NBD tests, too, therefore it should
> reside in common.filter.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/083           | 12 ------------
>  tests/qemu-iotests/common.filter | 12 ++++++++++++
>  2 files changed, 12 insertions(+), 12 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 5/8] iotests: Make _filter_nbd drop log lines

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1483 bytes --]

On 01/25/2016 11:41 AM, Max Reitz wrote:
> The NBD log lines ("/your/source/dir/nbd.c:function():line: error")

This line doesn't match the pre- or post-patch sed pattern.  Something
that would match would be "/your/source/dir/nbd/file.c:function():line:
error".  Maybe you should tweak the commit message?

> should not be converted to empty lines but removed altogether.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/083.out       | 10 ----------
>  tests/qemu-iotests/common.filter |  2 +-
>  2 files changed, 1 insertion(+), 11 deletions(-)

> +++ b/tests/qemu-iotests/common.filter
> @@ -237,7 +237,7 @@ _filter_nbd()
>      # receive callbacks sometimes, making them unreliable.
>      #
>      # Filter out the TCP port number since this changes between runs.
> -    sed -e 's#^.*nbd/.*\.c:.*##g' \
> +    sed -e '/nbd\/.*\.c:/d' \

The filter may be over-broad,...

>          -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \

...given that this line will be useless if anything in the resulting
message contains '.c'.  A tighter filter might be worth it (maybe
'^[!:]*nbd\/[!:]*\.c:/d', to at least limit it to lines where
"nbd/...c:" appears at the front of the line?  But that's a separate
change.  Your conversion looks sane by itself, so:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 8/8] iotests: Add test for a nonexistent NBD export

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 643 bytes --]

On 01/25/2016 11:41 AM, Max Reitz wrote:
> Trying to connect to a nonexistent NBD export should not crash the
> server.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/143     | 73 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/143.out |  7 +++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 81 insertions(+)
>  create mode 100755 tests/qemu-iotests/143
>  create mode 100644 tests/qemu-iotests/143.out
> 

Reviewed-by: Eric Blake <eblake@redhat.com>


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/8] nbd: client_close on error in nbd_co_client_start

[Daniel P. Berrange]

On Mon, Jan 25, 2016 at 07:41:08PM +0100, Max Reitz wrote:
> Use client_close() if an error in nbd_co_client_start() occurs instead
> of manually inlining parts of it. This fixes an assertion error on the
> server side if nbd_negotiate() fails.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  nbd/server.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 2265cb0..5169b59 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1080,8 +1080,7 @@ static coroutine_fn void nbd_co_client_start(void *opaque)
>          nbd_export_get(exp);
>      }
>      if (nbd_negotiate(data)) {
> -        shutdown(client->sock, 2);
> -        client->close(client);
> +        client_close(client);
>          goto out;
>      }
>      qemu_co_mutex_init(&client->send_lock);

The same as my fix last week :-)

https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg03409.html

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 1/8] nbd: client_close on error in nbd_co_client_start

[Kevin Wolf]

Am 25.01.2016 um 19:41 hat Max Reitz geschrieben:
> Use client_close() if an error in nbd_co_client_start() occurs instead
> of manually inlining parts of it. This fixes an assertion error on the
> server side if nbd_negotiate() fails.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Paolo, if you can Ack this one, I can take the series through my tree.

Kevin

>  nbd/server.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 2265cb0..5169b59 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1080,8 +1080,7 @@ static coroutine_fn void nbd_co_client_start(void *opaque)
>          nbd_export_get(exp);
>      }
>      if (nbd_negotiate(data)) {
> -        shutdown(client->sock, 2);
> -        client->close(client);
> +        client_close(client);
>          goto out;
>      }
>      qemu_co_mutex_init(&client->send_lock);
> -- 
> 2.7.0
> 

Re: [Qemu-devel] [PATCH 1/8] nbd: client_close on error in nbd_co_client_start

[Paolo Bonzini]

On 26/01/2016 10:32, Kevin Wolf wrote:
> Am 25.01.2016 um 19:41 hat Max Reitz geschrieben:
>> > Use client_close() if an error in nbd_co_client_start() occurs instead
>> > of manually inlining parts of it. This fixes an assertion error on the
>> > server side if nbd_negotiate() fails.
>> > 
>> > Signed-off-by: Max Reitz <mreitz@redhat.com>
> Paolo, if you can Ack this one, I can take the series through my tree.

Of course.  I had the same patch queued from Daniel.

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [Qemu-devel] [PATCH 0/8] nbd: Fix failed assertion on negotiation error

[Kevin Wolf]

Am 25.01.2016 um 19:41 hat Max Reitz geschrieben:
> An error during negotiation, e.g. by the client trying to open an export
> that does not exist, should not lead to a crash of the server process.
> 
> The middle six patches of this series are taken from my series
> "block: Rework bdrv_close_all()", so here is a git-backport-diff against
> v7 of that series:

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH 1/8] nbd: client_close on error in nbd_co_client_start

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1134 bytes --]

On 25.01.2016 23:05, Daniel P. Berrange wrote:
> On Mon, Jan 25, 2016 at 07:41:08PM +0100, Max Reitz wrote:
>> Use client_close() if an error in nbd_co_client_start() occurs instead
>> of manually inlining parts of it. This fixes an assertion error on the
>> server side if nbd_negotiate() fails.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  nbd/server.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/nbd/server.c b/nbd/server.c
>> index 2265cb0..5169b59 100644
>> --- a/nbd/server.c
>> +++ b/nbd/server.c
>> @@ -1080,8 +1080,7 @@ static coroutine_fn void nbd_co_client_start(void *opaque)
>>          nbd_export_get(exp);
>>      }
>>      if (nbd_negotiate(data)) {
>> -        shutdown(client->sock, 2);
>> -        client->close(client);
>> +        client_close(client);
>>          goto out;
>>      }
>>      qemu_co_mutex_init(&client->send_lock);
> 
> The same as my fix last week :-)
> 
> https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg03409.html

Oops, sorry. I guess that at least shows that the fix is probably good. O:-)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]