[Qemu-block] [PATCH v4 10/10] qemu-iotests: add support for running multi-threaded iotests

[Jeff Cody]

This adds support for running qemu-iotests in an arbitrary number
of sub-processes, so that tests can be run in parallel.

This necessarily changes the output format, although it should still
be familiar.  If you run in a single thread, the output format will
largely be the same as before this patch.

To run in more than one process, use the '-j num' option, e.g.:
  ./check -qcow2 -j 5

Some caveats:

* Some output format options, such as timestamps, are currently
  not compatible with multiple jobs.  If you select multiple
  jobs, timestamps will be disabled.

* Some tests may be more prone to failure with multiple jobs.
  This isn't a flaw of multiple jobs per se, but rather of
  fragile tests.  Some tests (181, 183) are very sensitive in
  timing, and high cpu loads can cause them to fail.  It may be
  worth adding support for 'single-thread only' tests in subsequent
  patches, that complete designated single-thread jobs at the end.

* Running protocol tests multi-threaded may fail, as multiple
  tests may try to bind the same address.

If '-j' is not specified, the default is a single iotest being run
at a time.

Signed-off-by: Jeff Cody 
---
 tests/qemu-iotests/check | 425 ---
 1 file changed, 293 insertions(+), 132 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 7908a9d..4f06715 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -26,6 +26,8 @@ n_bad=0
 bad=""
 notrun=""
 interrupt=true
+TEST_DIR_SEQ="$TEST_DIR"
+MAX_JOBS=1
 
 # by default don't output timestamps
 timestamp=${TIMESTAMP:=false}
@@ -125,6 +127,7 @@ sortme=false
 expunge=true
 have_test_arg=false
 cachemode=false
+multijob=false
 save_on_err=false
 
 tmp="${TEST_DIR}"/$$
@@ -220,6 +223,11 @@ s/ .*//p
 CACHEMODE_IS_DEFAULT=false
 cachemode=false
 continue
+elif $multijob
+then
+MAX_JOBS="$r"
+multijob=false
+continue
 fi
 
 xpand=true
@@ -262,9 +270,10 @@ other options
 -misalign   misalign memory allocations
 -n  show me, do not run tests
 -o options  -o options to pass to qemu-img create/convert
--T  output timestamps
+-T  output timestamps, disabled if using '-j'
 -c mode cache mode
 -s  save test scratch directory on test failure
+-j num  run tests in 'num' processes
 
 
 testlist options
@@ -442,6 +451,10 @@ testlist options
 save_on_err=true
 xpand=false
 ;;
+-j)
+multijob=true
+xpand=false
+;;
 '[0-9][0-9][0-9] [0-9][0-9][0-9][0-9]')
 echo "No tests?"
 status=1
@@ -506,6 +519,20 @@ BEGIN{ for (t='$start'; t<='$end'; t++) printf 
"%03d\n",t }' \
 
 done
 
+# No need for multi-process support, and this keeps output simpler
+if $showme
+then
+MAX_JOBS=1
+fi
+
+# TODO: Change test output format so that this can be useful
+#   with multi-process jobs
+if $timestamp && [ $MAX_JOBS -gt 1 ]
+then
+echo "Not showing timestamps with multi-job test"
+timestamp=false
+fi
+
 # Set qemu-io cache mode with $CACHEMODE we have
 QEMU_IO_OPTIONS="$QEMU_IO_OPTIONS --cache $CACHEMODE"
 
@@ -670,8 +697,11 @@ END{ if (NR > 0) {
 
 if [ ! -z "$notrun" ]
 then
-echo "Not run:$notrun"
-echo "Not run:$notrun" >>check.log
+# if run with $MAX_JOBS > 1, this will likely be
+# out of order
+notrun=$(echo $notrun|tr " " "\n"|sort|tr "\n" " ")
+echo "Not run: $notrun"
+echo "Not run: $notrun" >>check.log
 fi
 if [ ! -z "$n_bad" -a $n_bad != 0 ]
 then
@@ -694,7 +724,7 @@ END{ if (NR > 0) {
 rm -f $tmp.*
 }
 
-trap "_wrapup; exit \$status" 0 1 2 3 15
+trap "_wait_to_finish; _wrapup; exit \$status" 0 1 2 3 15
 
 [ -f $TIMESTAMP_FILE ] || touch $TIMESTAMP_FILE
 
@@ -718,11 +748,234 @@ seq="check"
 
 [ -n "$TESTS_REMAINING_LOG" ] && echo $list > $TESTS_REMAINING_LOG
 
+# Execute actual test.  This will be run in a background process.
+_do_test()
+{
+local seq=$1
+local err=false
+local tmp="${TEST_DIR}/$seq/$seq"
+local TEST_DIR_SEQ=$TEST_DIR/$seq
+
+_wallclock > "${TEST_DIR}/$seq.start.clock"
+$timestamp && printf %s "[$(date "+%T")]"
+
+if [ "$(head -n 1 "$source_iotests/$seq")" == "#!/usr/bin/env python" ]; 
then
+run_command="$PYTHON $seq"
+else
+run_command="./$seq"
+fi
+export OUTPUT_DIR=$PWD
+if $debug; then
+# Do this in a sub-shell, so we are operating on the right
+# TEST_DIR / QEMU_TEST_DIR
+(
+export TEST_DIR=$TEST_DIR_SEQ
+. "$source_iotests/common.config"
+. "$source_iotests/common.rc"
+cd 

[Qemu-block] [PATCH v4 04/10] qemu-iotests: remove file cleanup from bash tests

[Jeff Cody]

All files for a given test are now self-contained in a subdirectory,
and therefore the "./check" script can do all file-related cleanup
without any help.

This removes file cleanups from the bash tests.  The only cleanup left
is whatever is needed to kill any spawned processes; e.g. _cleanup_qemu.

Signed-off-by: Jeff Cody 
---
 tests/qemu-iotests/001 |  6 --
 tests/qemu-iotests/002 |  6 --
 tests/qemu-iotests/003 |  6 --
 tests/qemu-iotests/004 |  6 --
 tests/qemu-iotests/005 |  6 --
 tests/qemu-iotests/007 |  7 ---
 tests/qemu-iotests/008 |  6 --
 tests/qemu-iotests/009 |  6 --
 tests/qemu-iotests/010 |  6 --
 tests/qemu-iotests/011 |  6 --
 tests/qemu-iotests/012 |  6 --
 tests/qemu-iotests/013 |  6 --
 tests/qemu-iotests/014 |  6 --
 tests/qemu-iotests/015 |  7 ---
 tests/qemu-iotests/017 |  6 --
 tests/qemu-iotests/018 |  6 --
 tests/qemu-iotests/019 |  8 
 tests/qemu-iotests/020 |  8 
 tests/qemu-iotests/021 |  6 --
 tests/qemu-iotests/022 |  6 --
 tests/qemu-iotests/023 |  6 --
 tests/qemu-iotests/024 |  8 
 tests/qemu-iotests/025 |  6 --
 tests/qemu-iotests/026 |  7 ---
 tests/qemu-iotests/027 |  6 --
 tests/qemu-iotests/028 |  8 
 tests/qemu-iotests/029 |  7 ---
 tests/qemu-iotests/031 |  6 --
 tests/qemu-iotests/032 |  6 --
 tests/qemu-iotests/033 |  6 --
 tests/qemu-iotests/034 |  6 --
 tests/qemu-iotests/035 |  6 --
 tests/qemu-iotests/036 |  6 --
 tests/qemu-iotests/037 |  6 --
 tests/qemu-iotests/038 |  6 --
 tests/qemu-iotests/039 |  6 --
 tests/qemu-iotests/042 |  6 --
 tests/qemu-iotests/043 |  7 ---
 tests/qemu-iotests/046 |  6 --
 tests/qemu-iotests/047 |  6 --
 tests/qemu-iotests/048 |  8 
 tests/qemu-iotests/048.out |  1 -
 tests/qemu-iotests/049 |  6 --
 tests/qemu-iotests/050 |  8 
 tests/qemu-iotests/051 |  6 --
 tests/qemu-iotests/052 |  6 --
 tests/qemu-iotests/053 |  7 ---
 tests/qemu-iotests/054 |  6 --
 tests/qemu-iotests/058 | 47 ++
 tests/qemu-iotests/059 |  7 ---
 tests/qemu-iotests/060 |  6 --
 tests/qemu-iotests/061 |  6 --
 tests/qemu-iotests/062 |  6 --
 tests/qemu-iotests/063 |  7 ---
 tests/qemu-iotests/064 |  6 --
 tests/qemu-iotests/066 |  6 --
 tests/qemu-iotests/068 |  6 --
 tests/qemu-iotests/069 |  6 --
 tests/qemu-iotests/070 |  6 --
 tests/qemu-iotests/071 |  6 --
 tests/qemu-iotests/072 |  6 --
 tests/qemu-iotests/073 |  6 --
 tests/qemu-iotests/074 |  9 -
 tests/qemu-iotests/074.out |  1 -
 tests/qemu-iotests/075 |  6 --
 tests/qemu-iotests/076 |  6 --
 tests/qemu-iotests/077 |  6 --
 tests/qemu-iotests/078 |  6 --
 tests/qemu-iotests/079 |  6 --
 tests/qemu-iotests/080 |  7 ---
 tests/qemu-iotests/081 |  8 
 tests/qemu-iotests/082 |  6 --
 tests/qemu-iotests/083 |  8 
 tests/qemu-iotests/084 |  6 --
 tests/qemu-iotests/085 | 13 +
 tests/qemu-iotests/086 |  6 --
 tests/qemu-iotests/088 |  7 ---
 tests/qemu-iotests/089 |  6 --
 tests/qemu-iotests/090 |  6 --
 tests/qemu-iotests/091 |  8 +---
 tests/qemu-iotests/092 |  7 ---
 tests/qemu-iotests/094 |  9 +
 tests/qemu-iotests/095 |  8 +---
 tests/qemu-iotests/097 |  7 ---
 tests/qemu-iotests/098 |  7 ---
 tests/qemu-iotests/099 |  6 --
 tests/qemu-iotests/101 |  6 --
 tests/qemu-iotests/102 |  7 +--
 tests/qemu-iotests/103 |  6 --
 tests/qemu-iotests/104 |  2 --
 tests/qemu-iotests/105 |  6 --
 tests/qemu-iotests/106 |  6 --
 tests/qemu-iotests/107 |  6 --
 tests/qemu-iotests/108 |  6 --
 tests/qemu-iotests/109 |  8 +---
 tests/qemu-iotests/110 |  6 --
 tests/qemu-iotests/111 |  6 --
 tests/qemu-iotests/112 |  6 --
 tests/qemu-iotests/113 |  6 --
 tests/qemu-iotests/114 |  6 --
 tests/qemu-iotests/115 |  6 --
 tests/qemu-iotests/116 |  6 --
 tests/qemu-iotests/117 |  7 +--
 tests/qemu-iotests/119 |  6 --
 tests/qemu-iotests/120 |  6 --
 tests/qemu-iotests/121 |  6 --
 tests/qemu-iotests/122 |  7 ---
 tests/qemu-iotests/123 |  7 ---
 tests/qemu-iotests/125 |  6 --
 tests/qemu-iotests/130 |  7 +--
 tests/qemu-iotests/131 |  6 --
 tests/qemu-iotests/133 |  6 --
 tests/qemu-iotests/134 |  6 --
 tests/qemu-iotests/135 |  6 --
 

[Qemu-block] [PATCH v4 06/10] qemu-iotests: make ./check automatically reap QEMU processes

[Jeff Cody]

Check will now take care of cleaning up all QEMU processes started
from bash tests using the common.qemu framework.

This also paves the way to added another check option to keep QEMU
processes around, in the case of a failed test.

Signed-off-by: Jeff Cody 
---
 tests/qemu-iotests/085   | 2 --
 tests/qemu-iotests/091   | 2 --
 tests/qemu-iotests/094   | 2 --
 tests/qemu-iotests/095   | 2 --
 tests/qemu-iotests/102   | 2 --
 tests/qemu-iotests/109   | 2 --
 tests/qemu-iotests/117   | 2 --
 tests/qemu-iotests/130   | 2 --
 tests/qemu-iotests/140   | 2 --
 tests/qemu-iotests/141   | 2 --
 tests/qemu-iotests/143   | 2 --
 tests/qemu-iotests/144   | 2 --
 tests/qemu-iotests/146   | 2 --
 tests/qemu-iotests/156   | 2 --
 tests/qemu-iotests/173   | 2 --
 tests/qemu-iotests/181   | 2 --
 tests/qemu-iotests/183   | 2 --
 tests/qemu-iotests/185   | 2 --
 tests/qemu-iotests/191   | 6 --
 tests/qemu-iotests/check | 2 ++
 20 files changed, 2 insertions(+), 42 deletions(-)

diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index 7b69f86..283f9a9 100755
--- a/tests/qemu-iotests/085
+++ b/tests/qemu-iotests/085
@@ -37,8 +37,6 @@ snapshot_virt1="snapshot-v1.qcow2"
 
 SNAPSHOTS=10
 
-trap "_cleanup_qemu; exit \$status" 0 1 2 3 15
-
 # get standard environment, filters and checks
 . ./common.rc
 . ./common.filter
diff --git a/tests/qemu-iotests/091 b/tests/qemu-iotests/091
index c4df2fb..cc4c50c 100755
--- a/tests/qemu-iotests/091
+++ b/tests/qemu-iotests/091
@@ -31,8 +31,6 @@ status=1# failure is the default!
 
 MIG_FIFO="${TEST_DIR}/migrate"
 
-trap "_cleanup_qemu; exit \$status" 0 1 2 3 15
-
 # get standard environment, filters and checks
 . ./common.rc
 . ./common.filter
diff --git a/tests/qemu-iotests/094 b/tests/qemu-iotests/094
index 35e882c..1587550 100755
--- a/tests/qemu-iotests/094
+++ b/tests/qemu-iotests/094
@@ -27,8 +27,6 @@ echo "QA output created by $seq"
 here="$PWD"
 status=1   # failure is the default!
 
-trap "_cleanup_qemu; exit \$status" 0 1 2 3 15
-
 # get standard environment, filters and checks
 . ./common.rc
 . ./common.filter
diff --git a/tests/qemu-iotests/095 b/tests/qemu-iotests/095
index 75d60c4..2891014 100755
--- a/tests/qemu-iotests/095
+++ b/tests/qemu-iotests/095
@@ -30,8 +30,6 @@ echo "QA output created by $seq"
 here=`pwd`
 status=1   # failure is the default!
 
-trap "_cleanup_qemu; exit \$status" 0 1 2 3 15
-
 # get standard environment, filters and checks
 . ./common.rc
 . ./common.filter
diff --git a/tests/qemu-iotests/102 b/tests/qemu-iotests/102
index 201c520..2980638 100755
--- a/tests/qemu-iotests/102
+++ b/tests/qemu-iotests/102
@@ -27,8 +27,6 @@ echo "QA output created by $seq"
 here=$PWD
 status=1# failure is the default!
 
-trap "_cleanup_qemu; exit \$status" 0 1 2 3 15
-
 # get standard environment, filters and qemu instance handling
 . ./common.rc
 . ./common.filter
diff --git a/tests/qemu-iotests/109 b/tests/qemu-iotests/109
index d4fca99..2f6e456 100755
--- a/tests/qemu-iotests/109
+++ b/tests/qemu-iotests/109
@@ -27,8 +27,6 @@ echo "QA output created by $seq"
 here="$PWD"
 status=1   # failure is the default!
 
-trap "_cleanup_qemu; exit \$status" 0 1 2 3 15
-
 # get standard environment, filters and checks
 . ./common.rc
 . ./common.filter
diff --git a/tests/qemu-iotests/117 b/tests/qemu-iotests/117
index 579cecb..a427ee7 100755
--- a/tests/qemu-iotests/117
+++ b/tests/qemu-iotests/117
@@ -27,8 +27,6 @@ echo "QA output created by $seq"
 here="$PWD"
 status=1   # failure is the default!
 
-trap "_cleanup_qemu; exit \$status" 0 1 2 3 15
-
 # get standard environment, filters and checks
 . ./common.rc
 . ./common.filter
diff --git a/tests/qemu-iotests/130 b/tests/qemu-iotests/130
index 4aad4ea..3610738 100755
--- a/tests/qemu-iotests/130
+++ b/tests/qemu-iotests/130
@@ -29,8 +29,6 @@ echo "QA output created by $seq"
 here="$PWD"
 status=1   # failure is the default!
 
-trap "_cleanup_qemu; exit \$status" 0 1 2 3 15
-
 # get standard environment, filters and checks
 . ./common.rc
 . ./common.filter
diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140
index c5e1a5b..ec79402 100755
--- a/tests/qemu-iotests/140
+++ b/tests/qemu-iotests/140
@@ -31,8 +31,6 @@ echo "QA output created by $seq"
 here="$PWD"
 status=1   # failure is the default!
 
-trap "_cleanup_qemu; exit \$status" 0 1 2 3 15
-
 # get standard environment, filters and checks
 . ./common.rc
 . ./common.filter
diff --git a/tests/qemu-iotests/141 b/tests/qemu-iotests/141
index cff2319..39b75a4 100755
--- a/tests/qemu-iotests/141
+++ b/tests/qemu-iotests/141
@@ -27,8 +27,6 @@ echo "QA output created by $seq"
 here="$PWD"
 status=1   # failure is the default!
 
-trap "_cleanup_qemu; exit \$status" 0 1 2 3 15
-
 # get standard environment, filters and checks
 . ./common.rc
 . ./common.filter
diff --git a/tests/qemu-iotests/143 b/tests/qemu-iotests/143
index b4736aa..e107ae3 100755
--- a/tests/qemu-iotests/143
+++ b/tests/qemu-iotests/143
@@ 

[Qemu-block] [PATCH v4 00/10] qemu-iotests improvements

[Jeff Cody]

Previous series subject: "qemu-iotests: place output in unique dir"

Significant changes from v3.  Highlights of the major changes from v3:

* Python tests are properly run in hierarchial sub-directories now
   (Thanks Stefan, John)

* Protocol servers are cleaned up automatically (Thanks Kevin)

* Prevent running qemu-iotests if TEST_DIR contains spaces

* common.qemu process reaping overhauled, and qemu processes
  also automatically killed on test conclusion

* multi-thread iotest job support.  Here is an example of the
  speedup:


Previous, single-thread run of everything qcow2:


# ./check -qcow2

[...]

Not run: 045 059 064 070 075 076 077 078 081 083 084 088 092 093 094 101
 106 109 113 116 119 123 128 131 135 136 146 148 149 160 171 173 175
Failures: 191
Failed 1 of 149 tests

real8m22.077s
user4m48.177s
sys 1m16.553s


Multi-process run:

# ./check -qcow2 -j 5

[...]

Not run: 045 059 064 070 075 076 077 078 081 083 084 088 092 093 094 101
 106 109 113 116 119 123 128 131 135 136 146 148 149 160 171 173 175
Failures: 183 191
Failed 2 of 149 tests

real3m7.458s
user5m29.678s
sys 1m55.007s


(See commit message on patch 10 for why there is an additional test
 failure)


git-backport-diff -r qemu/master.. -u devel-iotests-v3
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/10:[down] 'qemu-iotests: refuse to run if TEST_DIR contains spaces'
002/10:[] [--] 'qemu-iotests: set TEST_DIR to a unique dir for each test'
003/10:[down] 'qemu-iotests: automatically clean up bash protocol servers'
004/10:[0073] [FC] 'qemu-iotests: remove file cleanup from bash tests'
005/10:[down] 'qemu-iotests: change qemu pid and fd tracking / cleanup'
006/10:[down] 'qemu-iotests: make ./check automatically reap QEMU processes'
007/10:[down] 'qemu-iotests: run python tests in own subdirectories'
008/10:[down] 'qemu-iotests: modify python tests to run from subdir'
009/10:[0017] [FC] 'qemu-iotests: add option to save temp files on error'
010/10:[down] 'qemu-iotests: add support for running multi-threaded iotests'


Jeff Cody (10):
  qemu-iotests: refuse to run if TEST_DIR contains spaces
  qemu-iotests: set TEST_DIR to a unique dir for each test
  qemu-iotests: automatically clean up bash protocol servers
  qemu-iotests: remove file cleanup from bash tests
  qemu-iotests: change qemu pid and fd tracking / cleanup
  qemu-iotests: make ./check automatically reap QEMU processes
  qemu-iotests: run python tests in own subdirectories
  qemu-iotests: modify python tests to run from subdir
  qemu-iotests: add option to save temp files on error
  qemu-iotests: add support for running multi-threaded iotests

 tests/qemu-iotests/001 |   6 -
 tests/qemu-iotests/002 |   6 -
 tests/qemu-iotests/003 |   6 -
 tests/qemu-iotests/004 |   6 -
 tests/qemu-iotests/005 |   6 -
 tests/qemu-iotests/007 |   7 -
 tests/qemu-iotests/008 |   6 -
 tests/qemu-iotests/009 |   6 -
 tests/qemu-iotests/010 |   6 -
 tests/qemu-iotests/011 |   6 -
 tests/qemu-iotests/012 |   6 -
 tests/qemu-iotests/013 |   6 -
 tests/qemu-iotests/014 |   6 -
 tests/qemu-iotests/015 |   7 -
 tests/qemu-iotests/017 |   6 -
 tests/qemu-iotests/018 |   6 -
 tests/qemu-iotests/019 |   8 -
 tests/qemu-iotests/020 |   8 -
 tests/qemu-iotests/021 |   6 -
 tests/qemu-iotests/022 |   6 -
 tests/qemu-iotests/023 |   6 -
 tests/qemu-iotests/024 |   8 -
 tests/qemu-iotests/025 |   6 -
 tests/qemu-iotests/026 |   7 -
 tests/qemu-iotests/027 |   6 -
 tests/qemu-iotests/028 |   8 -
 tests/qemu-iotests/029 |   7 -
 tests/qemu-iotests/030 |  82 -
 tests/qemu-iotests/031 |   6 -
 tests/qemu-iotests/032 |   6 -
 tests/qemu-iotests/033 |   6 -
 tests/qemu-iotests/034 |   6 -
 tests/qemu-iotests/035 |   6 -
 tests/qemu-iotests/036 |   6 -
 tests/qemu-iotests/037 |   6 -
 tests/qemu-iotests/038 |   6 -
 tests/qemu-iotests/039 |   6 -
 tests/qemu-iotests/040 | 128 ++---
 tests/qemu-iotests/041 | 333 -
 tests/qemu-iotests/042 |   6 -
 tests/qemu-iotests/043 |   7 -
 tests/qemu-iotests/044 |  11 +-
 tests/qemu-iotests/045 |  42 ++---
 tests/qemu-iotests/046 |   6 -
 tests/qemu-iotests/047 |   6 -
 tests/qemu-iotests/048 |   8 -
 tests/qemu-iotests/048.out |   1 -
 tests/qemu-iotests/049 |   6 -
 tests/qemu-iotests/050 |   8 -
 tests/qemu-iotests/051 |   6 -
 tests/qemu-iotests/052 |   6 -
 

[Qemu-block] [PATCH v4 02/10] qemu-iotests: set TEST_DIR to a unique dir for each test

[Jeff Cody]

Right now, all qemu-iotests output data into the same scratch directory,
and so each test needs to be responsible for cleaning up its own files.

Have each test use 'scratch/$seq' as its temp directory, so the check
script can do simple cleanup of removing the whole temporary directory.

Reviewed-by: Eric Blake 
Signed-off-by: Jeff Cody 
---
 tests/qemu-iotests/check | 21 +
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index e2163cc..c7b9526 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -713,6 +713,7 @@ seq="check"
 
 for seq in $list
 do
+TEST_DIR_SEQ=$TEST_DIR/$seq
 err=false
 printf %s "$seq"
 if [ -n "$TESTS_REMAINING_LOG" ] ; then
@@ -756,13 +757,23 @@ do
 fi
 export OUTPUT_DIR=$PWD
 if $debug; then
-(cd "$source_iotests";
+(
+export TEST_DIR=$TEST_DIR_SEQ
+. "$source_iotests/common.config"
+. "$source_iotests/common.rc"
+cd "$source_iotests" &&
 MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(($RANDOM % 255 + 1))} \
-$run_command -d 2>&1 | tee $tmp.out)
+$run_command -d 2>&1 | tee $tmp.out
+)
 else
-(cd "$source_iotests";
+(
+export TEST_DIR=$TEST_DIR_SEQ
+. "$source_iotests/common.config"
+. "$source_iotests/common.rc"
+ cd "$source_iotests" &&
 MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(($RANDOM % 255 + 1))} \
-$run_command >$tmp.out 2>&1)
+$run_command >$tmp.out 2>&1
+)
 fi
 sts=$?
 $timestamp && _timestamp
@@ -826,6 +837,8 @@ do
 fi
 fi
 
+rm -rf "$TEST_DIR_SEQ"
+
 fi
 
 # come here for each test, except when $showme is true
-- 
2.9.5

[Qemu-block] [PATCH v4 09/10] qemu-iotests: add option to save temp files on error

[Jeff Cody]

Now that ./check takes care of cleaning up after each tests, it
can also selectively not clean up.  Add option to leave all output from
tests intact if that test encountered an error.

Signed-off-by: Jeff Cody 
---
 tests/qemu-iotests/check | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index d2fb933..7908a9d 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -125,6 +125,7 @@ sortme=false
 expunge=true
 have_test_arg=false
 cachemode=false
+save_on_err=false
 
 tmp="${TEST_DIR}"/$$
 rm -f $tmp.list $tmp.tmp $tmp.sed
@@ -263,6 +264,8 @@ other options
 -o options  -o options to pass to qemu-img create/convert
 -T  output timestamps
 -c mode cache mode
+-s  save test scratch directory on test failure
+
 
 testlist options
 -g group[,group...]include tests from these groups
@@ -435,6 +438,10 @@ testlist options
 xgroup=true
 xpand=false
 ;;
+-s)
+save_on_err=true
+xpand=false
+;;
 '[0-9][0-9][0-9] [0-9][0-9][0-9][0-9]')
 echo "No tests?"
 status=1
@@ -852,7 +859,11 @@ do
 _cleanup_protocols
 _cleanup_qemu
 )
-rm -rf "$TEST_DIR_SEQ"
+
+if [ "$save_on_err" != "true" ] || [ "$err" != "true" ]
+then
+rm -rf "$TEST_DIR_SEQ"
+fi
 
 fi
 
-- 
2.9.5

[Qemu-block] [PATCH v4 07/10] qemu-iotests: run python tests in own subdirectories

[Jeff Cody]

This adds the framework to iotests.py to run python iotests in a
subdirectory structure, structured like so:

scratch/
├── TestNumber
│   ├── TestClassName
│   │   ├── test_method_name

Prior to this patch, tests were run in a test subdirectory from
previous patches in the series, like this:

scratch/
├── TestNumber

However, given the nature of python's unittest framework, additional
subdirectories are needed, if we want to insure that we can save
intermediate files in case of test failures (as we will do in a
subsequent patch) without running the risk of tainting other test
methods from the test file.

In python tests using iiotests.QMPTestCase, any reference to
'iotests.test_dir' should be replaced by 'self.workdir'.  This
may also require changing the scope of path name variables.

Suggested-by: Stefan Hajnoczi 
Signed-off-by: Jeff Cody 
---
 tests/qemu-iotests/iotests.py | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 6f05790..bf9fe6f 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -262,6 +262,16 @@ index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
 class QMPTestCase(unittest.TestCase):
 '''Abstract base class for QMP test cases'''
 
+def __init__(self, *args):
+super(QMPTestCase, self).__init__(*args)
+self.workdir = os.path.join(test_dir, self.__class__.__name__, 
self._testMethodName)
+try:
+os.makedirs(self.workdir)
+except OSError, error:
+if error.errno != errno.EEXIST:
+raise
+os.chdir(self.workdir)
+
 def dictpath(self, d, path):
 '''Traverse a path in a nested dict'''
 for component in path.split('/'):
-- 
2.9.5

[Qemu-block] [PATCH v4 01/10] qemu-iotests: refuse to run if TEST_DIR contains spaces

[Jeff Cody]

Currently, not all qemu-iotests work if TEST_DIR has spaces, and they
also might not be safe.  Refuse to run if TEST_DIR in this case, at
least until all tests are fixed sometime in the future.

Signed-off-by: Jeff Cody 
---
 tests/qemu-iotests/check | 8 
 1 file changed, 8 insertions(+)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index e6b6ff7..e2163cc 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -102,6 +102,14 @@ if [ -z "$TEST_DIR" ]; then
 TEST_DIR=`pwd`/scratch
 fi
 
+case $TEST_DIR in
+*[[:blank:]]*)
+echo "The TEST_DIR pathname '$TEST_DIR' contains whitespace. "
+echo "This is currently unsupported by qemu-iotests"
+exit 1
+;;
+esac
+
 if [ ! -e "$TEST_DIR" ]; then
 mkdir "$TEST_DIR"
 fi
-- 
2.9.5

[Qemu-block] [PATCH v4 05/10] qemu-iotests: change qemu pid and fd tracking / cleanup

[Jeff Cody]

So that later patches can cleanup running qemu processes called from
different subshells, track resources to cleanup in pid and fd list
files.

In subsquent patches, ./check will kill all QEMU processes launched with
the common.qemu framework, and the tests will not need to cleanup
manually (unless they want to do so as part of the test, e.g. wait for
a process to end such as migration).

Signed-off-by: Jeff Cody 
---
 tests/qemu-iotests/common.qemu | 65 +++---
 tests/qemu-iotests/common.rc   |  2 +-
 2 files changed, 49 insertions(+), 18 deletions(-)

diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
index 7b3052d..7bd21ea 100644
--- a/tests/qemu-iotests/common.qemu
+++ b/tests/qemu-iotests/common.qemu
@@ -193,6 +193,8 @@ function _launch_qemu()
 QEMU_OUT[${_QEMU_HANDLE}]=${_out_fd}
 QEMU_IN[${_QEMU_HANDLE}]=${_in_fd}
 QEMU_STATUS[${_QEMU_HANDLE}]=0
