* [PATCH for-5.0 v2 1/3] qemu-img: Fix check's leak/corruption fix report
2020-03-24 17:27 [PATCH for-5.0 v2 0/3] qemu-img: Fix check's leak/corruption fix report Max Reitz
@ 2020-03-24 17:27 ` Max Reitz
2020-03-24 17:27 ` [PATCH for-5.0 v2 2/3] iotests: Add poke_file_[bl]e functions Max Reitz
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Max Reitz @ 2020-03-24 17:27 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
There are two problems with qemu-img check's report on how many leaks
and/or corruptions have been fixed:
(1) ImageCheck.has_leaks_fixed and ImageCheck.has_corruptions_fixed are
only true when ImageCheck.leaks or ImageCheck.corruptions (respectively)
are non-zero. qcow2's check implementation will set the latter to zero
after it has fixed leaks and corruptions, though, so leaks-fixed and
corruptions-fixed are actually never reported after successful repairs.
We should always report them when they are non-zero, just like all the
other fields of ImageCheck.
(2) After something has been fixed and we run the check a second time,
leaks_fixed and corruptions_fixed are taken from the first run; but
has_leaks_fixed and has_corruptions_fixed are not. The second run
actually cannot fix anything, so with (1) fixed, has_leaks_fixed and
has_corruptions_fixed will always be false here. (With (1) unfixed,
they will at least be false on successful runs, because then the number
of leaks and corruptions found in the second run should be 0.)
We should save has_leaks_fixed and has_corruptions_fixed just like we
save leaks_fixed and corruptions_fixed.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
qemu-img.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index afddf33f08..b167376bd7 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -647,9 +647,9 @@ static int collect_image_check(BlockDriverState *bs,
check->leaks = result.leaks;
check->has_leaks = result.leaks != 0;
check->corruptions_fixed = result.corruptions_fixed;
- check->has_corruptions_fixed = result.corruptions != 0;
+ check->has_corruptions_fixed = result.corruptions_fixed != 0;
check->leaks_fixed = result.leaks_fixed;
- check->has_leaks_fixed = result.leaks != 0;
+ check->has_leaks_fixed = result.leaks_fixed != 0;
check->image_end_offset = result.image_end_offset;
check->has_image_end_offset = result.image_end_offset != 0;
check->total_clusters = result.bfi.total_clusters;
@@ -803,9 +803,12 @@ static int img_check(int argc, char **argv)
if (check->corruptions_fixed || check->leaks_fixed) {
int corruptions_fixed, leaks_fixed;
+ bool has_leaks_fixed, has_corruptions_fixed;
leaks_fixed = check->leaks_fixed;
+ has_leaks_fixed = check->has_leaks_fixed;
corruptions_fixed = check->corruptions_fixed;
+ has_corruptions_fixed = check->has_corruptions_fixed;
if (output_format == OFORMAT_HUMAN) {
qprintf(quiet,
@@ -822,7 +825,9 @@ static int img_check(int argc, char **argv)
ret = collect_image_check(bs, check, filename, fmt, 0);
check->leaks_fixed = leaks_fixed;
+ check->has_leaks_fixed = has_leaks_fixed;
check->corruptions_fixed = corruptions_fixed;
+ check->has_corruptions_fixed = has_corruptions_fixed;
}
if (!ret) {
--
2.25.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH for-5.0 v2 2/3] iotests: Add poke_file_[bl]e functions
2020-03-24 17:27 [PATCH for-5.0 v2 0/3] qemu-img: Fix check's leak/corruption fix report Max Reitz
2020-03-24 17:27 ` [PATCH for-5.0 v2 1/3] " Max Reitz
@ 2020-03-24 17:27 ` Max Reitz
2020-03-24 17:51 ` Eric Blake
2020-03-24 17:27 ` [PATCH for-5.0 v2 3/3] iotests/138: Test leaks/corruptions fixed report Max Reitz
2020-03-26 13:53 ` [PATCH for-5.0 v2 0/3] qemu-img: Fix check's leak/corruption fix report Max Reitz
3 siblings, 1 reply; 7+ messages in thread
From: Max Reitz @ 2020-03-24 17:27 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
Similarly to peek_file_[bl]e, we may want to write binary integers into
a file. Currently, this often means messing around with poke_file and
raw binary strings. I hope these functions make it a bit more
comfortable.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Code-suggested-by: Eric Blake <eblake@redhat.com>
---
tests/qemu-iotests/common.rc | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 4c246c0450..bf3b9fdea0 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -53,6 +53,30 @@ poke_file()
printf "$3" | dd "of=$1" bs=1 "seek=$2" conv=notrunc &>/dev/null
}
+# poke_file_le $img_filename $offset $byte_width $value
+# Example: poke_file_le "$TEST_IMG" 512 2 65534
+poke_file_le()
+{
+ local img=$1 ofs=$2 len=$3 val=$4 str=''
+
+ while ((len--)); do
+ str+=$(printf '\\x%02x' $((val & 0xff)))
+ val=$((val >> 8))
+ done
+
+ poke_file "$img" "$ofs" "$str"
+}
+
+# poke_file_be $img_filename $offset $byte_width $value
+# Example: poke_file_be "$TEST_IMG" 512 2 65279
+poke_file_be()
+{
+ local img=$1 ofs=$2 len=$3 val=$4
+ local str=$(printf "%0$((len * 2))x\n" $val | sed 's/\(..\)/\\x\1/g')
+
+ poke_file "$img" "$ofs" "$str"
+}
+
# peek_file_le 'test.img' 512 2 => 65534
peek_file_le()
{
--
2.25.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH for-5.0 v2 2/3] iotests: Add poke_file_[bl]e functions
2020-03-24 17:27 ` [PATCH for-5.0 v2 2/3] iotests: Add poke_file_[bl]e functions Max Reitz
@ 2020-03-24 17:51 ` Eric Blake
0 siblings, 0 replies; 7+ messages in thread
From: Eric Blake @ 2020-03-24 17:51 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
On 3/24/20 12:27 PM, Max Reitz wrote:
> Similarly to peek_file_[bl]e, we may want to write binary integers into
> a file. Currently, this often means messing around with poke_file and
> raw binary strings. I hope these functions make it a bit more
> comfortable.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Code-suggested-by: Eric Blake <eblake@redhat.com>
> ---
> tests/qemu-iotests/common.rc | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 4c246c0450..bf3b9fdea0 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -53,6 +53,30 @@ poke_file()
> printf "$3" | dd "of=$1" bs=1 "seek=$2" conv=notrunc &>/dev/null
> }
>
> +# poke_file_le $img_filename $offset $byte_width $value
> +# Example: poke_file_le "$TEST_IMG" 512 2 65534
> +poke_file_le()
Yep, that's nicer.
> +{
> + local img=$1 ofs=$2 len=$3 val=$4 str=''
> +
> + while ((len--)); do
> + str+=$(printf '\\x%02x' $((val & 0xff)))
> + val=$((val >> 8))
> + done
> +
> + poke_file "$img" "$ofs" "$str"
> +}
and so is that (but I'm biased, here :)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH for-5.0 v2 3/3] iotests/138: Test leaks/corruptions fixed report
2020-03-24 17:27 [PATCH for-5.0 v2 0/3] qemu-img: Fix check's leak/corruption fix report Max Reitz
2020-03-24 17:27 ` [PATCH for-5.0 v2 1/3] " Max Reitz
2020-03-24 17:27 ` [PATCH for-5.0 v2 2/3] iotests: Add poke_file_[bl]e functions Max Reitz
@ 2020-03-24 17:27 ` Max Reitz
2020-03-24 17:56 ` Eric Blake
2020-03-26 13:53 ` [PATCH for-5.0 v2 0/3] qemu-img: Fix check's leak/corruption fix report Max Reitz
3 siblings, 1 reply; 7+ messages in thread
From: Max Reitz @ 2020-03-24 17:27 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
Test that qemu-img check reports the number of leaks and corruptions
fixed in its JSON report (after a successful run).
While touching the _unsupported_imgopts line, adjust the note on why
data_file does not work with this test: The current comment sounds a bit
like it is a mistake for qemu-img check not to check external data
files' refcounts. But there are no such refcounts, so it is no mistake.
Just say that qemu-img check does not do much for external data files,
and this is why this test does not work with them.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/138 | 41 ++++++++++++++++++++++++++++++++++++--
tests/qemu-iotests/138.out | 14 +++++++++++++
2 files changed, 53 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/138 b/tests/qemu-iotests/138
index 54b01046ad..1d5b0bed6d 100755
--- a/tests/qemu-iotests/138
+++ b/tests/qemu-iotests/138
@@ -41,8 +41,10 @@ _supported_fmt qcow2
_supported_proto file
_supported_os Linux
# With an external data file, data clusters are not refcounted
-# (and so qemu-img check does not check their refcount)
-_unsupported_imgopts data_file
+# (so qemu-img check would not do much);
+# we want to modify the refcounts, so we need them to have a specific
+# format (namely u16)
+_unsupported_imgopts data_file 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
echo
echo '=== Check on an image with a multiple of 2^32 clusters ==='
@@ -65,6 +67,41 @@ poke_file "$TEST_IMG" $((2048 + 8)) "\x00\x80\x00\x00\x00\x00\x00\x00"
# allocate memory", we have an error showing that l2 entry is invalid.
_check_test_img
+echo
+echo '=== Check leaks-fixed/corruptions-fixed report'
+echo
+
+# After leaks and corruptions were fixed, those numbers should be
+# reported by qemu-img check
+_make_test_img 64k
+
+# Allocate data cluster
+$QEMU_IO -c 'write 0 64k' "$TEST_IMG" | _filter_qemu_io
+
+reftable_ofs=$(peek_file_be "$TEST_IMG" 48 8)
+refblock_ofs=$(peek_file_be "$TEST_IMG" $reftable_ofs 8)
+
+# Introduce a leak: Make the image header's refcount 2
+poke_file_be "$TEST_IMG" "$refblock_ofs" 2 2
+
+l1_ofs=$(peek_file_be "$TEST_IMG" 40 8)
+
+# Introduce a corruption: Drop the COPIED flag from the (first) L1 entry
+l1_entry=$(peek_file_be "$TEST_IMG" $l1_ofs 8)
+l1_entry=$((l1_entry & ~(1 << 63)))
+poke_file_be "$TEST_IMG" $l1_ofs 8 $l1_entry
+
+echo
+# Should print the number of corruptions and leaks fixed
+# (Filter out all JSON fields (recognizable by their four-space
+# indentation), but keep the "-fixed" fields (by removing two spaces
+# from their indentation))
+# (Also filter out the L1 entry, because why not)
+_check_test_img -r all --output=json \
+ | sed -e 's/^ \(.*\)-fixed"/\1-fixed"/' \
+ -e '/^ /d' \
+ -e "s/\\([^0-9a-f]\\)$(printf %x $l1_entry)\\([^0-9a-f]\\)/\1L1_ENTRY_VALUE\2/"
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/138.out b/tests/qemu-iotests/138.out
index aca7d47a80..79681e7cc9 100644
--- a/tests/qemu-iotests/138.out
+++ b/tests/qemu-iotests/138.out
@@ -9,4 +9,18 @@ ERROR: counting reference for region exceeding the end of the file by one cluste
1 errors were found on the image.
Data may be corrupted, or further writes to the image may corrupt it.
+
+=== Check leaks-fixed/corruptions-fixed report
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+Leaked cluster 0 refcount=2 reference=1
+Repairing cluster 0 refcount=2 reference=1
+Repairing OFLAG_COPIED L2 cluster: l1_index=0 l1_entry=L1_ENTRY_VALUE refcount=1
+{
+ "corruptions-fixed": 1,
+ "leaks-fixed": 1,
+}
*** done
--
2.25.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH for-5.0 v2 3/3] iotests/138: Test leaks/corruptions fixed report
2020-03-24 17:27 ` [PATCH for-5.0 v2 3/3] iotests/138: Test leaks/corruptions fixed report Max Reitz
@ 2020-03-24 17:56 ` Eric Blake
0 siblings, 0 replies; 7+ messages in thread
From: Eric Blake @ 2020-03-24 17:56 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
On 3/24/20 12:27 PM, Max Reitz wrote:
> Test that qemu-img check reports the number of leaks and corruptions
> fixed in its JSON report (after a successful run).
>
> While touching the _unsupported_imgopts line, adjust the note on why
> data_file does not work with this test: The current comment sounds a bit
> like it is a mistake for qemu-img check not to check external data
> files' refcounts. But there are no such refcounts, so it is no mistake.
> Just say that qemu-img check does not do much for external data files,
> and this is why this test does not work with them.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/qemu-iotests/138 | 41 ++++++++++++++++++++++++++++++++++++--
> tests/qemu-iotests/138.out | 14 +++++++++++++
> 2 files changed, 53 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qemu-iotests/138 b/tests/qemu-iotests/138
> index 54b01046ad..1d5b0bed6d 100755
> --- a/tests/qemu-iotests/138
> +++ b/tests/qemu-iotests/138
> @@ -41,8 +41,10 @@ _supported_fmt qcow2
> _supported_proto file
> _supported_os Linux
> # With an external data file, data clusters are not refcounted
> -# (and so qemu-img check does not check their refcount)
> -_unsupported_imgopts data_file
> +# (so qemu-img check would not do much);
Works for me.
> +echo
> +# Should print the number of corruptions and leaks fixed
> +# (Filter out all JSON fields (recognizable by their four-space
> +# indentation), but keep the "-fixed" fields (by removing two spaces
> +# from their indentation))
> +# (Also filter out the L1 entry, because why not)
> +_check_test_img -r all --output=json \
> + | sed -e 's/^ \(.*\)-fixed"/\1-fixed"/' \
> + -e '/^ /d' \
> + -e "s/\\([^0-9a-f]\\)$(printf %x $l1_entry)\\([^0-9a-f]\\)/\1L1_ENTRY_VALUE\2/"
It's fun to see the post-review results. Given the limited times where
the third -e fires in the output,
> +++ b/tests/qemu-iotests/138.out
> +Leaked cluster 0 refcount=2 reference=1
> +Repairing cluster 0 refcount=2 reference=1
> +Repairing OFLAG_COPIED L2 cluster: l1_index=0 l1_entry=L1_ENTRY_VALUE refcount=1
it could be written with less typing as:
-e "s/=$(printf %x $l1_entry) /=L1_ENTRY_VALUE /"
but that's not essential. So either way,
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH for-5.0 v2 0/3] qemu-img: Fix check's leak/corruption fix report
2020-03-24 17:27 [PATCH for-5.0 v2 0/3] qemu-img: Fix check's leak/corruption fix report Max Reitz
` (2 preceding siblings ...)
2020-03-24 17:27 ` [PATCH for-5.0 v2 3/3] iotests/138: Test leaks/corruptions fixed report Max Reitz
@ 2020-03-26 13:53 ` Max Reitz
3 siblings, 0 replies; 7+ messages in thread
From: Max Reitz @ 2020-03-26 13:53 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 327 bytes --]
On 24.03.20 18:27, Max Reitz wrote:
> Branch: https://github.com/XanClic/qemu.git fix-check-result-v2
> Branch: https://git.xanclic.moe/XanClic/qemu.git fix-check-result-v2
>
> v1: https://lists.nongnu.org/archive/html/qemu-block/2020-02/msg01418.html
Thanks for the review again, applied to my block branch.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread