[PATCH for-5.0 v2 0/3] qemu-img: Fix check's leak/corruption fix report

[PATCH for-5.0 v2 0/3] qemu-img: Fix check's leak/corruption fix report

[Max Reitz]

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


Hi,

While reviewing the “fix two small memleaks” series
(<20200227012950.12256-1-pannengyuan@huawei.com>) I noticed that we save
ImageCheck.(leaks|corruptions)_fixed from the first run and overwrite
the values obtained in the second run (where they must be zero because
we do not request any fixes in that second run), but we do not overwrite
ImageCheck.has_(leaks|corruptions)_fixed after the second run.  That
smells fishy.

Furthermore, ImageCheck.has_(leaks|corruptions)_fixed are not set based
on whether (leaks|corruptions)_fixed is non-zero, but actually based on
whether the leaks and corruptions fields (respectively) are non-zero.
qcow2’s check implementation reduces the leaks and corruptions values
when it fixes them, because then there are no leaks and corruptions
after the check anymore.

All in all, after a successful run, you will not see
“qemu-img check --output=json” report corruptions-fixed or leaks-fixed.
Which is a shame.  So this series fixes that and adds a test to ensure
those fields are indeed reported.


v2:
- Patch 2:
  - Rewrote the new functions as per Eric’s suggestions (thanks a lot!)
  - Changed the functions’ “doc strings” to not just show an example,
    but perhaps more importantly what each parameter means
- Patch 3:
  - Eric said the pre-existing comment explaining why 138 doesn’t
    supported data_file made it sound like qemu-img check had a bug.
    I’m sure it does have bugs, but this isn’t one, so I tried to
    explain it a bit differently.
  - Use poke_file_be in another place
  - Convert sed | grep | sed to a single sed, as per Eric’s suggestion,
    and avoid \< and \>


git-backport-diff against v1:

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/3:[----] [--] 'qemu-img: Fix check's leak/corruption fix report'
002/3:[0029] [FC] 'iotests: Add poke_file_[bl]e functions'
003/3:[0008] [FC] 'iotests/138: Test leaks/corruptions fixed report'


Max Reitz (3):
  qemu-img: Fix check's leak/corruption fix report
  iotests: Add poke_file_[bl]e functions
  iotests/138: Test leaks/corruptions fixed report

 qemu-img.c                   |  9 ++++++--
 tests/qemu-iotests/138       | 41 ++++++++++++++++++++++++++++++++++--
 tests/qemu-iotests/138.out   | 14 ++++++++++++
 tests/qemu-iotests/common.rc | 24 +++++++++++++++++++++
 4 files changed, 84 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH for-5.0 v2 1/3] qemu-img: Fix check's leak/corruption fix report

[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

[PATCH for-5.0 v2 2/3] iotests: Add poke_file_[bl]e functions

[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

[PATCH for-5.0 v2 3/3] iotests/138: Test leaks/corruptions fixed report

[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

Re: [PATCH for-5.0 v2 2/3] iotests: Add poke_file_[bl]e functions

[Eric Blake]

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

Re: [PATCH for-5.0 v2 3/3] iotests/138: Test leaks/corruptions fixed report

[Eric Blake]

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

Re: [PATCH for-5.0 v2 0/3] qemu-img: Fix check's leak/corruption fix report

[Max Reitz]

[-- 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 --]