+echo ${_out_fd} >> "$QEMU_TEST_DIR/qemu-out-fd.lst"
+echo ${_in_fd} >> "$QEMU_TEST_DIR/qemu-in-fd.lst"
 
 if [ "${qemu_comm_method}" == "qmp" ]
 then
@@ -209,35 +211,64 @@ function _launch_qemu()
 
 # Silenty kills the QEMU process
 #
+# This is able to kill and clean up after QEMU processes without the
+# need for any subshell-specific variables, so long as the qemu-pidlist
+# and qemu-out-fd.list and qemu-in-fd.list files are properly maintained.
+#
 # If $wait is set to anything other than the empty string, the process will not
 # be killed but only waited for, and any output will be forwarded to stdout. If
 # $wait is empty, the process will be killed and all output will be suppressed.
 function _cleanup_qemu()
 {
-# QEMU_PID[], QEMU_IN[], QEMU_OUT[] all use same indices
-for i in "${!QEMU_OUT[@]}"
+local PIDFILE="${QEMU_TEST_DIR}"/qemu-pidlist.pid
+local OUT_FD_FILE="${QEMU_TEST_DIR}"/qemu-out-fd.lst
+local IN_FD_FILE="${QEMU_TEST_DIR}"/qemu-in-fd.lst
+
+if [ ! -e "${PIDFILE}" ]; then
+return
+fi
+
+# get line count, and therefore number of processes to kill
+numproc=$(wc -l "${PIDFILE}" | sed 's/\s.*//')
+
+for i in $(seq 1 $numproc)
 do
 local QEMU_PID
-if [ -f "${QEMU_TEST_DIR}/qemu-${i}.pid" ]; then
-read QEMU_PID < "${QEMU_TEST_DIR}/qemu-${i}.pid"
-rm -f "${QEMU_TEST_DIR}/qemu-${i}.pid"
-if [ -z "${wait}" ] && [ -n "${QEMU_PID}" ]; then
-kill -KILL ${QEMU_PID} 2>/dev/null
-fi
-if [ -n "${QEMU_PID}" ]; then
-wait ${QEMU_PID} 2>/dev/null # silent kill
-fi
+local OUT_FD
+local IN_FD
+
+QEMU_PID=$(tail -n+${i} "${PIDFILE}" | head -n1)
+OUT_FD=$(tail -n+${i} "${OUT_FD_FILE}" | head -n1)
+IN_FD=$(tail -n+${i} "${IN_FD_FILE}" | head -n1)
+
+if [ -z "${wait}" ] && [ -n "${QEMU_PID}" ]; then
+kill -KILL ${QEMU_PID} 2>/dev/null
+fi
+if [ -n "${QEMU_PID}" ]; then
+wait ${QEMU_PID} 2>/dev/null # silent kill
+fi
+
+if [ -n "${wait}" ] && [ -n "${OUT_FD}" ]; then
+cat <&${OUT_FD} | _filter_testdir | _filter_qemu \
+| _filter_qemu_io | _filter_qmp | _filter_hmp
 fi
 
-if [ -n "${wait}" ]; then
-cat <&${QEMU_OUT[$i]} | _filter_testdir | _filter_qemu \
-  | _filter_qemu_io | _filter_qmp | _filter_hmp
+if [ -n "${IN_FD}" ]; then
+eval "exec ${IN_FD}<&-"   # close file descriptors
+fi
+if [ -n "${OUT_FD}" ]; then
+eval "exec ${OUT_FD}<&-"
+fi
+if [ -n "${QEMU_FIFO_IN}" ]; then
+rm -f "${QEMU_FIFO_IN}_${i}"
+fi
+if [ -n "${QEMU_FIFO_OUT}" ]; then
+rm -f "${QEMU_FIFO_OUT}_${i}"
 fi
-rm -f "${QEMU_FIFO_IN}_${i}" "${QEMU_FIFO_OUT}_${i}"
-eval "exec ${QEMU_IN[$i]}<&-"   # close file descriptors
-eval "exec ${QEMU_OUT[$i]}<&-"
 
 unset QEMU_IN[$i]
 unset QEMU_OUT[$i]
 done
+
+rm -f "${PIDFILE}" "${OUT_FD_FILE}" "${IN_FD_FILE}"
 }
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index a345ffd..c81f712 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -51,7 +51,7 @@ _qemu_wrapper()
 {
 (
 if [ -n "${QEMU_NEED_PID}" ]; then
-echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"
+echo $BASHPID >> "${QEMU_TEST_DIR}/qemu-pidlist.pid"
 fi
 exec "$QEMU_PROG" $QEMU_OPTIONS "$@"
 )
-- 
2.9.5

[Qemu-block] [PATCH v4 03/10] qemu-iotests: automatically clean up bash protocol servers

[Jeff Cody]

For bash tests, this allows 'check' to reap all launch protocol
servers / processes, after a test has finished running.

Signed-off-by: Jeff Cody 
---
 tests/qemu-iotests/check | 13 +++
 tests/qemu-iotests/common.rc | 93 +---
 2 files changed, 75 insertions(+), 31 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index c7b9526..45fad05 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -757,6 +757,8 @@ do
 fi
 export OUTPUT_DIR=$PWD
 if $debug; then
+# Do this in a sub-shell, so we are operating on the right
+# TEST_DIR / QEMU_TEST_DIR
 (
 export TEST_DIR=$TEST_DIR_SEQ
 . "$source_iotests/common.config"
@@ -766,6 +768,8 @@ do
 $run_command -d 2>&1 | tee $tmp.out
 )
 else
+# Do this in a sub-shell, so we are operating on the right
+# TEST_DIR / QEMU_TEST_DIR
 (
 export TEST_DIR=$TEST_DIR_SEQ
 . "$source_iotests/common.config"
@@ -837,6 +841,15 @@ do
 fi
 fi
 
+# Do this in a sub-shell, so we are operating on the right
+# TEST_DIR / QEMU_TEST_DIR
+(
+export TEST_DIR=$TEST_DIR_SEQ
+. "$source_iotests/common.config"
+. "$source_iotests/common.rc"
+
+_cleanup_protocols
+)
 rm -rf "$TEST_DIR_SEQ"
 
 fi
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 0e8a33c..a345ffd 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -101,7 +101,7 @@ _qemu_nbd_wrapper()
 _qemu_vxhs_wrapper()
 {
 (
-echo $BASHPID > "${TEST_DIR}/qemu-vxhs.pid"
+echo $BASHPID > "${QEMU_TEST_DIR}/qemu-vxhs.pid"
 exec "$QEMU_VXHS_PROG" $QEMU_VXHS_OPTIONS "$@"
 )
 }
@@ -248,7 +248,7 @@ _make_test_img()
 
 # Start QNIO server on image directory for vxhs protocol
 if [ $IMGPROTO = "vxhs" ]; then
-eval "$QEMU_VXHS -d  $TEST_DIR > /dev/null &"
+eval "$QEMU_VXHS -d  $QEMU_TEST_DIR > /dev/null &"
 sleep 1 # Wait for server to come up.
 fi
 }
@@ -264,29 +264,64 @@ _rm_test_img()
 rm -f "$img"
 }
 
+_cleanup_nbd()
+{
+if [ -f "${QEMU_TEST_DIR}/qemu-nbd.pid" ]; then
+local QEMU_NBD_PID
+read QEMU_NBD_PID < "${QEMU_TEST_DIR}/qemu-nbd.pid"
+rm -f "${QEMU_TEST_DIR}/qemu-nbd.pid"
+kill ${QEMU_NBD_PID} >/dev/null 2>&1
+ fi
+}
+
+_cleanup_vxhs()
+{
+if [ -f "${QEMU_TEST_DIR}/qemu-vxhs.pid" ]; then
+local QEMU_VXHS_PID
+read QEMU_VXHS_PID < "${QEMU_TEST_DIR}/qemu-vxhs.pid"
+rm -f "${QEMU_TEST_DIR}/qemu-vxhs.pid"
+kill ${QEMU_VXHS_PID} >/dev/null 2>&1
+fi
+}
+
+_cleanup_rbd()
+{
+rbd --no-progress rm "$QEMU_TEST_DIR/t.$IMGFMT" > /dev/null
+}
+
+_cleanup_sheepdog()
+{
+collie vdi delete "$QEMU_TEST_DIR/t.$IMGFMT"
+}
+
+
+_cleanup_protocols()
+{
+# Some tests (e.g. 058) start some protocols
+# even though the protocol was not specified when running
+# check.  If the wrappers create pidfiles, go ahead and clean
+# up without checking $IMGPROTO.
+_cleanup_nbd
+_cleanup_vxhs
+
+case "$IMGPROTO" in
+
+rbd)
+_cleanup_rbd
+;;
+
+sheepdog)
+_cleanup_sheepdog
+;;
+
+esac
+}
+
 _cleanup_test_img()
 {
+_cleanup_protocols
+
 case "$IMGPROTO" in
-
-nbd)
-if [ -f "${QEMU_TEST_DIR}/qemu-nbd.pid" ]; then
-local QEMU_NBD_PID
-read QEMU_NBD_PID < "${QEMU_TEST_DIR}/qemu-nbd.pid"
-kill ${QEMU_NBD_PID}
-rm -f "${QEMU_TEST_DIR}/qemu-nbd.pid"
-fi
-rm -f "$TEST_IMG_FILE"
-;;
-vxhs)
-if [ -f "${TEST_DIR}/qemu-vxhs.pid" ]; then
-local QEMU_VXHS_PID
-read QEMU_VXHS_PID < "${TEST_DIR}/qemu-vxhs.pid"
-kill ${QEMU_VXHS_PID} >/dev/null 2>&1
-rm -f "${TEST_DIR}/qemu-vxhs.pid"
-fi
-rm -f "$TEST_IMG_FILE"
-;;
-
 file)
 _rm_test_img "$TEST_DIR/t.$IMGFMT"
 _rm_test_img "$TEST_DIR/t.$IMGFMT.orig"
@@ -298,16 +333,12 @@ _cleanup_test_img()
 TEST_IMG="$ORIG_TEST_IMG"
 fi
 ;;
-
-rbd)
-rbd --no-progress rm "$TEST_DIR/t.$IMGFMT" > /dev/null
-;;
-
-sheepdog)
-collie vdi delete "$TEST_DIR/t.$IMGFMT"
-;;
-
 esac
