* [PATCH 0/2] iotests: Make _filter_img_create more active
@ 2020-06-16 13:17 Max Reitz
2020-06-16 13:17 ` [PATCH 1/2] " Max Reitz
2020-06-16 13:17 ` [PATCH 2/2] iotests: filter few more luks specific create options Max Reitz
0 siblings, 2 replies; 8+ messages in thread
From: Max Reitz @ 2020-06-16 13:17 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Maxim Levitsky, qemu-devel, Max Reitz
Hi,
Applying Maxim’s series for LUKS encryption slot management through
qemu-img amend / blockdev-amend has brought a – on the first glance –
rather minor problem: It changes the order of qcow2’s creation options,
which results in some reference output changes (patch 5:
https://lists.nongnu.org/archive/html/qemu-block/2020-06/msg00335.html
). This affects some tests that can also run with other image formats,
such as qcow, whose order does not change. So this patch breaks those
tests (I’ve seen it for 134 and 158) when run with a different format
than qcow2 (e.g. qcow).
Now we could just create a difference reference output for qcow2, as is
done e.g. for test 150. But that would not only be boring, but also not
really sustainable: The actual problem is that the order of creation
options simply does not have to be the same between different image
formats, and so we should not just dump qemu-img create’s output to a
reference output, drop some format-specific options and expect it to
work independent of the format for which the test is run.
So patch 1 in this series makes _filter_img_create sort the creation
options as they appear in the “Formatting” line, so it’s always the same
order between formats. (And I took this opportunity to also reverse the
filtering implementation from denylisting to allowlisting.)
Patch 2 is taken from Maxim’s series and modified to fit the new
implementation.
I propose putting this series underneath Maxim’s series (in my block
branch) so the latter won’t break 134 and 158 for qcow. (Doing so will
require dropping some hunks from the patch linked above, but that should
be fine.)
Max Reitz (1):
iotests: Make _filter_img_create more active
Maxim Levitsky (1):
iotests: filter few more luks specific create options
tests/qemu-iotests/087.out | 6 +-
tests/qemu-iotests/112.out | 2 +-
tests/qemu-iotests/134.out | 2 +-
tests/qemu-iotests/153 | 9 ++-
tests/qemu-iotests/158.out | 4 +-
tests/qemu-iotests/188.out | 2 +-
tests/qemu-iotests/189.out | 4 +-
tests/qemu-iotests/198.out | 4 +-
tests/qemu-iotests/263.out | 4 +-
tests/qemu-iotests/284.out | 6 +-
tests/qemu-iotests/common.filter | 97 ++++++++++++++++++++++++--------
11 files changed, 94 insertions(+), 46 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] iotests: Make _filter_img_create more active
2020-06-16 13:17 [PATCH 0/2] iotests: Make _filter_img_create more active Max Reitz
@ 2020-06-16 13:17 ` Max Reitz
2020-06-17 11:46 ` Maxim Levitsky
2020-06-16 13:17 ` [PATCH 2/2] iotests: filter few more luks specific create options Max Reitz
1 sibling, 1 reply; 8+ messages in thread
From: Max Reitz @ 2020-06-16 13:17 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Maxim Levitsky, qemu-devel, Max Reitz
Right now, _filter_img_create just filters out everything that looks
format-dependent, and applies some filename filters. That means that we
have to add another filter line every time some format gets a new
creation option. This can be avoided by instead discarding everything
and just keeping what we know is format-independent (format, size,
backing file, encryption information[1], preallocation) or just
interesting to have in the reference output (external data file path).
Furthermore, we probably want to sort these options. Format drivers are
not required to define them in any specific order, so the output is
effectively random (although this has never bothered us until now). We
need a specific order for our reference outputs, though. Unfortunately,
just using a plain "sort" would change a lot of existing reference
outputs, so we have to pre-filter the option keys to keep our existing
order (fmt, size, backing*, data, encryption info, preallocation).
[1] Actually, the only thing that is really important is whether
encryption is enabled or not. A patch by Maxim thus removes all
other "encrypt.*" options from the output:
https://lists.nongnu.org/archive/html/qemu-block/2020-06/msg00339.html
But that patch needs to come later so we can get away with changing
as few reference outputs in this patch here as possible.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/112.out | 2 +-
tests/qemu-iotests/153 | 9 ++-
tests/qemu-iotests/common.filter | 100 +++++++++++++++++++++++--------
3 files changed, 81 insertions(+), 30 deletions(-)
diff --git a/tests/qemu-iotests/112.out b/tests/qemu-iotests/112.out
index ae0318cabe..182655dbf6 100644
--- a/tests/qemu-iotests/112.out
+++ b/tests/qemu-iotests/112.out
@@ -5,7 +5,7 @@ QA output created by 112
qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and may not exceed 64 bits
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and may not exceed 64 bits
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 refcount_bits=-1
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and may not exceed 64 bits
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and may not exceed 64 bits
diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153
index cf961d3609..11e3d28841 100755
--- a/tests/qemu-iotests/153
+++ b/tests/qemu-iotests/153
@@ -167,11 +167,10 @@ done
echo
echo "== Creating ${TEST_IMG}.[abc] ==" | _filter_testdir
-(
- $QEMU_IMG create -f qcow2 "${TEST_IMG}.a" -b "${TEST_IMG}"
- $QEMU_IMG create -f qcow2 "${TEST_IMG}.b" -b "${TEST_IMG}"
- $QEMU_IMG create -f qcow2 "${TEST_IMG}.c" -b "${TEST_IMG}.b"
-) | _filter_img_create
+$QEMU_IMG create -f qcow2 "${TEST_IMG}.a" -b "${TEST_IMG}" | _filter_img_create
+$QEMU_IMG create -f qcow2 "${TEST_IMG}.b" -b "${TEST_IMG}" | _filter_img_create
+$QEMU_IMG create -f qcow2 "${TEST_IMG}.c" -b "${TEST_IMG}.b" \
+ | _filter_img_create
echo
echo "== Two devices sharing the same file in backing chain =="
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 03e4f71808..f104ad7a9b 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -122,38 +122,90 @@ _filter_actual_image_size()
# replace driver-specific options in the "Formatting..." line
_filter_img_create()
{
- data_file_filter=()
- if data_file=$(_get_data_file "$TEST_IMG"); then
- data_file_filter=(-e "s# data_file=$data_file##")
+ # Keep QMP output unchanged
+ qmp_pre=''
+ qmp_post=''
+ to_filter=''
+
+ while read -r line; do
+ if echo "$line" | grep -q '^{.*}$'; then
+ if [ -z "$to_filter" ]; then
+ # Use $'\n' so the newline is not dropped on variable
+ # expansion
+ qmp_pre="$qmp_pre$line"$'\n'
+ else
+ qmp_post="$qmp_post$line"$'\n'
+ fi
+ else
+ to_filter="$to_filter$line"$'\n'
+ fi
+ done
+
+ readarray -td '' formatting_line < \
+ <(echo "$to_filter" | sed -e 's/, fmt=/\x0/')
+
+ filename_part=${formatting_line[0]}
+ if [ -n "${formatting_line[1]}" ]; then
+ options="fmt=${formatting_line[1]}"
+ else
+ options=''
fi
- $SED "${data_file_filter[@]}" \
+ # Set grep_data_file to '\|data_file' to keep it; make it empty
+ # to drop it.
+ # We want to drop it if it is part of the global $IMGOPTS, and we
+ # want to keep it otherwise (if the test specifically wants to
+ # test data files).
+ grep_data_file='\|data_file'
+ if _get_data_file "$TEST_IMG" > /dev/null; then
+ grep_data_file=''
+ fi
+
+ filename_filters=(
-e "s#$REMOTE_TEST_DIR#TEST_DIR#g" \
-e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
-e "s#$TEST_DIR#TEST_DIR#g" \
-e "s#$SOCK_DIR#SOCK_DIR#g" \
-e "s#$IMGFMT#IMGFMT#g" \
-e 's#nbd:127.0.0.1:[0-9]\\+#TEST_DIR/t.IMGFMT#g' \
- -e 's#nbd+unix:///\??socket=SOCK_DIR/nbd#TEST_DIR/t.IMGFMT#g' \
- -e "s# encryption=off##g" \
- -e "s# cluster_size=[0-9]\\+##g" \
- -e "s# table_size=[0-9]\\+##g" \
- -e "s# compat=[^ ]*##g" \
- -e "s# compat6=\\(on\\|off\\)##g" \
- -e "s# static=\\(on\\|off\\)##g" \
- -e "s# zeroed_grain=\\(on\\|off\\)##g" \
- -e "s# subformat=[^ ]*##g" \
- -e "s# adapter_type=[^ ]*##g" \
- -e "s# hwversion=[^ ]*##g" \
- -e "s# lazy_refcounts=\\(on\\|off\\)##g" \
- -e "s# block_size=[0-9]\\+##g" \
- -e "s# block_state_zero=\\(on\\|off\\)##g" \
- -e "s# log_size=[0-9]\\+##g" \
- -e "s# refcount_bits=[0-9]\\+##g" \
- -e "s# key-secret=[a-zA-Z0-9]\\+##g" \
- -e "s# iter-time=[0-9]\\+##g" \
- -e "s# force_size=\\(on\\|off\\)##g" \
- -e "s# compression_type=[a-zA-Z0-9]\\+##g"
+ -e 's#nbd+unix:///\??socket=SOCK_DIR/nbd#TEST_DIR/t.IMGFMT#g'
+ )
+
+ filename_part=$(echo "$filename_part" | $SED "${filename_filters[@]}")
+
+ # Break the option line before each option (preserving pre-existing
+ # line breaks by replacing them by \0 and restoring them at the end),
+ # then filter out the options we want to keep and sort them according
+ # to some order that all block drivers used at the time of writing
+ # this function.
+ options=$(
+ echo "$options" \
+ | tr '\n' '\0' \
+ | sed -e 's/\x0$//' -e 's/ \([a-z0-9_.-]*\)=/\n\1=/g' \
+ | grep -ae "^\(fmt\\|size\\|backing\\|preallocation\\|encrypt$grep_data_file\\)" \
+ | $SED "${filename_filters[@]}" \
+ -e 's/^\(fmt\)/0-\1/' \
+ -e 's/^\(size\)/1-\1/' \
+ -e 's/^\(backing\)/2-\1/' \
+ -e 's/^\(data_file\)/3-\1/' \
+ -e 's/^\(encryption\)/4-\1/' \
+ -e 's/^\(encrypt\.format\)/5-\1/' \
+ -e 's/^\(encrypt\.key-secret\)/6-\1/' \
+ -e 's/^\(encrypt\.iter-time\)/7-\1/' \
+ -e 's/^\(preallocation\)/8-\1/' \
+ | sort \
+ | $SED -e 's/^[0-9]-//' \
+ | tr '\n\0' ' \n' \
+ | $SED -e 's/^ *$//' -e 's/ *$//'
+ )
+
+ echo -n "$qmp_pre"
+ if [ -n "$options" ]; then
+ echo "$filename_part, $options"
+ elif [ -n "$filename_part" ]; then
+ echo "$filename_part"
+ fi
+ echo -n "$qmp_post"
}
_filter_img_create_size()
--
2.26.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] iotests: filter few more luks specific create options
2020-06-16 13:17 [PATCH 0/2] iotests: Make _filter_img_create more active Max Reitz
2020-06-16 13:17 ` [PATCH 1/2] " Max Reitz
@ 2020-06-16 13:17 ` Max Reitz
2020-06-17 11:47 ` Maxim Levitsky
1 sibling, 1 reply; 8+ messages in thread
From: Max Reitz @ 2020-06-16 13:17 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Maxim Levitsky, qemu-devel, Max Reitz
From: Maxim Levitsky <mlevitsk@redhat.com>
This allows more tests to be able to have same output on both qcow2 luks encrypted images
and raw luks images
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/087.out | 6 +++---
tests/qemu-iotests/134.out | 2 +-
tests/qemu-iotests/158.out | 4 ++--
tests/qemu-iotests/188.out | 2 +-
tests/qemu-iotests/189.out | 4 ++--
tests/qemu-iotests/198.out | 4 ++--
tests/qemu-iotests/263.out | 4 ++--
tests/qemu-iotests/284.out | 6 +++---
tests/qemu-iotests/common.filter | 5 +----
9 files changed, 17 insertions(+), 20 deletions(-)
diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out
index 2d92ea847b..b61ba638af 100644
--- a/tests/qemu-iotests/087.out
+++ b/tests/qemu-iotests/087.out
@@ -34,7 +34,7 @@ QMP_VERSION
=== Encrypted image QCow ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on encrypt.key-secret=sec0
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on
Testing:
QMP_VERSION
{"return": {}}
@@ -46,7 +46,7 @@ QMP_VERSION
=== Encrypted image LUKS ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encrypt.format=luks encrypt.key-secret=sec0
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
Testing:
QMP_VERSION
{"return": {}}
@@ -58,7 +58,7 @@ QMP_VERSION
=== Missing driver ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on encrypt.key-secret=sec0
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on
Testing: -S
QMP_VERSION
{"return": {}}
diff --git a/tests/qemu-iotests/134.out b/tests/qemu-iotests/134.out
index 09d46f6b17..4abc5b5f7d 100644
--- a/tests/qemu-iotests/134.out
+++ b/tests/qemu-iotests/134.out
@@ -1,5 +1,5 @@
QA output created by 134
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on encrypt.key-secret=sec0
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on
== reading whole image ==
read 134217728/134217728 bytes at offset 0
diff --git a/tests/qemu-iotests/158.out b/tests/qemu-iotests/158.out
index 6def216e55..f28a17626b 100644
--- a/tests/qemu-iotests/158.out
+++ b/tests/qemu-iotests/158.out
@@ -1,6 +1,6 @@
QA output created by 158
== create base ==
-Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728 encryption=on encrypt.key-secret=sec0
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728 encryption=on
== writing whole image ==
wrote 134217728/134217728 bytes at offset 0
@@ -10,7 +10,7 @@ wrote 134217728/134217728 bytes at offset 0
read 134217728/134217728 bytes at offset 0
128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
== create overlay ==
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.base encryption=on encrypt.key-secret=sec0
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.base encryption=on
== writing part of a cluster ==
wrote 1024/1024 bytes at offset 0
diff --git a/tests/qemu-iotests/188.out b/tests/qemu-iotests/188.out
index c568ef3701..5426861b18 100644
--- a/tests/qemu-iotests/188.out
+++ b/tests/qemu-iotests/188.out
@@ -1,5 +1,5 @@
QA output created by 188
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=16777216 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=16777216
== reading whole image ==
read 16777216/16777216 bytes at offset 0
diff --git a/tests/qemu-iotests/189.out b/tests/qemu-iotests/189.out
index a0b7c9c24c..bc213cbe14 100644
--- a/tests/qemu-iotests/189.out
+++ b/tests/qemu-iotests/189.out
@@ -1,6 +1,6 @@
QA output created by 189
== create base ==
-Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=16777216 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=16777216
== writing whole image ==
wrote 16777216/16777216 bytes at offset 0
@@ -10,7 +10,7 @@ wrote 16777216/16777216 bytes at offset 0
read 16777216/16777216 bytes at offset 0
16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
== create overlay ==
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=16777216 backing_file=TEST_DIR/t.IMGFMT.base encrypt.format=luks encrypt.key-secret=sec1 encrypt.iter-time=10
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=16777216 backing_file=TEST_DIR/t.IMGFMT.base
== writing part of a cluster ==
wrote 1024/1024 bytes at offset 0
diff --git a/tests/qemu-iotests/198.out b/tests/qemu-iotests/198.out
index 6280ae6eed..4b800e70db 100644
--- a/tests/qemu-iotests/198.out
+++ b/tests/qemu-iotests/198.out
@@ -1,12 +1,12 @@
QA output created by 198
== create base ==
-Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=16777216 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=16777216
== writing whole image base ==
wrote 16777216/16777216 bytes at offset 0
16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
== create overlay ==
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=16777216 backing_file=TEST_DIR/t.IMGFMT.base encrypt.format=luks encrypt.key-secret=sec1 encrypt.iter-time=10
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=16777216 backing_file=TEST_DIR/t.IMGFMT.base
== writing whole image layer ==
wrote 16777216/16777216 bytes at offset 0
diff --git a/tests/qemu-iotests/263.out b/tests/qemu-iotests/263.out
index 0c982c55cb..54bfbeeff8 100644
--- a/tests/qemu-iotests/263.out
+++ b/tests/qemu-iotests/263.out
@@ -2,7 +2,7 @@ QA output created by 263
testing LUKS qcow2 encryption
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
== reading the whole image ==
read 1048576/1048576 bytes at offset 0
1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
@@ -21,7 +21,7 @@ read 982528/982528 bytes at offset 66048
testing legacy AES qcow2 encryption
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=aes encrypt.key-secret=sec0
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
== reading the whole image ==
read 1048576/1048576 bytes at offset 0
1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
diff --git a/tests/qemu-iotests/284.out b/tests/qemu-iotests/284.out
index 48216f5742..a929239302 100644
--- a/tests/qemu-iotests/284.out
+++ b/tests/qemu-iotests/284.out
@@ -2,7 +2,7 @@ QA output created by 284
testing LUKS qcow2 encryption
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
== cluster size 512
== checking image refcounts ==
@@ -21,7 +21,7 @@ wrote 1/1 bytes at offset 512
== rechecking image refcounts ==
No errors were found on the image.
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
== cluster size 2048
== checking image refcounts ==
@@ -40,7 +40,7 @@ wrote 1/1 bytes at offset 2048
== rechecking image refcounts ==
No errors were found on the image.
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
== cluster size 32768
== checking image refcounts ==
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index f104ad7a9b..9ae7ac9891 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -182,16 +182,13 @@ _filter_img_create()
echo "$options" \
| tr '\n' '\0' \
| sed -e 's/\x0$//' -e 's/ \([a-z0-9_.-]*\)=/\n\1=/g' \
- | grep -ae "^\(fmt\\|size\\|backing\\|preallocation\\|encrypt$grep_data_file\\)" \
+ | grep -ae "^\(fmt\\|size\\|backing\\|preallocation\\|encryption$grep_data_file\\)" \
| $SED "${filename_filters[@]}" \
-e 's/^\(fmt\)/0-\1/' \
-e 's/^\(size\)/1-\1/' \
-e 's/^\(backing\)/2-\1/' \
-e 's/^\(data_file\)/3-\1/' \
-e 's/^\(encryption\)/4-\1/' \
- -e 's/^\(encrypt\.format\)/5-\1/' \
- -e 's/^\(encrypt\.key-secret\)/6-\1/' \
- -e 's/^\(encrypt\.iter-time\)/7-\1/' \
-e 's/^\(preallocation\)/8-\1/' \
| sort \
| $SED -e 's/^[0-9]-//' \
--
2.26.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] iotests: Make _filter_img_create more active
2020-06-16 13:17 ` [PATCH 1/2] " Max Reitz
@ 2020-06-17 11:46 ` Maxim Levitsky
2020-06-17 13:50 ` Max Reitz
0 siblings, 1 reply; 8+ messages in thread
From: Maxim Levitsky @ 2020-06-17 11:46 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
On Tue, 2020-06-16 at 15:17 +0200, Max Reitz wrote:
> Right now, _filter_img_create just filters out everything that looks
> format-dependent, and applies some filename filters. That means that we
> have to add another filter line every time some format gets a new
> creation option. This can be avoided by instead discarding everything
> and just keeping what we know is format-independent (format, size,
> backing file, encryption information[1], preallocation) or just
> interesting to have in the reference output (external data file path).
>
> Furthermore, we probably want to sort these options. Format drivers are
> not required to define them in any specific order, so the output is
> effectively random (although this has never bothered us until now). We
> need a specific order for our reference outputs, though. Unfortunately,
> just using a plain "sort" would change a lot of existing reference
> outputs, so we have to pre-filter the option keys to keep our existing
> order (fmt, size, backing*, data, encryption info, preallocation).
>
> [1] Actually, the only thing that is really important is whether
> encryption is enabled or not. A patch by Maxim thus removes all
> other "encrypt.*" options from the output:
> https://lists.nongnu.org/archive/html/qemu-block/2020-06/msg00339.html
> But that patch needs to come later so we can get away with changing
> as few reference outputs in this patch here as possible.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/qemu-iotests/112.out | 2 +-
> tests/qemu-iotests/153 | 9 ++-
> tests/qemu-iotests/common.filter | 100 +++++++++++++++++++++++--------
> 3 files changed, 81 insertions(+), 30 deletions(-)
>
> diff --git a/tests/qemu-iotests/112.out b/tests/qemu-iotests/112.out
> index ae0318cabe..182655dbf6 100644
> --- a/tests/qemu-iotests/112.out
> +++ b/tests/qemu-iotests/112.out
> @@ -5,7 +5,7 @@ QA output created by 112
> qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and may not exceed 64 bits
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and may not exceed 64 bits
> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 refcount_bits=-1
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and may not exceed 64 bits
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and may not exceed 64 bits
> diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153
> index cf961d3609..11e3d28841 100755
> --- a/tests/qemu-iotests/153
> +++ b/tests/qemu-iotests/153
> @@ -167,11 +167,10 @@ done
>
> echo
> echo "== Creating ${TEST_IMG}.[abc] ==" | _filter_testdir
> -(
> - $QEMU_IMG create -f qcow2 "${TEST_IMG}.a" -b "${TEST_IMG}"
> - $QEMU_IMG create -f qcow2 "${TEST_IMG}.b" -b "${TEST_IMG}"
> - $QEMU_IMG create -f qcow2 "${TEST_IMG}.c" -b "${TEST_IMG}.b"
> -) | _filter_img_create
> +$QEMU_IMG create -f qcow2 "${TEST_IMG}.a" -b "${TEST_IMG}" | _filter_img_create
> +$QEMU_IMG create -f qcow2 "${TEST_IMG}.b" -b "${TEST_IMG}" | _filter_img_create
> +$QEMU_IMG create -f qcow2 "${TEST_IMG}.c" -b "${TEST_IMG}.b" \
> + | _filter_img_create
>
> echo
> echo "== Two devices sharing the same file in backing chain =="
I guess this is done because now the filter expectes only a single
qemu-img output. IMHO this is better anyway.
> diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
> index 03e4f71808..f104ad7a9b 100644
> --- a/tests/qemu-iotests/common.filter
> +++ b/tests/qemu-iotests/common.filter
> @@ -122,38 +122,90 @@ _filter_actual_image_size()
> # replace driver-specific options in the "Formatting..." line
> _filter_img_create()
> {
> - data_file_filter=()
> - if data_file=$(_get_data_file "$TEST_IMG"); then
> - data_file_filter=(-e "s# data_file=$data_file##")
> + # Keep QMP output unchanged
> + qmp_pre=''
> + qmp_post=''
> + to_filter=''
> +
> + while read -r line; do
> + if echo "$line" | grep -q '^{.*}$'; then
> + if [ -z "$to_filter" ]; then
> + # Use $'\n' so the newline is not dropped on variable
> + # expansion
> + qmp_pre="$qmp_pre$line"$'\n'
> + else
> + qmp_post="$qmp_post$line"$'\n'
> + fi
> + else
> + to_filter="$to_filter$line"$'\n'
> + fi
> + done
The above code basically assumes that qmp output starts with '{' and ends with '}'
which I guess is fair, and then it assumes that we can have set of qmp commands prior
to qemu-img line and another set of qmp commands after it.
To me it feels like we should have another filter for that, since
qemu-img itself doesn't use qmp.
Which test needs it?
> +
> + readarray -td '' formatting_line < \
> + <(echo "$to_filter" | sed -e 's/, fmt=/\x0/')
OK, took me a while to understand what this does, but looks OK.
> +
> + filename_part=${formatting_line[0]}
> + if [ -n "${formatting_line[1]}" ]; then
> + options="fmt=${formatting_line[1]}"
> + else
> + options=''
> fi
>
OK.
>
> - $SED "${data_file_filter[@]}" \
> + # Set grep_data_file to '\|data_file' to keep it; make it empty
> + # to drop it.
> + # We want to drop it if it is part of the global $IMGOPTS, and we
> + # want to keep it otherwise (if the test specifically wants to
> + # test data files).
> + grep_data_file='\|data_file'
> + if _get_data_file "$TEST_IMG" > /dev/null; then
> + grep_data_file=''
> + fi
> +
> + filename_filters=(
> -e "s#$REMOTE_TEST_DIR#TEST_DIR#g" \
> -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
> -e "s#$TEST_DIR#TEST_DIR#g" \
> -e "s#$SOCK_DIR#SOCK_DIR#g" \
> -e "s#$IMGFMT#IMGFMT#g" \
> -e 's#nbd:127.0.0.1:[0-9]\\+#TEST_DIR/t.IMGFMT#g' \
> - -e 's#nbd+unix:///\??socket=SOCK_DIR/nbd#TEST_DIR/t.IMGFMT#g' \
> - -e "s# encryption=off##g" \
> - -e "s# cluster_size=[0-9]\\+##g" \
> - -e "s# table_size=[0-9]\\+##g" \
> - -e "s# compat=[^ ]*##g" \
> - -e "s# compat6=\\(on\\|off\\)##g" \
> - -e "s# static=\\(on\\|off\\)##g" \
> - -e "s# zeroed_grain=\\(on\\|off\\)##g" \
> - -e "s# subformat=[^ ]*##g" \
> - -e "s# adapter_type=[^ ]*##g" \
> - -e "s# hwversion=[^ ]*##g" \
> - -e "s# lazy_refcounts=\\(on\\|off\\)##g" \
> - -e "s# block_size=[0-9]\\+##g" \
> - -e "s# block_state_zero=\\(on\\|off\\)##g" \
> - -e "s# log_size=[0-9]\\+##g" \
> - -e "s# refcount_bits=[0-9]\\+##g" \
> - -e "s# key-secret=[a-zA-Z0-9]\\+##g" \
> - -e "s# iter-time=[0-9]\\+##g" \
> - -e "s# force_size=\\(on\\|off\\)##g" \
> - -e "s# compression_type=[a-zA-Z0-9]\\+##g"
> + -e 's#nbd+unix:///\??socket=SOCK_DIR/nbd#TEST_DIR/t.IMGFMT#g'
> + )
> +
> + filename_part=$(echo "$filename_part" | $SED "${filename_filters[@]}")
Looks good as well.
> +
> + # Break the option line before each option (preserving pre-existing
> + # line breaks by replacing them by \0 and restoring them at the end),
> + # then filter out the options we want to keep and sort them according
> + # to some order that all block drivers used at the time of writing
> + # this function.
> + options=$(
> + echo "$options" \
> + | tr '\n' '\0' \
> + | sed -e 's/\x0$//' -e 's/ \([a-z0-9_.-]*\)=/\n\1=/g' \
You sometimes use $SED and sometimes sed. Is this intentional?
> + | grep -ae "^\(fmt\\|size\\|backing\\|preallocation\\|encrypt$grep_data_file\\)" \
> + | $SED "${filename_filters[@]}" \
> + -e 's/^\(fmt\)/0-\1/' \
> + -e 's/^\(size\)/1-\1/' \
> + -e 's/^\(backing\)/2-\1/' \
> + -e 's/^\(data_file\)/3-\1/' \
> + -e 's/^\(encryption\)/4-\1/' \
> + -e 's/^\(encrypt\.format\)/5-\1/' \
> + -e 's/^\(encrypt\.key-secret\)/6-\1/' \
> + -e 's/^\(encrypt\.iter-time\)/7-\1/' \
> + -e 's/^\(preallocation\)/8-\1/' \
All right, I understand this now, but do we have to do this?
Maybe it is better to just update the outputs once to avoid keeping
the custom sort order?
> + | sort \
> + | $SED -e 's/^[0-9]-//' \
> + | tr '\n\0' ' \n' \
> + | $SED -e 's/^ *$//' -e 's/ *$//'
> + )
For the above bash pipeline overall: It was hard to decipher :-), but I must admit
I learned something from it.
> +
> + echo -n "$qmp_pre"
> + if [ -n "$options" ]; then
> + echo "$filename_part, $options"
> + elif [ -n "$filename_part" ]; then
> + echo "$filename_part"
> + fi
> + echo -n "$qmp_post"
> }
>
> _filter_img_create_size()
Overall I like the idea of this patch.
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] iotests: filter few more luks specific create options
2020-06-16 13:17 ` [PATCH 2/2] iotests: filter few more luks specific create options Max Reitz
@ 2020-06-17 11:47 ` Maxim Levitsky
0 siblings, 0 replies; 8+ messages in thread
From: Maxim Levitsky @ 2020-06-17 11:47 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
On Tue, 2020-06-16 at 15:17 +0200, Max Reitz wrote:
> From: Maxim Levitsky <mlevitsk@redhat.com>
>
> This allows more tests to be able to have same output on both qcow2 luks encrypted images
> and raw luks images
>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/qemu-iotests/087.out | 6 +++---
> tests/qemu-iotests/134.out | 2 +-
> tests/qemu-iotests/158.out | 4 ++--
> tests/qemu-iotests/188.out | 2 +-
> tests/qemu-iotests/189.out | 4 ++--
> tests/qemu-iotests/198.out | 4 ++--
> tests/qemu-iotests/263.out | 4 ++--
> tests/qemu-iotests/284.out | 6 +++---
> tests/qemu-iotests/common.filter | 5 +----
> 9 files changed, 17 insertions(+), 20 deletions(-)
>
> diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out
> index 2d92ea847b..b61ba638af 100644
> --- a/tests/qemu-iotests/087.out
> +++ b/tests/qemu-iotests/087.out
> @@ -34,7 +34,7 @@ QMP_VERSION
>
> === Encrypted image QCow ===
>
> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on encrypt.key-secret=sec0
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on
> Testing:
> QMP_VERSION
> {"return": {}}
> @@ -46,7 +46,7 @@ QMP_VERSION
>
> === Encrypted image LUKS ===
>
> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encrypt.format=luks encrypt.key-secret=sec0
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> Testing:
> QMP_VERSION
> {"return": {}}
> @@ -58,7 +58,7 @@ QMP_VERSION
>
> === Missing driver ===
>
> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on encrypt.key-secret=sec0
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on
> Testing: -S
> QMP_VERSION
> {"return": {}}
> diff --git a/tests/qemu-iotests/134.out b/tests/qemu-iotests/134.out
> index 09d46f6b17..4abc5b5f7d 100644
> --- a/tests/qemu-iotests/134.out
> +++ b/tests/qemu-iotests/134.out
> @@ -1,5 +1,5 @@
> QA output created by 134
> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on encrypt.key-secret=sec0
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on
>
> == reading whole image ==
> read 134217728/134217728 bytes at offset 0
> diff --git a/tests/qemu-iotests/158.out b/tests/qemu-iotests/158.out
> index 6def216e55..f28a17626b 100644
> --- a/tests/qemu-iotests/158.out
> +++ b/tests/qemu-iotests/158.out
> @@ -1,6 +1,6 @@
> QA output created by 158
> == create base ==
> -Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728 encryption=on encrypt.key-secret=sec0
> +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728 encryption=on
>
> == writing whole image ==
> wrote 134217728/134217728 bytes at offset 0
> @@ -10,7 +10,7 @@ wrote 134217728/134217728 bytes at offset 0
> read 134217728/134217728 bytes at offset 0
> 128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> == create overlay ==
> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.base encryption=on encrypt.key-secret=sec0
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.base encryption=on
>
> == writing part of a cluster ==
> wrote 1024/1024 bytes at offset 0
> diff --git a/tests/qemu-iotests/188.out b/tests/qemu-iotests/188.out
> index c568ef3701..5426861b18 100644
> --- a/tests/qemu-iotests/188.out
> +++ b/tests/qemu-iotests/188.out
> @@ -1,5 +1,5 @@
> QA output created by 188
> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=16777216 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=16777216
>
> == reading whole image ==
> read 16777216/16777216 bytes at offset 0
> diff --git a/tests/qemu-iotests/189.out b/tests/qemu-iotests/189.out
> index a0b7c9c24c..bc213cbe14 100644
> --- a/tests/qemu-iotests/189.out
> +++ b/tests/qemu-iotests/189.out
> @@ -1,6 +1,6 @@
> QA output created by 189
> == create base ==
> -Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=16777216 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
> +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=16777216
>
> == writing whole image ==
> wrote 16777216/16777216 bytes at offset 0
> @@ -10,7 +10,7 @@ wrote 16777216/16777216 bytes at offset 0
> read 16777216/16777216 bytes at offset 0
> 16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> == create overlay ==
> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=16777216 backing_file=TEST_DIR/t.IMGFMT.base encrypt.format=luks encrypt.key-secret=sec1 encrypt.iter-time=10
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=16777216 backing_file=TEST_DIR/t.IMGFMT.base
>
> == writing part of a cluster ==
> wrote 1024/1024 bytes at offset 0
> diff --git a/tests/qemu-iotests/198.out b/tests/qemu-iotests/198.out
> index 6280ae6eed..4b800e70db 100644
> --- a/tests/qemu-iotests/198.out
> +++ b/tests/qemu-iotests/198.out
> @@ -1,12 +1,12 @@
> QA output created by 198
> == create base ==
> -Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=16777216 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
> +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=16777216
>
> == writing whole image base ==
> wrote 16777216/16777216 bytes at offset 0
> 16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> == create overlay ==
> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=16777216 backing_file=TEST_DIR/t.IMGFMT.base encrypt.format=luks encrypt.key-secret=sec1 encrypt.iter-time=10
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=16777216 backing_file=TEST_DIR/t.IMGFMT.base
>
> == writing whole image layer ==
> wrote 16777216/16777216 bytes at offset 0
> diff --git a/tests/qemu-iotests/263.out b/tests/qemu-iotests/263.out
> index 0c982c55cb..54bfbeeff8 100644
> --- a/tests/qemu-iotests/263.out
> +++ b/tests/qemu-iotests/263.out
> @@ -2,7 +2,7 @@ QA output created by 263
>
> testing LUKS qcow2 encryption
>
> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
> == reading the whole image ==
> read 1048576/1048576 bytes at offset 0
> 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> @@ -21,7 +21,7 @@ read 982528/982528 bytes at offset 66048
>
> testing legacy AES qcow2 encryption
>
> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=aes encrypt.key-secret=sec0
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
> == reading the whole image ==
> read 1048576/1048576 bytes at offset 0
> 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> diff --git a/tests/qemu-iotests/284.out b/tests/qemu-iotests/284.out
> index 48216f5742..a929239302 100644
> --- a/tests/qemu-iotests/284.out
> +++ b/tests/qemu-iotests/284.out
> @@ -2,7 +2,7 @@ QA output created by 284
>
> testing LUKS qcow2 encryption
>
> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
>
> == cluster size 512
> == checking image refcounts ==
> @@ -21,7 +21,7 @@ wrote 1/1 bytes at offset 512
>
> == rechecking image refcounts ==
> No errors were found on the image.
> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
>
> == cluster size 2048
> == checking image refcounts ==
> @@ -40,7 +40,7 @@ wrote 1/1 bytes at offset 2048
>
> == rechecking image refcounts ==
> No errors were found on the image.
> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
>
> == cluster size 32768
> == checking image refcounts ==
> diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
> index f104ad7a9b..9ae7ac9891 100644
> --- a/tests/qemu-iotests/common.filter
> +++ b/tests/qemu-iotests/common.filter
> @@ -182,16 +182,13 @@ _filter_img_create()
> echo "$options" \
> | tr '\n' '\0' \
> | sed -e 's/\x0$//' -e 's/ \([a-z0-9_.-]*\)=/\n\1=/g' \
> - | grep -ae "^\(fmt\\|size\\|backing\\|preallocation\\|encrypt$grep_data_file\\)" \
> + | grep -ae "^\(fmt\\|size\\|backing\\|preallocation\\|encryption$grep_data_file\\)" \
> | $SED "${filename_filters[@]}" \
> -e 's/^\(fmt\)/0-\1/' \
> -e 's/^\(size\)/1-\1/' \
> -e 's/^\(backing\)/2-\1/' \
> -e 's/^\(data_file\)/3-\1/' \
> -e 's/^\(encryption\)/4-\1/' \
> - -e 's/^\(encrypt\.format\)/5-\1/' \
> - -e 's/^\(encrypt\.key-secret\)/6-\1/' \
> - -e 's/^\(encrypt\.iter-time\)/7-\1/' \
> -e 's/^\(preallocation\)/8-\1/' \
> | sort \
> | $SED -e 's/^[0-9]-//' \
Looks OK to me,
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] iotests: Make _filter_img_create more active
2020-06-17 11:46 ` Maxim Levitsky
@ 2020-06-17 13:50 ` Max Reitz
2020-06-17 14:02 ` Maxim Levitsky
0 siblings, 1 reply; 8+ messages in thread
From: Max Reitz @ 2020-06-17 13:50 UTC (permalink / raw)
To: Maxim Levitsky, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 10601 bytes --]
On 17.06.20 13:46, Maxim Levitsky wrote:
> On Tue, 2020-06-16 at 15:17 +0200, Max Reitz wrote:
>> Right now, _filter_img_create just filters out everything that looks
>> format-dependent, and applies some filename filters. That means that we
>> have to add another filter line every time some format gets a new
>> creation option. This can be avoided by instead discarding everything
>> and just keeping what we know is format-independent (format, size,
>> backing file, encryption information[1], preallocation) or just
>> interesting to have in the reference output (external data file path).
>>
>> Furthermore, we probably want to sort these options. Format drivers are
>> not required to define them in any specific order, so the output is
>> effectively random (although this has never bothered us until now). We
>> need a specific order for our reference outputs, though. Unfortunately,
>> just using a plain "sort" would change a lot of existing reference
>> outputs, so we have to pre-filter the option keys to keep our existing
>> order (fmt, size, backing*, data, encryption info, preallocation).
>>
>> [1] Actually, the only thing that is really important is whether
>> encryption is enabled or not. A patch by Maxim thus removes all
>> other "encrypt.*" options from the output:
>> https://lists.nongnu.org/archive/html/qemu-block/2020-06/msg00339.html
>> But that patch needs to come later so we can get away with changing
>> as few reference outputs in this patch here as possible.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> tests/qemu-iotests/112.out | 2 +-
>> tests/qemu-iotests/153 | 9 ++-
>> tests/qemu-iotests/common.filter | 100 +++++++++++++++++++++++--------
>> 3 files changed, 81 insertions(+), 30 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/112.out b/tests/qemu-iotests/112.out
>> index ae0318cabe..182655dbf6 100644
>> --- a/tests/qemu-iotests/112.out
>> +++ b/tests/qemu-iotests/112.out
>> @@ -5,7 +5,7 @@ QA output created by 112
>> qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and may not exceed 64 bits
>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>> qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and may not exceed 64 bits
>> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 refcount_bits=-1
>> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>> qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and may not exceed 64 bits
>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>> qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and may not exceed 64 bits
>> diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153
>> index cf961d3609..11e3d28841 100755
>> --- a/tests/qemu-iotests/153
>> +++ b/tests/qemu-iotests/153
>> @@ -167,11 +167,10 @@ done
>>
>> echo
>> echo "== Creating ${TEST_IMG}.[abc] ==" | _filter_testdir
>> -(
>> - $QEMU_IMG create -f qcow2 "${TEST_IMG}.a" -b "${TEST_IMG}"
>> - $QEMU_IMG create -f qcow2 "${TEST_IMG}.b" -b "${TEST_IMG}"
>> - $QEMU_IMG create -f qcow2 "${TEST_IMG}.c" -b "${TEST_IMG}.b"
>> -) | _filter_img_create
>> +$QEMU_IMG create -f qcow2 "${TEST_IMG}.a" -b "${TEST_IMG}" | _filter_img_create
>> +$QEMU_IMG create -f qcow2 "${TEST_IMG}.b" -b "${TEST_IMG}" | _filter_img_create
>> +$QEMU_IMG create -f qcow2 "${TEST_IMG}.c" -b "${TEST_IMG}.b" \
>> + | _filter_img_create
>>
>> echo
>> echo "== Two devices sharing the same file in backing chain =="
>
> I guess this is done because now the filter expectes only a single
> qemu-img output.
Yes, that’s right.
> IMHO this is better anyway.
>
>> diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
>> index 03e4f71808..f104ad7a9b 100644
>> --- a/tests/qemu-iotests/common.filter
>> +++ b/tests/qemu-iotests/common.filter
>> @@ -122,38 +122,90 @@ _filter_actual_image_size()
>> # replace driver-specific options in the "Formatting..." line
>> _filter_img_create()
>> {
>> - data_file_filter=()
>> - if data_file=$(_get_data_file "$TEST_IMG"); then
>> - data_file_filter=(-e "s# data_file=$data_file##")
>> + # Keep QMP output unchanged
>> + qmp_pre=''
>> + qmp_post=''
>> + to_filter=''
>> +
>> + while read -r line; do
>> + if echo "$line" | grep -q '^{.*}$'; then
>> + if [ -z "$to_filter" ]; then
>> + # Use $'\n' so the newline is not dropped on variable
>> + # expansion
>> + qmp_pre="$qmp_pre$line"$'\n'
>> + else
>> + qmp_post="$qmp_post$line"$'\n'
>> + fi
>> + else
>> + to_filter="$to_filter$line"$'\n'
>> + fi
>> + done
>
> The above code basically assumes that qmp output starts with '{' and ends with '}'
> which I guess is fair, and then it assumes that we can have set of qmp commands prior
> to qemu-img line and another set of qmp commands after it.
> To me it feels like we should have another filter for that, since
> qemu-img itself doesn't use qmp.
Yes, but drive-backup and drive-mirror with mode=absolute-paths use
bdrv_img_create() (quiet=false), which emits the “Formatting” line, too.
So some QMP commands may emit it and then require the same filtering as
qemu-img create.
Not having to do that would certainly make things simpler.
> Which test needs it?
141.
>> +
>> + readarray -td '' formatting_line < \
>> + <(echo "$to_filter" | sed -e 's/, fmt=/\x0/')
> OK, took me a while to understand what this does, but looks OK.
Maybe I should at least put a comment like “Split $to_filter into the
pre-options part (before ", fmt=") and the options part (starting with
"fmt=").
>> +
>> + filename_part=${formatting_line[0]}
>> + if [ -n "${formatting_line[1]}" ]; then
>> + options="fmt=${formatting_line[1]}"
>> + else
>> + options=''
>> fi
>>
> OK.
>
>>
>> - $SED "${data_file_filter[@]}" \
>> + # Set grep_data_file to '\|data_file' to keep it; make it empty
>> + # to drop it.
>> + # We want to drop it if it is part of the global $IMGOPTS, and we
>> + # want to keep it otherwise (if the test specifically wants to
>> + # test data files).
>> + grep_data_file='\|data_file'
>> + if _get_data_file "$TEST_IMG" > /dev/null; then
>> + grep_data_file=''
>> + fi
>> +
>> + filename_filters=(
>> -e "s#$REMOTE_TEST_DIR#TEST_DIR#g" \
>> -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
>> -e "s#$TEST_DIR#TEST_DIR#g" \
>> -e "s#$SOCK_DIR#SOCK_DIR#g" \
>> -e "s#$IMGFMT#IMGFMT#g" \
>> -e 's#nbd:127.0.0.1:[0-9]\\+#TEST_DIR/t.IMGFMT#g' \
>> - -e 's#nbd+unix:///\??socket=SOCK_DIR/nbd#TEST_DIR/t.IMGFMT#g' \
>> - -e "s# encryption=off##g" \
>> - -e "s# cluster_size=[0-9]\\+##g" \
>> - -e "s# table_size=[0-9]\\+##g" \
>> - -e "s# compat=[^ ]*##g" \
>> - -e "s# compat6=\\(on\\|off\\)##g" \
>> - -e "s# static=\\(on\\|off\\)##g" \
>> - -e "s# zeroed_grain=\\(on\\|off\\)##g" \
>> - -e "s# subformat=[^ ]*##g" \
>> - -e "s# adapter_type=[^ ]*##g" \
>> - -e "s# hwversion=[^ ]*##g" \
>> - -e "s# lazy_refcounts=\\(on\\|off\\)##g" \
>> - -e "s# block_size=[0-9]\\+##g" \
>> - -e "s# block_state_zero=\\(on\\|off\\)##g" \
>> - -e "s# log_size=[0-9]\\+##g" \
>> - -e "s# refcount_bits=[0-9]\\+##g" \
>> - -e "s# key-secret=[a-zA-Z0-9]\\+##g" \
>> - -e "s# iter-time=[0-9]\\+##g" \
>> - -e "s# force_size=\\(on\\|off\\)##g" \
>> - -e "s# compression_type=[a-zA-Z0-9]\\+##g"
>> + -e 's#nbd+unix:///\??socket=SOCK_DIR/nbd#TEST_DIR/t.IMGFMT#g'
>> + )
>> +
>> + filename_part=$(echo "$filename_part" | $SED "${filename_filters[@]}")
> Looks good as well.
>
>> +
>> + # Break the option line before each option (preserving pre-existing
>> + # line breaks by replacing them by \0 and restoring them at the end),
>> + # then filter out the options we want to keep and sort them according
>> + # to some order that all block drivers used at the time of writing
>> + # this function.
>> + options=$(
>> + echo "$options" \
>> + | tr '\n' '\0' \
>> + | sed -e 's/\x0$//' -e 's/ \([a-z0-9_.-]*\)=/\n\1=/g' \
> You sometimes use $SED and sometimes sed. Is this intentional?
Oops, no. It should be $SED everywhere.
>> + | grep -ae "^\(fmt\\|size\\|backing\\|preallocation\\|encrypt$grep_data_file\\)" \
>> + | $SED "${filename_filters[@]}" \
>> + -e 's/^\(fmt\)/0-\1/' \
>> + -e 's/^\(size\)/1-\1/' \
>> + -e 's/^\(backing\)/2-\1/' \
>> + -e 's/^\(data_file\)/3-\1/' \
>> + -e 's/^\(encryption\)/4-\1/' \
>> + -e 's/^\(encrypt\.format\)/5-\1/' \
>> + -e 's/^\(encrypt\.key-secret\)/6-\1/' \
>> + -e 's/^\(encrypt\.iter-time\)/7-\1/' \
>> + -e 's/^\(preallocation\)/8-\1/' \
> All right, I understand this now, but do we have to do this?
> Maybe it is better to just update the outputs once to avoid keeping
> the custom sort order?
Well. I don’t know. The advantage of doing this is that I can’t
accidentally miss updating any reference outputs that aren’t used on my
machine (like 026.out.nocache or 051.out).
So we don’t strictly have to do this, but I found this to be simpler.
If someone wants to drop this and in turn update all reference outputs,
they’ll be my guest, but I preferred not to do that myself.
>> + | sort \
>> + | $SED -e 's/^[0-9]-//' \
>> + | tr '\n\0' ' \n' \
>> + | $SED -e 's/^ *$//' -e 's/ *$//'
>> + )
>
> For the above bash pipeline overall: It was hard to decipher :-), but I must admit
> I learned something from it.
I definitely learned something while working on this, too. I’m not sure
whether I also gained any useful knowledge, though. O:)
>> +
>> + echo -n "$qmp_pre"
>> + if [ -n "$options" ]; then
>> + echo "$filename_part, $options"
>> + elif [ -n "$filename_part" ]; then
>> + echo "$filename_part"
>> + fi
>> + echo -n "$qmp_post"
>> }
>>
>> _filter_img_create_size()
>
> Overall I like the idea of this patch.
Good!
Thanks for reviewing! :)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] iotests: Make _filter_img_create more active
2020-06-17 13:50 ` Max Reitz
@ 2020-06-17 14:02 ` Maxim Levitsky
2020-06-17 14:20 ` Max Reitz
0 siblings, 1 reply; 8+ messages in thread
From: Maxim Levitsky @ 2020-06-17 14:02 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
On Wed, 2020-06-17 at 15:50 +0200, Max Reitz wrote:
> On 17.06.20 13:46, Maxim Levitsky wrote:
> > On Tue, 2020-06-16 at 15:17 +0200, Max Reitz wrote:
> > > Right now, _filter_img_create just filters out everything that looks
> > > format-dependent, and applies some filename filters. That means that we
> > > have to add another filter line every time some format gets a new
> > > creation option. This can be avoided by instead discarding everything
> > > and just keeping what we know is format-independent (format, size,
> > > backing file, encryption information[1], preallocation) or just
> > > interesting to have in the reference output (external data file path).
> > >
> > > Furthermore, we probably want to sort these options. Format drivers are
> > > not required to define them in any specific order, so the output is
> > > effectively random (although this has never bothered us until now). We
> > > need a specific order for our reference outputs, though. Unfortunately,
> > > just using a plain "sort" would change a lot of existing reference
> > > outputs, so we have to pre-filter the option keys to keep our existing
> > > order (fmt, size, backing*, data, encryption info, preallocation).
> > >
> > > [1] Actually, the only thing that is really important is whether
> > > encryption is enabled or not. A patch by Maxim thus removes all
> > > other "encrypt.*" options from the output:
> > > https://lists.nongnu.org/archive/html/qemu-block/2020-06/msg00339.html
> > > But that patch needs to come later so we can get away with changing
> > > as few reference outputs in this patch here as possible.
> > >
> > > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > > ---
> > > tests/qemu-iotests/112.out | 2 +-
> > > tests/qemu-iotests/153 | 9 ++-
> > > tests/qemu-iotests/common.filter | 100 +++++++++++++++++++++++--------
> > > 3 files changed, 81 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/tests/qemu-iotests/112.out b/tests/qemu-iotests/112.out
> > > index ae0318cabe..182655dbf6 100644
> > > --- a/tests/qemu-iotests/112.out
> > > +++ b/tests/qemu-iotests/112.out
> > > @@ -5,7 +5,7 @@ QA output created by 112
> > > qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and may not exceed 64 bits
> > > Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> > > qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and may not exceed 64 bits
> > > -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 refcount_bits=-1
> > > +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> > > qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and may not exceed 64 bits
> > > Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> > > qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and may not exceed 64 bits
> > > diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153
> > > index cf961d3609..11e3d28841 100755
> > > --- a/tests/qemu-iotests/153
> > > +++ b/tests/qemu-iotests/153
> > > @@ -167,11 +167,10 @@ done
> > >
> > > echo
> > > echo "== Creating ${TEST_IMG}.[abc] ==" | _filter_testdir
> > > -(
> > > - $QEMU_IMG create -f qcow2 "${TEST_IMG}.a" -b "${TEST_IMG}"
> > > - $QEMU_IMG create -f qcow2 "${TEST_IMG}.b" -b "${TEST_IMG}"
> > > - $QEMU_IMG create -f qcow2 "${TEST_IMG}.c" -b "${TEST_IMG}.b"
> > > -) | _filter_img_create
> > > +$QEMU_IMG create -f qcow2 "${TEST_IMG}.a" -b "${TEST_IMG}" | _filter_img_create
> > > +$QEMU_IMG create -f qcow2 "${TEST_IMG}.b" -b "${TEST_IMG}" | _filter_img_create
> > > +$QEMU_IMG create -f qcow2 "${TEST_IMG}.c" -b "${TEST_IMG}.b" \
> > > + | _filter_img_create
> > >
> > > echo
> > > echo "== Two devices sharing the same file in backing chain =="
> >
> > I guess this is done because now the filter expectes only a single
> > qemu-img output.
>
> Yes, that’s right.
>
> > IMHO this is better anyway.
> >
> > > diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
> > > index 03e4f71808..f104ad7a9b 100644
> > > --- a/tests/qemu-iotests/common.filter
> > > +++ b/tests/qemu-iotests/common.filter
> > > @@ -122,38 +122,90 @@ _filter_actual_image_size()
> > > # replace driver-specific options in the "Formatting..." line
> > > _filter_img_create()
> > > {
> > > - data_file_filter=()
> > > - if data_file=$(_get_data_file "$TEST_IMG"); then
> > > - data_file_filter=(-e "s# data_file=$data_file##")
> > > + # Keep QMP output unchanged
> > > + qmp_pre=''
> > > + qmp_post=''
> > > + to_filter=''
> > > +
> > > + while read -r line; do
> > > + if echo "$line" | grep -q '^{.*}$'; then
> > > + if [ -z "$to_filter" ]; then
> > > + # Use $'\n' so the newline is not dropped on variable
> > > + # expansion
> > > + qmp_pre="$qmp_pre$line"$'\n'
> > > + else
> > > + qmp_post="$qmp_post$line"$'\n'
> > > + fi
> > > + else
> > > + to_filter="$to_filter$line"$'\n'
> > > + fi
> > > + done
> >
> > The above code basically assumes that qmp output starts with '{' and ends with '}'
> > which I guess is fair, and then it assumes that we can have set of qmp commands prior
> > to qemu-img line and another set of qmp commands after it.
> > To me it feels like we should have another filter for that, since
> > qemu-img itself doesn't use qmp.
>
> Yes, but drive-backup and drive-mirror with mode=absolute-paths use
> bdrv_img_create() (quiet=false), which emits the “Formatting” line, too.
> So some QMP commands may emit it and then require the same filtering as
> qemu-img create.
Ah. Do we need this though?
Assuming yes, maybe we should create a new filter called something like _filter_drive_backup_mirror
or something like that that would filter qmp output, and pipe the 'Formatting' line
to _filter_img_create which then can have the qmp parsing part removed?
>
> Not having to do that would certainly make things simpler.
>
> > Which test needs it?
>
> 141.
>
> > > +
> > > + readarray -td '' formatting_line < \
> > > + <(echo "$to_filter" | sed -e 's/, fmt=/\x0/')
> > OK, took me a while to understand what this does, but looks OK.
>
> Maybe I should at least put a comment like “Split $to_filter into the
> pre-options part (before ", fmt=") and the options part (starting with
> "fmt=").
>
> > > +
> > > + filename_part=${formatting_line[0]}
> > > + if [ -n "${formatting_line[1]}" ]; then
> > > + options="fmt=${formatting_line[1]}"
> > > + else
> > > + options=''
> > > fi
> > >
> > OK.
> >
> > >
> > > - $SED "${data_file_filter[@]}" \
> > > + # Set grep_data_file to '\|data_file' to keep it; make it empty
> > > + # to drop it.
> > > + # We want to drop it if it is part of the global $IMGOPTS, and we
> > > + # want to keep it otherwise (if the test specifically wants to
> > > + # test data files).
> > > + grep_data_file='\|data_file'
> > > + if _get_data_file "$TEST_IMG" > /dev/null; then
> > > + grep_data_file=''
> > > + fi
> > > +
> > > + filename_filters=(
> > > -e "s#$REMOTE_TEST_DIR#TEST_DIR#g" \
> > > -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
> > > -e "s#$TEST_DIR#TEST_DIR#g" \
> > > -e "s#$SOCK_DIR#SOCK_DIR#g" \
> > > -e "s#$IMGFMT#IMGFMT#g" \
> > > -e 's#nbd:127.0.0.1:[0-9]\\+#TEST_DIR/t.IMGFMT#g' \
> > > - -e 's#nbd+unix:///\??socket=SOCK_DIR/nbd#TEST_DIR/t.IMGFMT#g' \
> > > - -e "s# encryption=off##g" \
> > > - -e "s# cluster_size=[0-9]\\+##g" \
> > > - -e "s# table_size=[0-9]\\+##g" \
> > > - -e "s# compat=[^ ]*##g" \
> > > - -e "s# compat6=\\(on\\|off\\)##g" \
> > > - -e "s# static=\\(on\\|off\\)##g" \
> > > - -e "s# zeroed_grain=\\(on\\|off\\)##g" \
> > > - -e "s# subformat=[^ ]*##g" \
> > > - -e "s# adapter_type=[^ ]*##g" \
> > > - -e "s# hwversion=[^ ]*##g" \
> > > - -e "s# lazy_refcounts=\\(on\\|off\\)##g" \
> > > - -e "s# block_size=[0-9]\\+##g" \
> > > - -e "s# block_state_zero=\\(on\\|off\\)##g" \
> > > - -e "s# log_size=[0-9]\\+##g" \
> > > - -e "s# refcount_bits=[0-9]\\+##g" \
> > > - -e "s# key-secret=[a-zA-Z0-9]\\+##g" \
> > > - -e "s# iter-time=[0-9]\\+##g" \
> > > - -e "s# force_size=\\(on\\|off\\)##g" \
> > > - -e "s# compression_type=[a-zA-Z0-9]\\+##g"
> > > + -e 's#nbd+unix:///\??socket=SOCK_DIR/nbd#TEST_DIR/t.IMGFMT#g'
> > > + )
> > > +
> > > + filename_part=$(echo "$filename_part" | $SED "${filename_filters[@]}")
> > Looks good as well.
> >
> > > +
> > > + # Break the option line before each option (preserving pre-existing
> > > + # line breaks by replacing them by \0 and restoring them at the end),
> > > + # then filter out the options we want to keep and sort them according
> > > + # to some order that all block drivers used at the time of writing
> > > + # this function.
> > > + options=$(
> > > + echo "$options" \
> > > + | tr '\n' '\0' \
> > > + | sed -e 's/\x0$//' -e 's/ \([a-z0-9_.-]*\)=/\n\1=/g' \
> > You sometimes use $SED and sometimes sed. Is this intentional?
>
> Oops, no. It should be $SED everywhere.
>
> > > + | grep -ae "^\(fmt\\|size\\|backing\\|preallocation\\|encrypt$grep_data_file\\)" \
> > > + | $SED "${filename_filters[@]}" \
> > > + -e 's/^\(fmt\)/0-\1/' \
> > > + -e 's/^\(size\)/1-\1/' \
> > > + -e 's/^\(backing\)/2-\1/' \
> > > + -e 's/^\(data_file\)/3-\1/' \
> > > + -e 's/^\(encryption\)/4-\1/' \
> > > + -e 's/^\(encrypt\.format\)/5-\1/' \
> > > + -e 's/^\(encrypt\.key-secret\)/6-\1/' \
> > > + -e 's/^\(encrypt\.iter-time\)/7-\1/' \
> > > + -e 's/^\(preallocation\)/8-\1/' \
> > All right, I understand this now, but do we have to do this?
> > Maybe it is better to just update the outputs once to avoid keeping
> > the custom sort order?
>
> Well. I don’t know. The advantage of doing this is that I can’t
> accidentally miss updating any reference outputs that aren’t used on my
> machine (like 026.out.nocache or 051.out).
Fair enough. A follow up patch can always be made to fix this.
>
> So we don’t strictly have to do this, but I found this to be simpler.
> If someone wants to drop this and in turn update all reference outputs,
> they’ll be my guest, but I preferred not to do that myself.
>
> > > + | sort \
> > > + | $SED -e 's/^[0-9]-//' \
> > > + | tr '\n\0' ' \n' \
> > > + | $SED -e 's/^ *$//' -e 's/ *$//'
> > > + )
> >
> > For the above bash pipeline overall: It was hard to decipher :-), but I must admit
> > I learned something from it.
>
> I definitely learned something while working on this, too. I’m not sure
> whether I also gained any useful knowledge, though. O:)
In my past experience, any knowelege eventually turns out to be useful.
>
> > > +
> > > + echo -n "$qmp_pre"
> > > + if [ -n "$options" ]; then
> > > + echo "$filename_part, $options"
> > > + elif [ -n "$filename_part" ]; then
> > > + echo "$filename_part"
> > > + fi
> > > + echo -n "$qmp_post"
> > > }
> > >
> > > _filter_img_create_size()
> >
> > Overall I like the idea of this patch.
>
> Good!
>
> Thanks for reviewing! :)
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] iotests: Make _filter_img_create more active
2020-06-17 14:02 ` Maxim Levitsky
@ 2020-06-17 14:20 ` Max Reitz
0 siblings, 0 replies; 8+ messages in thread
From: Max Reitz @ 2020-06-17 14:20 UTC (permalink / raw)
To: Maxim Levitsky, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 9146 bytes --]
On 17.06.20 16:02, Maxim Levitsky wrote:
> On Wed, 2020-06-17 at 15:50 +0200, Max Reitz wrote:
>> On 17.06.20 13:46, Maxim Levitsky wrote:
>>> On Tue, 2020-06-16 at 15:17 +0200, Max Reitz wrote:
>>>> Right now, _filter_img_create just filters out everything that looks
>>>> format-dependent, and applies some filename filters. That means that we
>>>> have to add another filter line every time some format gets a new
>>>> creation option. This can be avoided by instead discarding everything
>>>> and just keeping what we know is format-independent (format, size,
>>>> backing file, encryption information[1], preallocation) or just
>>>> interesting to have in the reference output (external data file path).
>>>>
>>>> Furthermore, we probably want to sort these options. Format drivers are
>>>> not required to define them in any specific order, so the output is
>>>> effectively random (although this has never bothered us until now). We
>>>> need a specific order for our reference outputs, though. Unfortunately,
>>>> just using a plain "sort" would change a lot of existing reference
>>>> outputs, so we have to pre-filter the option keys to keep our existing
>>>> order (fmt, size, backing*, data, encryption info, preallocation).
>>>>
>>>> [1] Actually, the only thing that is really important is whether
>>>> encryption is enabled or not. A patch by Maxim thus removes all
>>>> other "encrypt.*" options from the output:
>>>> https://lists.nongnu.org/archive/html/qemu-block/2020-06/msg00339.html
>>>> But that patch needs to come later so we can get away with changing
>>>> as few reference outputs in this patch here as possible.
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>> tests/qemu-iotests/112.out | 2 +-
>>>> tests/qemu-iotests/153 | 9 ++-
>>>> tests/qemu-iotests/common.filter | 100 +++++++++++++++++++++++--------
>>>> 3 files changed, 81 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/tests/qemu-iotests/112.out b/tests/qemu-iotests/112.out
>>>> index ae0318cabe..182655dbf6 100644
>>>> --- a/tests/qemu-iotests/112.out
>>>> +++ b/tests/qemu-iotests/112.out
>>>> @@ -5,7 +5,7 @@ QA output created by 112
>>>> qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and may not exceed 64 bits
>>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>>>> qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and may not exceed 64 bits
>>>> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 refcount_bits=-1
>>>> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>>>> qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and may not exceed 64 bits
>>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>>>> qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and may not exceed 64 bits
>>>> diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153
>>>> index cf961d3609..11e3d28841 100755
>>>> --- a/tests/qemu-iotests/153
>>>> +++ b/tests/qemu-iotests/153
>>>> @@ -167,11 +167,10 @@ done
>>>>
>>>> echo
>>>> echo "== Creating ${TEST_IMG}.[abc] ==" | _filter_testdir
>>>> -(
>>>> - $QEMU_IMG create -f qcow2 "${TEST_IMG}.a" -b "${TEST_IMG}"
>>>> - $QEMU_IMG create -f qcow2 "${TEST_IMG}.b" -b "${TEST_IMG}"
>>>> - $QEMU_IMG create -f qcow2 "${TEST_IMG}.c" -b "${TEST_IMG}.b"
>>>> -) | _filter_img_create
>>>> +$QEMU_IMG create -f qcow2 "${TEST_IMG}.a" -b "${TEST_IMG}" | _filter_img_create
>>>> +$QEMU_IMG create -f qcow2 "${TEST_IMG}.b" -b "${TEST_IMG}" | _filter_img_create
>>>> +$QEMU_IMG create -f qcow2 "${TEST_IMG}.c" -b "${TEST_IMG}.b" \
>>>> + | _filter_img_create
>>>>
>>>> echo
>>>> echo "== Two devices sharing the same file in backing chain =="
>>>
>>> I guess this is done because now the filter expectes only a single
>>> qemu-img output.
>>
>> Yes, that’s right.
>>
>>> IMHO this is better anyway.
>>>
>>>> diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
>>>> index 03e4f71808..f104ad7a9b 100644
>>>> --- a/tests/qemu-iotests/common.filter
>>>> +++ b/tests/qemu-iotests/common.filter
>>>> @@ -122,38 +122,90 @@ _filter_actual_image_size()
>>>> # replace driver-specific options in the "Formatting..." line
>>>> _filter_img_create()
>>>> {
>>>> - data_file_filter=()
>>>> - if data_file=$(_get_data_file "$TEST_IMG"); then
>>>> - data_file_filter=(-e "s# data_file=$data_file##")
>>>> + # Keep QMP output unchanged
>>>> + qmp_pre=''
>>>> + qmp_post=''
>>>> + to_filter=''
>>>> +
>>>> + while read -r line; do
>>>> + if echo "$line" | grep -q '^{.*}$'; then
>>>> + if [ -z "$to_filter" ]; then
>>>> + # Use $'\n' so the newline is not dropped on variable
>>>> + # expansion
>>>> + qmp_pre="$qmp_pre$line"$'\n'
>>>> + else
>>>> + qmp_post="$qmp_post$line"$'\n'
>>>> + fi
>>>> + else
>>>> + to_filter="$to_filter$line"$'\n'
>>>> + fi
>>>> + done
>>>
>>> The above code basically assumes that qmp output starts with '{' and ends with '}'
>>> which I guess is fair, and then it assumes that we can have set of qmp commands prior
>>> to qemu-img line and another set of qmp commands after it.
>>> To me it feels like we should have another filter for that, since
>>> qemu-img itself doesn't use qmp.
>>
>> Yes, but drive-backup and drive-mirror with mode=absolute-paths use
>> bdrv_img_create() (quiet=false), which emits the “Formatting” line, too.
>> So some QMP commands may emit it and then require the same filtering as
>> qemu-img create.
>
> Ah. Do we need this though?
> Assuming yes, maybe we should create a new filter called something like _filter_drive_backup_mirror
> or something like that that would filter qmp output, and pipe the 'Formatting' line
> to _filter_img_create which then can have the qmp parsing part removed?
Hm. I don’t think it would be that much more simpler. (Instead of just
putting it into this function.) But perhaps cleaner.
One of the problems is that I don’t want to actually parse JSON here.
That leaves us with the problem of what to do with line breaks. Both
QMP stuff and any Formatting line may contain line breaks (and there
actually are iotests that have line breaks in their Formatting line).
In this function, I opted to allow line breaks in Formatting lines, but
not in QMP. If we had a separate function specifically for Formatting
stuff in QMP, I think it should probably work the other way around.
So what this other function would do is probably to just echo all lines
that don’t start with Formatting, and filter those Formatting line(s)
through _filter_img_create. (I don’t think we need to support
multi-line Formatting lines in QMP output.)
I think that should work. And it’s probably indeed cleaner than putting
both into a single function. I’ll try my hands on it.
[...]
>>>> + | grep -ae "^\(fmt\\|size\\|backing\\|preallocation\\|encrypt$grep_data_file\\)" \
>>>> + | $SED "${filename_filters[@]}" \
>>>> + -e 's/^\(fmt\)/0-\1/' \
>>>> + -e 's/^\(size\)/1-\1/' \
>>>> + -e 's/^\(backing\)/2-\1/' \
>>>> + -e 's/^\(data_file\)/3-\1/' \
>>>> + -e 's/^\(encryption\)/4-\1/' \
>>>> + -e 's/^\(encrypt\.format\)/5-\1/' \
>>>> + -e 's/^\(encrypt\.key-secret\)/6-\1/' \
>>>> + -e 's/^\(encrypt\.iter-time\)/7-\1/' \
>>>> + -e 's/^\(preallocation\)/8-\1/' \
>>> All right, I understand this now, but do we have to do this?
>>> Maybe it is better to just update the outputs once to avoid keeping
>>> the custom sort order?
>>
>> Well. I don’t know. The advantage of doing this is that I can’t
>> accidentally miss updating any reference outputs that aren’t used on my
>> machine (like 026.out.nocache or 051.out).
>
> Fair enough. A follow up patch can always be made to fix this.
Right.
>> So we don’t strictly have to do this, but I found this to be simpler.
>> If someone wants to drop this and in turn update all reference outputs,
>> they’ll be my guest, but I preferred not to do that myself.
>>
>>>> + | sort \
>>>> + | $SED -e 's/^[0-9]-//' \
>>>> + | tr '\n\0' ' \n' \
>>>> + | $SED -e 's/^ *$//' -e 's/ *$//'
>>>> + )
>>>
>>> For the above bash pipeline overall: It was hard to decipher :-), but I must admit
>>> I learned something from it.
>>
>> I definitely learned something while working on this, too. I’m not sure
>> whether I also gained any useful knowledge, though. O:)
>
> In my past experience, any knowelege eventually turns out to be useful.
In my experience, some knowledge can turn out harmful, because suddenly
you’re the expert on it. O:)
(I really don’t want to be the expert on bash. Luckily, though, we have
Eric who fills that role quite nicely already.)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-06-17 14:22 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-16 13:17 [PATCH 0/2] iotests: Make _filter_img_create more active Max Reitz
2020-06-16 13:17 ` [PATCH 1/2] " Max Reitz
2020-06-17 11:46 ` Maxim Levitsky
2020-06-17 13:50 ` Max Reitz
2020-06-17 14:02 ` Maxim Levitsky
2020-06-17 14:20 ` Max Reitz
2020-06-16 13:17 ` [PATCH 2/2] iotests: filter few more luks specific create options Max Reitz
2020-06-17 11:47 ` Maxim Levitsky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.