+
+if [ -n "$TEST_IMG_FILE" ]
+then
+rm -f "$TEST_IMG_FILE"
+fi
 }
 
 _check_test_img()
-- 
2.9.5

Re: [Qemu-block] [PATCH v1 1/1] iotests: fix the actual-size in 191.out

[QingFeng Hao]

在 2017/10/16 20:31, Eric Blake 写道:

On 10/16/2017 03:30 AM, Kevin Wolf wrote:

Am 16.10.2017 um 07:23 hat QingFeng Hao geschrieben:

The actual-size set in 191.out is wrong, which causes the test case failed
and shall be 331776.
The actual-size indicates the image's (e.g. t.qcow2.base) actual size
defined by ImageInfo.actual_size.

Signed-off-by: QingFeng Hao 

This is wrong. It may make the test work on your machine, but it breaks
it on mine.

The correct approach to fix this is Max' "[PATCH 2/2] iotests: Filter
actual image size in 184 and 191".

That, and ALL patches should be sent to qemu-de...@nongnu.org, in
addition to whatever other lists are appropriate (you are correct that
this patch should cc qemu-block, but you forgot qemu-devel).

Thanks for your reminder. I'll pay attention next time.




--
Regards
QingFeng Hao

Re: [Qemu-block] [PATCH v1 1/1] iotests: fix the actual-size in 191.out

[QingFeng Hao]

在 2017/10/16 16:30, Kevin Wolf 写道:

Am 16.10.2017 um 07:23 hat QingFeng Hao geschrieben:

The actual-size set in 191.out is wrong, which causes the test case failed
and shall be 331776.
The actual-size indicates the image's (e.g. t.qcow2.base) actual size
defined by ImageInfo.actual_size.

Signed-off-by: QingFeng Hao 

This is wrong. It may make the test work on your machine, but it breaks
it on mine.

The correct approach to fix this is Max' "[PATCH 2/2] iotests: Filter
actual image size in 184 and 191".
Thanks, I'll look into why it behaves differently in host file systems. 
Maybe it's
related with block size etc. If that's true, Max's patch to filter it 
makes sense.


Kevin



--
Regards
QingFeng Hao

Re: [Qemu-block] [Qemu-devel] [PATCH v4 4/8] nbd: Minimal structured read for server

[Eric Blake]

On 10/14/2017 08:01 PM, Eric Blake wrote:
> From: Vladimir Sementsov-Ogievskiy 
> 
> Minimal implementation of structured read: one structured reply chunk,
> no segmentation.
> Minimal structured error implementation: no text message.
> Support DF flag, but just ignore it, as there is no segmentation any
> way.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> Signed-off-by: Eric Blake 
> 

> +
> +case NBD_OPT_STRUCTURED_REPLY:
> +if (client->structured_reply) {
> +ret = nbd_negotiate_send_rep_err(
> +client->ioc, NBD_REP_ERR_INVALID, option, errp,
> +"structured reply already negotiated");
> +} else {
> +ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
> + option, errp);
> +}

Fails spectacularly if the client sent a non-zero length payload along
with the option, because we forgot to nbd_drop the payload, getting the
server out-of-sync with the client's next option request.  I'm
requesting clarification from the NBD list on how to best handle that
(presumably with NBD_REP_ERR_INVALID), and in the meantime will copy
what we do for NBD_OPT_LIST with unexpected payload.  It also made me
realize that we don't reject unexpected payload for NBD_OPT_STARTTLS; a
common helper function would make that easier, so guess what I'm adding
in v5 :)

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v4 1/8] nbd: Include error names in trace messages

[Eric Blake]

On 10/14/2017 08:01 PM, Eric Blake wrote:
> NBD errors were originally sent over the wire based on Linux errno
> values; but not all the world is Linux, and not all platforms share
> the same values.  Since a number isn't very easy to decipher on all
> platforms, update the trace messages to include the name of NBD
> errors being sent/received over the wire.  Tweak the trace messages
> to be at the point where we are using the NBD error, not the
> translation to the host errno values.
> 
> Signed-off-by: Eric Blake 
> ---

> +++ b/nbd/trace-events
> @@ -29,7 +29,7 @@ nbd_client_loop_ret(int ret, const char *error) "NBD loop 
> returned %d: %s"
>  nbd_client_clear_queue(void) "Clearing NBD queue"
>  nbd_client_clear_socket(void) "Clearing NBD socket"
>  nbd_send_request(uint64_t from, uint32_t len, uint64_t handle, uint16_t 
> flags, uint16_t type, const char *name) "Sending request to server: { .from = 
> %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags = 0x%" PRIx16 
> ", .type = %" PRIu16 " (%s) }"
> -nbd_receive_reply(uint32_t magic, int32_t error, uint64_t handle) "Got 
> reply: { magic = 0x%" PRIx32 ", .error = % " PRId32 ", handle = %" PRIu64" }"

Pre-existing unusual spacing in '.error = % " PRId32',

> +nbd_receive_reply(uint32_t magic, int32_t error, const char *errname, 
> uint64_t handle) "Got reply: { magic = 0x%" PRIx32 ", .error = % " PRId32 " 
> (%s), handle = %" PRIu64" }"

so I'll clean that up in the next revision.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 13/13] nbd: Minimal structured read for client

[Vladimir Sementsov-Ogievskiy]

13.10.2017 23:51, Eric Blake wrote:

On 10/12/2017 04:53 AM, Vladimir Sementsov-Ogievskiy wrote:

Minimal implementation: for structured error only error_report error
message.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/block/nbd.h |   6 +
  block/nbd-client.c  | 395 
  nbd/client.c|   7 +
  3 files changed, 379 insertions(+), 29 deletions(-)


The fun stuff!


diff --git a/include/block/nbd.h b/include/block/nbd.h
index 1ef8c8897f..e3350b67a4 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -203,6 +203,11 @@ enum {
  #define NBD_SREP_TYPE_ERROR NBD_SREP_ERR(1)
  #define NBD_SREP_TYPE_ERROR_OFFSET  NBD_SREP_ERR(2)
  
+static inline bool nbd_srep_type_is_error(int type)

+{
+return type & (1 << 15);
+}

Knock-on effects to your earlier reply that s/srep/reply/ was okay.
Again, I'm not a fan of inline functions until after all the structs and
constants have been declared, so I'd sink this a bit lower.


why? I put it here to be close to errors, and to NBD_REPLY_ERR which 
uses the same semantics..





+
  /* NBD errors are based on errno numbers, so there is a 1:1 mapping,
   * but only a limited set of errno values is specified in the protocol.
   * Everything else is squashed to EINVAL.
@@ -241,6 +246,7 @@ static inline int nbd_errno_to_system_errno(int err)
  struct NBDExportInfo {
  /* Set by client before nbd_receive_negotiate() */
  bool request_sizes;
+bool structured_reply;
  /* Set by server results during nbd_receive_negotiate() */

You are correct that the client has to set this before negotiate (if we
are in qemu-nbd and about to hand over to the kernel, we must NOT
request structured_reply unless the kernel has been patched to
understand structured replies - but upstream NBD isn't there yet; but if
we are using block/nbd-client.c, then we can request it). But we must
also check this AFTER negotiate, to see if the server understood our
request (how we handle reads depends on what mode the server is in).  So
maybe this needs another comment.


I just missed these comments. Accordingly to the patch, this field 
should be in the second section.

However, better is make it "in-out" as in your description.





+++ b/block/nbd-client.c
@@ -29,6 +29,7 @@
  
  #include "qemu/osdep.h"

  #include "qapi/error.h"
+#include "qemu/error-report.h"

What errors are we reporting directly, instead of feeding back through
errp?  Without seeing the rest of the patch yet, I'm suspicious of this
include.


I've postponed errp handling.. We have no errp parameters in 
.bdrv_co_preadv and friends, so where to feed them back?





  #include "nbd-client.h"
  
  #define HANDLE_TO_INDEX(bs, handle) ((handle) ^ (uint64_t)(intptr_t)(bs))

@@ -93,7 +94,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
  if (i >= MAX_NBD_REQUESTS ||
  !s->requests[i].coroutine ||
  !s->requests[i].receiving ||
-nbd_reply_is_structured(>reply))
+(nbd_reply_is_structured(>reply) && !s->info.structured_reply))
  {

Do we set a good error message when the server sends us garbage? [1]


no, and did not do it before.




  break;
  }
@@ -181,75 +182,406 @@ err:
  return rc;
  }
  
-static int nbd_co_receive_reply(NBDClientSession *s,

-uint64_t handle,
-QEMUIOVector *qiov)
+static inline void payload_advance16(uint8_t **payload, uint16_t **ptr)
+{
+*ptr = (uint16_t *)*payload;
+be16_to_cpus(*ptr);

Won't work.  This is a potentially unaligned cast, where you don't have


hmm, yes. payload is allocated by qemu_memalign, but it may be already 
"advanced".



the benefit of a packed struct, and the compiler will probably gripe at
you on platforms stricter than x86.  Instead, if I remember correctly,
we should use
  *ptr = ldw_be_p(*ptr);
Ditto to your other sizes.

Why not a helper for an 8-bit read?


as there is no usage for it




+static int nbd_parse_offset_hole_payload(NBDStructuredReplyChunk *chunk,
+ uint8_t *payload, QEMUIOVector *qiov)
+{
+uint64_t *offset;
+uint32_t *hole_size;
+
+if (chunk->length != sizeof(*offset) + sizeof(*hole_size)) {
+return -EINVAL;

Should we plumb in errp, and return a decent error message that way
rather than relying on a mere -EINVAL which forces us to drop connection
with the server and treat all remaining outstanding requests as EIO?

Thinking a bit more: If we get here, the server sent us the wrong number
of bytes - but we know from chunk->length how much data the server
claims to have coming, even if we don't know what to do with that data.
And we've already read the payload into malloc'd memory, so we are in
sync to read more replies from the server.  So we don't have to hang up,
but it's probably safer to hang up than to 

Re: [Qemu-block] [Qemu-devel] [PATCH v4 4/8] nbd: Minimal structured read for server

[Eric Blake]

On 10/16/2017 07:18 AM, Eric Blake wrote:
> On 10/16/2017 04:49 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 15.10.2017 04:01, Eric Blake wrote:
>>> From: Vladimir Sementsov-Ogievskiy 
>>>
>>> Minimal implementation of structured read: one structured reply chunk,
>>> no segmentation.
>>> Minimal structured error implementation: no text message.
>>> Support DF flag, but just ignore it, as there is no segmentation any
>>> way.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> Signed-off-by: Eric Blake 
>>>
> 
>>> +
>>> +    case NBD_OPT_STRUCTURED_REPLY:
>>> +    if (client->structured_reply) {
>>> +    ret = nbd_negotiate_send_rep_err(
>>> +    client->ioc, NBD_REP_ERR_INVALID, option, errp,
>>> +    "structured reply already negotiated");
>>
>> You were going to send a patch to spec for this..
> 
> Still am; it's on my queue for tasks to do today. :)

https://lists.debian.org/nbd/2017/nbd-201710/msg00013.html

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] German BSI analysed security of KVM / QEMU

[Kevin Wolf]

Am 16.10.2017 um 15:38 hat Daniel P. Berrange geschrieben:
> > Another thing that made me a bit sad is that they mention qed as a
> > better performing alternative for qcow2. Even in 2017, people keep
> > spreading this nonsense. :-(
> 
> Probably because when you google for QED you inevitably hit this article
> near the top:
> 
>   https://www.phoronix.com/scan.php?page=news_item=OTY0MQ
> 
> And there's little clear information on QEMU website to show that QED
> is essentially an obsolete experiment.
> 
> Perhaps some clear update to this page would help, and also in the
> qemu docs
> 
>   https://wiki.qemu.org/Features/QED

I updated the wiki page, maybe this will help a bit.

We could also consider printing a warning when you create a new image in
an obsolete native qemu format (i.e. qcow1 and qed).

Kevin

Re: [Qemu-block] [Qemu-devel] German BSI analysed security of KVM / QEMU

[Alberto Garcia]

On Mon 16 Oct 2017 03:38:12 PM CEST, Daniel P. Berrange wrote:
>> Another thing that made me a bit sad is that they mention qed as a
>> better performing alternative for qcow2. Even in 2017, people keep
>> spreading this nonsense. :-(
>
> Probably because when you google for QED you inevitably hit this article
> near the top:
>
>   https://www.phoronix.com/scan.php?page=news_item=OTY0MQ
>
> And there's little clear information on QEMU website to show that QED
> is essentially an obsolete experiment.

I remember having to ask on IRC in order to learn what the status of QED
was, I couldn't figure it out just by reading the documentation or even
the mailing list archives.

Berto

Re: [Qemu-block] [Qemu-devel] German BSI analysed security of KVM / QEMU

[Daniel P. Berrange]

On Mon, Oct 16, 2017 at 03:22:46PM +0200, Kevin Wolf wrote:
> Am 13.10.2017 um 11:44 hat Daniel P. Berrange geschrieben:
> > On Fri, Oct 13, 2017 at 11:37:08AM +0200, Cornelia Huck wrote:
> > > - Lack of support for encryption/signing of network-based images was
> > >   criticized. They ended up using Ceph and GlusterFS, which they were
> > >   reasonably happy with.
> > 
> > Hopefully the 'luks' driver (which can be layered over any block backend
> > including network ones), and the TLS support for NBD would be considered
> > to address this last point to some degree. At least from the encryption
> > side.
> 
> They refer to a blog post of yours about the weakness of the AES
> encryption in qcow2, but it seems they didn't see that luks exists as a
> replacement. While the qcow2 integration of luks is new in 2.10, we've
> had the separate 'luks' driver for a while. Do we need to inform our
> users better about it?

There is definitely room to improve our docs and general presentation
of QEMU security features.

> The option of using LUKS on the host is mentioned, but also that libvirt
> doesn't support this, so it comes with manual work.

Yep

> They saw the TLS support in NBD, but had the impression it's not mature:
> 
> nbd sieht zwar einen Schutz durch TLS vor, dieser wird jedoch als
> experimentell bezeichnet und nicht für den produktiven Betrieb
> empfohlen.
> 
> ("nbd provides protection by TLS; however, this is described as
> experimental and not recommended for production")

They probably read the old version of the NBD protocol specification
from early 2016 where TLS was still listed as "Experimental", due to
not having any implementation of the TLS spec. This "Experimental"
tag was removed after we released TLS in QEMU


> > Signing of disk images is impractical as it would imply having to download
> > the entire image contents to validate signature, rather defeating the point
> > of having a network based image. But perhaps this is lost in translation
> > and they mean something else by "signing of images" ?
> 
> I suppose they mean something on a per-block basis, like a checksum or
> hash that is included in the encryption and can be used to verify that
> nobody changed the encrypted data from outside.
> 
> Darüber hinaus besitzt qcow2 keinerlei Integritätsschutz. Selbst wenn
> die Verschlüsselung als ausreichend sicher angesehen werden würde, so
> ist es dem Angreifer prinzipiell weiterhin möglich, den Inhalt des
> Blockgeräts zu modifizieren
> 
> ("In addition, qcow2 has no integrity protection. Even if the
> encryption were considered sufficiently secure, in priciple the attacker
> could still modify the content of the block device.")
> 
> They mention dm-integrity as an option to implement this on the host
> kernel level.

Ah ok. I would think the user could  still run dm-integrity inside their
guest if they wanted to detect tampering explicitly instead of getting
back garbage decrypted data.

> Another thing that made me a bit sad is that they mention qed as a
> better performing alternative for qcow2. Even in 2017, people keep
> spreading this nonsense. :-(

Probably because when you google for QED you inevitably hit this article
near the top:

  https://www.phoronix.com/scan.php?page=news_item=OTY0MQ

And there's little clear information on QEMU website to show that QED
is essentially an obsolete experiment.

Perhaps some clear update to this page would help, and also in the
qemu docs

  https://wiki.qemu.org/Features/QED


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] German BSI analysed security of KVM / QEMU

[Kevin Wolf]

Am 13.10.2017 um 11:44 hat Daniel P. Berrange geschrieben:
> On Fri, Oct 13, 2017 at 11:37:08AM +0200, Cornelia Huck wrote:
> > - Lack of support for encryption/signing of network-based images was
> >   criticized. They ended up using Ceph and GlusterFS, which they were
> >   reasonably happy with.
> 
> Hopefully the 'luks' driver (which can be layered over any block backend
> including network ones), and the TLS support for NBD would be considered
> to address this last point to some degree. At least from the encryption
> side.

They refer to a blog post of yours about the weakness of the AES
encryption in qcow2, but it seems they didn't see that luks exists as a
replacement. While the qcow2 integration of luks is new in 2.10, we've
had the separate 'luks' driver for a while. Do we need to inform our
users better about it?

The option of using LUKS on the host is mentioned, but also that libvirt
doesn't support this, so it comes with manual work.


They saw the TLS support in NBD, but had the impression it's not mature:

nbd sieht zwar einen Schutz durch TLS vor, dieser wird jedoch als
experimentell bezeichnet und nicht für den produktiven Betrieb
empfohlen.

("nbd provides protection by TLS; however, this is described as
experimental and not recommended for production")

> Signing of disk images is impractical as it would imply having to download
> the entire image contents to validate signature, rather defeating the point
> of having a network based image. But perhaps this is lost in translation
> and they mean something else by "signing of images" ?

I suppose they mean something on a per-block basis, like a checksum or
hash that is included in the encryption and can be used to verify that
nobody changed the encrypted data from outside.

Darüber hinaus besitzt qcow2 keinerlei Integritätsschutz. Selbst wenn
die Verschlüsselung als ausreichend sicher angesehen werden würde, so
ist es dem Angreifer prinzipiell weiterhin möglich, den Inhalt des
Blockgeräts zu modifizieren

("In addition, qcow2 has no integrity protection. Even if the
encryption were considered sufficiently secure, in priciple the attacker
could still modify the content of the block device.")

They mention dm-integrity as an option to implement this on the host
kernel level.


Another thing that made me a bit sad is that they mention qed as a
better performing alternative for qcow2. Even in 2017, people keep
spreading this nonsense. :-(

Kevin

Re: [Qemu-block] [PATCH v1 1/1] iotests: fix the actual-size in 191.out

[Eric Blake]

On 10/16/2017 03:30 AM, Kevin Wolf wrote:
> Am 16.10.2017 um 07:23 hat QingFeng Hao geschrieben:
>> The actual-size set in 191.out is wrong, which causes the test case failed
>> and shall be 331776.
>> The actual-size indicates the image's (e.g. t.qcow2.base) actual size
>> defined by ImageInfo.actual_size.
>>
>> Signed-off-by: QingFeng Hao 
> 
> This is wrong. It may make the test work on your machine, but it breaks
> it on mine.
> 
> The correct approach to fix this is Max' "[PATCH 2/2] iotests: Filter
> actual image size in 184 and 191".

That, and ALL patches should be sent to qemu-de...@nongnu.org, in
addition to whatever other lists are appropriate (you are correct that
this patch should cc qemu-block, but you forgot qemu-devel).

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v4 5/8] nbd/server: Include human-readable message in structured errors

[Eric Blake]

On 10/16/2017 05:59 AM, Vladimir Sementsov-Ogievskiy wrote:
> 15.10.2017 04:01, Eric Blake wrote:
>> The NBD spec permits including a human-readable error string if
>> structured replies are in force, so we might as well send the
>> client the message that we logged on any error.
>>
>> Signed-off-by: Eric Blake 
>> ---
>>   nbd/server.c | 22 --
>>   nbd/trace-events |  2 +-
>>   2 files changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/nbd/server.c b/nbd/server.c
>> index 23dc6be708..8085d79076 100644
>> --- a/nbd/server.c
>> +++ b/nbd/server.c
>> @@ -1289,23 +1289,25 @@ static int coroutine_fn
>> nbd_co_send_structured_read(NBDClient *client,
>>   static int coroutine_fn nbd_co_send_structured_error(NBDClient *client,
>>    uint64_t handle,
>>    uint32_t error,
>> + char *msg,
> 
> it's not const because we want to put it into iov..
> may be it is better to make it const and convert to (char *) like in
> qio_channel_write_all, to
> 1: more native message parameter type
> 2: allow constat strings like nbd_co_send_structured_error(..., "some
> error")

Yes, casting away const would be a more convenient interface (it's a
bummer that iov can't be const-correct for write, because it is also
used by read which cannot be const).

> 
>>    Error **errp)
>>   {
>>   NBDStructuredError chunk;
>>   int nbd_err = system_errno_to_nbd_errno(error);
>>   struct iovec iov[] = {
>>   {.iov_base = , .iov_len = sizeof(chunk)},
>> -    /* FIXME: Support human-readable error message */
>> +    {.iov_base = msg, .iov_len = strlen(msg)},
>>   };
> 
> msg is always non-zero? so we don't want to send errors without
> message.. (1)

I played with the idea of allowing NULL, and don't know whether it was
easier to require non-NULL message (which may require NULL check at all
callers) or to allow NULL (requiring more code here).

> 
>>
>>   assert(nbd_err);
>> -    trace_nbd_co_send_structured_error(handle, nbd_err);
>> +    trace_nbd_co_send_structured_error(handle, nbd_err,
>> +   nbd_err_lookup(nbd_err), msg);
> 
> it doesn't really matter, but "nbd_err_lookup(nbd_err)" is a bit
> unrelated and looks like part of previous patch

The previous patch was yours; I tried to minimize the changes I made to
your patches, so I stuck this one in mine instead. But if you think we
should trace accurately from the get-go, I'm just fine rebasing the
series to make your patch trace the error name at its introduction,
rather than in my followup that supports error messages.

> 
>>   set_be_chunk(, NBD_REPLY_FLAG_DONE,
>> NBD_REPLY_TYPE_ERROR, handle,
>>    sizeof(chunk) - sizeof(chunk.h));
>>   stl_be_p(, nbd_err);
>> -    stw_be_p(_length, 0);
>> +    stw_be_p(_length, iov[1].iov_len);
>>
>> -    return nbd_co_send_iov(client, iov, 1, errp);
>> +    return nbd_co_send_iov(client, iov, 1 + !!iov[1].iov_len, errp);
> 
> but you allow messages of zero length..
> it's ok, but looks a bit strange in connection with (1)

Passing "" is the easiest thing to do for callers that can't pass NULL.
My first attempt had:

.iov_len = msg ? strlen(msg) : 0

and

trace_...(msg ? msg : "")

For this patch, there is only a single caller, so it's really hard to
judge which style (required non-NULL parameter, vs. doing more work in
the common function) will be more useful until we add further callers.


>> -    if (client->structured_reply && request.type == NBD_CMD_READ) {
>> +    if (client->structured_reply &&
>> +    (ret < 0 || request.type == NBD_CMD_READ)) {
>>   if (ret < 0) {
>> +    if (!msg) {
>> +    msg = g_strdup("");
> 
> I'd prefer to handle msg=NULL in nbd_co_send_structured_error instead of
> this.

Okay, then it sounds like accepting a NULL parameter is the way to go!

It's also worth considering that casting away const would mean I don't
have to g_strdup("").


> anyway it should work, so, with or without my suggestions:
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 

Thanks for your reviews!

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v4 3/8] nbd: Expose constants and structs for structured read

[Eric Blake]

On 10/16/2017 03:49 AM, Vladimir Sementsov-Ogievskiy wrote:
> 15.10.2017 04:01, Eric Blake wrote:
>> Upcoming patches will implement the NBD structured reply
>> extension [1] for both client and server roles.  Declare the
>> constants, structs, and lookup routines that will be valuable
>> whether the server or client code is backported in isolation.
>>
>> This includes moving one constant from an internal header to
>> the public header, as part of the structured read processing
>> will be done in block/nbd-client.c rather than nbd/client.c.
>>
>> [1]https://github.com/NetworkBlockDevice/nbd/blob/extension-structured-reply/doc/proto.md
>>
> 
> this link may become dead in future, after merging extension spec into
> master...

Perhaps; but it's a git branch, so it's not going to disappear, even
when it is no longer actively maintained.  For comparison, here's the
(now-stale) link for NBD_CMD_WRITE_ZEROES:

https://github.com/NetworkBlockDevice/nbd/blob/extension-write-zeroes/doc/proto.md

So I'm not too worried about having the URL in the commit message.

> 
>>
>> Based on patches from Vladimir Sementsov-Ogievskiy.
>>
>> Signed-off-by: Eric Blake 
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> 
> 
> 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v4 2/8] nbd: Move nbd_errno_to_system_errno() to public header

[Eric Blake]

On 10/16/2017 03:33 AM, Vladimir Sementsov-Ogievskiy wrote:
> 15.10.2017 04:01, Eric Blake wrote:
>> This is needed in preparation for structured reply handling,
>> as we will be performing the translation from NBD error to
>> system errno value higher in the stack at block/nbd-client.c.
> 
> you've forget to sign-off.

D'oh. Bad patch split on my part.

Signed-off-by: Eric Blake 

> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> 
>> ---
>>   include/block/nbd.h | 13 +
>>   nbd/nbd-internal.h  | 12 
>>   nbd/client.c    | 32 
>>   nbd/common.c    | 34 ++
>>   nbd/trace-events    |  4 +++-
>>   5 files changed, 50 insertions(+), 45 deletions(-)
>>

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v4 8/8] nbd: Move nbd_read() to common header

[Vladimir Sementsov-Ogievskiy]

15.10.2017 04:01, Eric Blake wrote:

An upcoming change to block/nbd-client.c will want to read the
tail of a structured reply chunk directly from the wire.  Move
this function to make it easier.

Based on a patch from Vladimir Sementsov-Ogievskiy.

Signed-off-by: Eric Blake 


Reviewed-by: Vladimir Sementsov-Ogievskiy 



--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v4 7/8] nbd/client: prepare nbd_receive_reply for structured reply

[Vladimir Sementsov-Ogievskiy]

15.10.2017 04:01, Eric Blake wrote:

From: Vladimir Sementsov-Ogievskiy 

In following patch nbd_receive_reply will be used both for simple
and structured reply header receiving.
NBDReply is altered into union of simple reply header and structured
reply chunk header, simple error translation moved to block/nbd-client
to be consistent with further structured reply error translation.

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


looks ok.



--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v4 6/8] nbd/client: refactor nbd_receive_starttls

[Vladimir Sementsov-Ogievskiy]

15.10.2017 04:01, Eric Blake wrote:

From: Vladimir Sementsov-Ogievskiy 

Split out nbd_request_simple_option to be reused for structured reply
option.

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



ok for me.


--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v4 5/8] nbd/server: Include human-readable message in structured errors

[Vladimir Sementsov-Ogievskiy]

15.10.2017 04:01, Eric Blake wrote:

The NBD spec permits including a human-readable error string if
structured replies are in force, so we might as well send the
client the message that we logged on any error.

Signed-off-by: Eric Blake 
---
  nbd/server.c | 22 --
  nbd/trace-events |  2 +-
  2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 23dc6be708..8085d79076 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1289,23 +1289,25 @@ static int coroutine_fn 
nbd_co_send_structured_read(NBDClient *client,
  static int coroutine_fn nbd_co_send_structured_error(NBDClient *client,
   uint64_t handle,
   uint32_t error,
+ char *msg,


it's not const because we want to put it into iov..
may be it is better to make it const and convert to (char *) like in 
qio_channel_write_all, to

1: more native message parameter type
2: allow constat strings like nbd_co_send_structured_error(..., "some 
error")



   Error **errp)
  {
  NBDStructuredError chunk;
  int nbd_err = system_errno_to_nbd_errno(error);
  struct iovec iov[] = {
  {.iov_base = , .iov_len = sizeof(chunk)},
-/* FIXME: Support human-readable error message */
+{.iov_base = msg, .iov_len = strlen(msg)},
  };


msg is always non-zero? so we don't want to send errors without 
message.. (1)




  assert(nbd_err);
-trace_nbd_co_send_structured_error(handle, nbd_err);
+trace_nbd_co_send_structured_error(handle, nbd_err,
+   nbd_err_lookup(nbd_err), msg);


it doesn't really matter, but "nbd_err_lookup(nbd_err)" is a bit 
unrelated and looks like part of previous patch



  set_be_chunk(, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_ERROR, handle,
   sizeof(chunk) - sizeof(chunk.h));
  stl_be_p(, nbd_err);
-stw_be_p(_length, 0);
+stw_be_p(_length, iov[1].iov_len);

-return nbd_co_send_iov(client, iov, 1, errp);
+return nbd_co_send_iov(client, iov, 1 + !!iov[1].iov_len, errp);


but you allow messages of zero length..
it's ok, but looks a bit strange in connection with (1)


  }

  /* nbd_co_receive_request
@@ -1406,6 +1408,7 @@ static coroutine_fn void nbd_trip(void *opaque)
  int flags;
  int reply_data_len = 0;
  Error *local_err = NULL;
+char *msg = NULL;

  trace_nbd_trip();
  if (client->closing) {
@@ -1521,14 +1524,20 @@ reply:
  if (local_err) {
  /* If we get here, local_err was not a fatal error, and should be sent
   * to the client. */
+assert(ret < 0);
+msg = g_strdup(error_get_pretty(local_err));
  error_report_err(local_err);
  local_err = NULL;
  }

-if (client->structured_reply && request.type == NBD_CMD_READ) {
+if (client->structured_reply &&
+(ret < 0 || request.type == NBD_CMD_READ)) {
  if (ret < 0) {
+if (!msg) {
+msg = g_strdup("");


I'd prefer to handle msg=NULL in nbd_co_send_structured_error instead of 
this.



+}
  ret = nbd_co_send_structured_error(req->client, request.handle,
-   -ret, _err);
+   -ret, msg, _err);
  } else {
  ret = nbd_co_send_structured_read(req->client, request.handle,
request.from, req->data,
@@ -1539,6 +1548,7 @@ reply:
 ret < 0 ? -ret : 0,
 req->data, reply_data_len, _err);
  }
+g_free(msg);
  if (ret < 0) {
  error_prepend(_err, "Failed to send reply: ");
  goto disconnect;
diff --git a/nbd/trace-events b/nbd/trace-events
index 15a9294445..880f211c07 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -57,7 +57,7 @@ nbd_blk_aio_attached(const char *name, void *ctx) "Export %s: 
Attaching clients
  nbd_blk_aio_detach(const char *name, void *ctx) "Export %s: Detaching clients from 
AIO context %p\n"
  nbd_co_send_simple_reply(uint64_t handle, uint32_t error, const char *errname, int len) "Send simple 
reply: handle = %" PRIu64 ", error = %" PRIu32 " (%s), len = %d"
  nbd_co_send_structured_read(uint64_t handle, uint64_t offset, void *data, size_t size) "Send 
structured read data reply: handle = %" PRIu64 ", offset = %" PRIu64 ", data = %p, len = 
%zu"
-nbd_co_send_structured_error(uint64_t handle, int err) "Send structured error reply: handle = 
%" PRIu64 ", error = %d"
+nbd_co_send_structured_error(uint64_t handle, int err, const char *errname, const char *msg) 
"Send structured error reply: handle = %" PRIu64 ", error = %d (%s), msg = '%s'"
  

Re: [Qemu-block] [PATCH v4 4/8] nbd: Minimal structured read for server

[Vladimir Sementsov-Ogievskiy]

15.10.2017 04:01, Eric Blake wrote:

From: Vladimir Sementsov-Ogievskiy 

Minimal implementation of structured read: one structured reply chunk,
no segmentation.
Minimal structured error implementation: no text message.
Support DF flag, but just ignore it, as there is no segmentation any
way.

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

---
v4: better _DF flag handling, convert errno to wire format, add
comments and tracing, rework structured error for less churn when adding
text message later, don't kill connection on redundant client option
---
  nbd/server.c | 106 +--
  nbd/trace-events |   2 ++
  2 files changed, 98 insertions(+), 10 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index efb6003364..23dc6be708 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -100,6 +100,8 @@ struct NBDClient {
  QTAILQ_ENTRY(NBDClient) next;
  int nb_requests;
  bool closing;
+
+bool structured_reply;
  };

  /* That's all folks */
@@ -762,6 +764,23 @@ static int nbd_negotiate_options(NBDClient *client, 
uint16_t myflags,
  return ret;
  }
  break;
+
+case NBD_OPT_STRUCTURED_REPLY:
+if (client->structured_reply) {
+ret = nbd_negotiate_send_rep_err(
+client->ioc, NBD_REP_ERR_INVALID, option, errp,
+"structured reply already negotiated");


You were going to send a patch to spec for this..


+} else {
+ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
+ option, errp);
+}
+if (ret < 0) {
+return ret;
+}
+client->structured_reply = true;
+myflags |= NBD_CMD_FLAG_DF;


it should be NBD_FLAG_SEND_DF

[...]

the following looks ok.

--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v4 3/8] nbd: Expose constants and structs for structured read

[Vladimir Sementsov-Ogievskiy]

15.10.2017 04:01, Eric Blake wrote:

Upcoming patches will implement the NBD structured reply
extension [1] for both client and server roles.  Declare the
constants, structs, and lookup routines that will be valuable
whether the server or client code is backported in isolation.

This includes moving one constant from an internal header to
the public header, as part of the structured read processing
will be done in block/nbd-client.c rather than nbd/client.c.

[1]https://github.com/NetworkBlockDevice/nbd/blob/extension-structured-reply/doc/proto.md


this link may become dead in future, after merging extension spec into 
master...




Based on patches from Vladimir Sementsov-Ogievskiy.

Signed-off-by: Eric Blake 


Reviewed-by: Vladimir Sementsov-Ogievskiy 



--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v4 2/8] nbd: Move nbd_errno_to_system_errno() to public header

[Vladimir Sementsov-Ogievskiy]

15.10.2017 04:01, Eric Blake wrote:

This is needed in preparation for structured reply handling,
as we will be performing the translation from NBD error to
system errno value higher in the stack at block/nbd-client.c.


you've forget to sign-off.

Reviewed-by: Vladimir Sementsov-Ogievskiy 


---
  include/block/nbd.h | 13 +
  nbd/nbd-internal.h  | 12 
  nbd/client.c| 32 
  nbd/common.c| 34 ++
  nbd/trace-events|  4 +++-
  5 files changed, 50 insertions(+), 45 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index a6df5ce8b5..dc62b5cd19 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -149,6 +149,18 @@ enum {
   * aren't overflowing some other buffer. */
  #define NBD_MAX_NAME_SIZE 256

+/* NBD errors are based on errno numbers, so there is a 1:1 mapping,
+ * but only a limited set of errno values is specified in the protocol.
+ * Everything else is squashed to EINVAL.
+ */
+#define NBD_SUCCESS0
+#define NBD_EPERM  1
+#define NBD_EIO5
+#define NBD_ENOMEM 12
+#define NBD_EINVAL 22
+#define NBD_ENOSPC 28
+#define NBD_ESHUTDOWN  108
+
  /* Details collected by NBD_OPT_EXPORT_NAME and NBD_OPT_GO */
  struct NBDExportInfo {
  /* Set by client before nbd_receive_negotiate() */
@@ -172,6 +184,7 @@ int nbd_send_request(QIOChannel *ioc, NBDRequest *request);
  int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp);
  int nbd_client(int fd);
  int nbd_disconnect(int fd);
+int nbd_errno_to_system_errno(int err);

  typedef struct NBDExport NBDExport;
  typedef struct NBDClient NBDClient;
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index 4bfe5be884..df6c8b2f24 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -64,18 +64,6 @@
  #define NBD_SET_TIMEOUT _IO(0xab, 9)
  #define NBD_SET_FLAGS   _IO(0xab, 10)

-/* NBD errors are based on errno numbers, so there is a 1:1 mapping,
- * but only a limited set of errno values is specified in the protocol.
- * Everything else is squashed to EINVAL.
- */
-#define NBD_SUCCESS0
-#define NBD_EPERM  1
-#define NBD_EIO5
-#define NBD_ENOMEM 12
-#define NBD_EINVAL 22
-#define NBD_ENOSPC 28
-#define NBD_ESHUTDOWN  108
-
  /* nbd_read_eof
   * Tries to read @size bytes from @ioc.
   * Returns 1 on success
diff --git a/nbd/client.c b/nbd/client.c
index 59d7c9d49f..50f36b511e 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -22,38 +22,6 @@
  #include "trace.h"
  #include "nbd-internal.h"

-static int nbd_errno_to_system_errno(int err)
-{
-int ret;
-switch (err) {
-case NBD_SUCCESS:
-ret = 0;
-break;
-case NBD_EPERM:
-ret = EPERM;
-break;
-case NBD_EIO:
-ret = EIO;
-break;
-case NBD_ENOMEM:
-ret = ENOMEM;
-break;
-case NBD_ENOSPC:
-ret = ENOSPC;
-break;
-case NBD_ESHUTDOWN:
-ret = ESHUTDOWN;
-break;
-default:
-trace_nbd_unknown_error(err);
-/* fallthrough */
-case NBD_EINVAL:
-ret = EINVAL;
-break;
-}
-return ret;
-}
-
  /* Definitions for opaque data types */

  static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
diff --git a/nbd/common.c b/nbd/common.c
index 7456021f7e..593904f148 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -18,6 +18,7 @@

  #include "qemu/osdep.h"
  #include "qapi/error.h"
+#include "trace.h"
  #include "nbd-internal.h"

  /* Discard length bytes from channel.  Return -errno on failure and 0 on
@@ -171,3 +172,36 @@ const char *nbd_err_lookup(int err)
  return "";
  }
  }
+
+
+int nbd_errno_to_system_errno(int err)
+{
+int ret;
+switch (err) {
+case NBD_SUCCESS:
+ret = 0;
+break;
+case NBD_EPERM:
+ret = EPERM;
+break;
+case NBD_EIO:
+ret = EIO;
+break;
+case NBD_ENOMEM:
+ret = ENOMEM;
+break;
+case NBD_ENOSPC:
+ret = ENOSPC;
+break;
+case NBD_ESHUTDOWN:
+ret = ESHUTDOWN;
+break;
+default:
+trace_nbd_unknown_error(err);
+/* fallthrough */
+case NBD_EINVAL:
+ret = EINVAL;
+break;
+}
+return ret;
+}
diff --git a/nbd/trace-events b/nbd/trace-events
index 9a72f458b2..d3b702dd9a 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -1,5 +1,4 @@
  # nbd/client.c
-nbd_unknown_error(int err) "Squashing unexpected error %d to EINVAL"
  nbd_send_option_request(uint32_t opt, const char *name, uint32_t len) "Sending option 
request %" PRIu32" (%s), len %" PRIu32
  nbd_receive_option_reply(uint32_t option, const char *optname, uint32_t type, const char *typename, 
uint32_t length) "Received option reply 0x%" PRIx32" (%s), type 0x%" PRIx32" (%s), 
len %" PRIu32
  nbd_reply_err_unsup(uint32_t option, const char *name) 

Re: [Qemu-block] [PATCH v1 1/1] iotests: fix the actual-size in 191.out

[Kevin Wolf]

Am 16.10.2017 um 07:23 hat QingFeng Hao geschrieben:
> The actual-size set in 191.out is wrong, which causes the test case failed
> and shall be 331776.
> The actual-size indicates the image's (e.g. t.qcow2.base) actual size
> defined by ImageInfo.actual_size.
> 
> Signed-off-by: QingFeng Hao 

This is wrong. It may make the test work on your machine, but it breaks
it on mine.

The correct approach to fix this is Max' "[PATCH 2/2] iotests: Filter
actual image size in 184 and 191".

Kevin

Re: [Qemu-block] [PATCH v4 1/8] nbd: Include error names in trace messages

[Vladimir Sementsov-Ogievskiy]

15.10.2017 04:01, Eric Blake wrote:

NBD errors were originally sent over the wire based on Linux errno
values; but not all the world is Linux, and not all platforms share
the same values.  Since a number isn't very easy to decipher on all
platforms, update the trace messages to include the name of NBD
errors being sent/received over the wire.  Tweak the trace messages
to be at the point where we are using the NBD error, not the
translation to the host errno values.

Signed-off-by: Eric Blake 


Reviewed-by: Vladimir Sementsov-Ogievskiy 


--
Best regards,
Vladimir