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

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

[Max Reitz]

(Cc-ing Eric because of patch 2, mostly)

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.


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 | 37 ++++++++++++++++++++++++++++++++
 4 files changed, 97 insertions(+), 4 deletions(-)

-- 
2.24.1

[PATCH 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>
---
 qemu-img.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 7b7087dd60..c7567e1979 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.24.1

[PATCH 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>
---
 tests/qemu-iotests/common.rc | 37 ++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 4c246c0450..604f837668 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -53,6 +53,43 @@ poke_file()
     printf "$3" | dd "of=$1" bs=1 "seek=$2" conv=notrunc &>/dev/null
 }
 
+# poke_file_le 'test.img' 512 2 65534
+poke_file_le()
+{
+    local img=$1 ofs=$2 len=$3 val=$4 str=''
+
+    for i in $(seq 0 $((len - 1))); do
+        byte=$((val & 0xff))
+        if [ $byte != 0 ]; then
+            chr="$(printf "\x$(printf %x $byte)")"
+        else
+            chr="\0"
+        fi
+        str+="$chr"
+        val=$((val >> 8))
+    done
+
+    poke_file "$img" "$ofs" "$str"
+}
+
+# poke_file_be 'test.img' 512 2 65279
+poke_file_be()
+{
+    local img=$1 ofs=$2 len=$3 val=$4 str=''
+
+    for i in $(seq 0 $((len - 1))); do
+        byte=$(((val >> ((len - 1 - i) * 8)) & 0xff))
+        if [ $byte != 0 ]; then
+            chr="$(printf "\x$(printf %x $byte)")"
+        else
+            chr="\0"
+        fi
+        str+=$chr
+    done
+
+    poke_file "$img" "$ofs" "$str"
+}
+
 # peek_file_le 'test.img' 512 2 => 65534
 peek_file_le()
 {
-- 
2.24.1

[PATCH 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).

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..25bfbd4cca 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
+# (and so qemu-img check does not check their refcount);
+# 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 "$TEST_IMG" "$refblock_ofs" "\x00\x02"
+
+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"/' \
+    | grep -v '^    ' \
+    | sed -e "s/\\<$(printf %x $l1_entry)\\>/L1_ENTRY_VALUE/"
+
 # 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.24.1

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

[Eric Blake]

On 2/27/20 11:02 AM, Max Reitz wrote:
> 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>
> ---
>   qemu-img.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 

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 2/3] iotests: Add poke_file_[bl]e functions

[Eric Blake]

On 2/27/20 11:02 AM, 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>
> ---
>   tests/qemu-iotests/common.rc | 37 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 37 insertions(+)
> 
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 4c246c0450..604f837668 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -53,6 +53,43 @@ poke_file()
>       printf "$3" | dd "of=$1" bs=1 "seek=$2" conv=notrunc &>/dev/null
>   }
>   
> +# poke_file_le 'test.img' 512 2 65534
> +poke_file_le()
> +{

I like the interface.  However, the implementation is a bit bloated (but 
then again, that's why you cc'd me for review ;)

> +    local img=$1 ofs=$2 len=$3 val=$4 str=''
> +
> +    for i in $(seq 0 $((len - 1))); do

No need to fork seq, when we can let bash do the iteration for us:

while ((len--)); do

> +        byte=$((val & 0xff))
> +        if [ $byte != 0 ]; then
> +            chr="$(printf "\x$(printf %x $byte)")"

Why are we doing two printf command substitutions instead of 1?

> +        else
> +            chr="\0"

Why do we have to special-case 0?  printf '\x00' does the right thing.

> +        fi
> +        str+="$chr"

I'd go with the faster str+=$(printf '\\x%02x' $((val & 0xff))), 
completely skipping the byte and chr variables.

> +        val=$((val >> 8))
> +    done
> +
> +    poke_file "$img" "$ofs" "$str"
> +}

So my version:

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 'test.img' 512 2 65279
> +poke_file_be()
> +{
> +    local img=$1 ofs=$2 len=$3 val=$4 str=''

And this one's even easier: we get big-endian for free from printf 
output, with a sed post-processing to add \x:

poke_file_be()
{
     local str="$(printf "%0$(($3 * 2))x\n" $4 | sed 's/\(..\)/\\x\1/g')"
     poke_file "$1" "$2" "$str"
}

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

Re: [PATCH 3/3] iotests/138: Test leaks/corruptions fixed report

[Eric Blake]

On 2/27/20 11:02 AM, Max Reitz wrote:
> Test that qemu-img check reports the number of leaks and corruptions
> fixed in its JSON report (after a successful run).
> 
> 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..25bfbd4cca 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
> +# (and so qemu-img check does not check their refcount);

Not this patch's problem, but is that a bug in 'qemu-img check' for not 
validating refcounts on an external data file?  Or is it merely this 
comment wording is not quite perfect?

> +# 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 "$TEST_IMG" "$refblock_ofs" "\x00\x02"

Why not use your brand-new 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

Yep, the new function makes this task easier.  (You could also just peek 
1 byte at $((l1_ofs+7)) then write it back out with poke_file 
"$TEST_IMG" $((l1_ofs + 7)) $(printf '\\x%02x' $((val & 0xfe)))", but 
that just doesn't look as nice)

> +
> +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"/' \
> +    | grep -v '^    ' \
> +    | sed -e "s/\\<$(printf %x $l1_entry)\\>/L1_ENTRY_VALUE/"

sed | grep | sed can often be done with a single sed:

... | sed -e 's/^  \(.*\)-fixed"/\1-fixed"/' \
        -e '/^    /d' \
        -e "s/\\..."

Using \\< and \\> in the sed regex is a GNUism; do we want this test to 
run on BSD?

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 2/3] iotests: Add poke_file_[bl]e functions

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2843 bytes --]

On 27.02.20 19:46, Eric Blake wrote:
> On 2/27/20 11:02 AM, 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>
>> ---
>>   tests/qemu-iotests/common.rc | 37 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
>> index 4c246c0450..604f837668 100644
>> --- a/tests/qemu-iotests/common.rc
>> +++ b/tests/qemu-iotests/common.rc
>> @@ -53,6 +53,43 @@ poke_file()
>>       printf "$3" | dd "of=$1" bs=1 "seek=$2" conv=notrunc &>/dev/null
>>   }
>>   +# poke_file_le 'test.img' 512 2 65534
>> +poke_file_le()
>> +{
> 
> I like the interface.  However, the implementation is a bit bloated (but
> then again, that's why you cc'd me for review ;)
> 
>> +    local img=$1 ofs=$2 len=$3 val=$4 str=''
>> +
>> +    for i in $(seq 0 $((len - 1))); do
> 
> No need to fork seq, when we can let bash do the iteration for us:
> 
> while ((len--)); do
> 
>> +        byte=$((val & 0xff))
>> +        if [ $byte != 0 ]; then
>> +            chr="$(printf "\x$(printf %x $byte)")"
> 
> Why are we doing two printf command substitutions instead of 1?

Because I had no idea that $() would evaluate '\x*' escape sequences.
Interesting.

>> +        else
>> +            chr="\0"
> 
> Why do we have to special-case 0?  printf '\x00' does the right thing.

The non-special-cased version didn’t seem to work for NUL.

>> +        fi
>> +        str+="$chr"
> 
> I'd go with the faster str+=$(printf '\\x%02x' $((val & 0xff))),
> completely skipping the byte and chr variables.

Sure!  That’s much better.

>> +        val=$((val >> 8))
>> +    done
>> +
>> +    poke_file "$img" "$ofs" "$str"
>> +}
> 
> So my version:
> 
> 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"
> }

Much better indeed.

>> +
>> +# poke_file_be 'test.img' 512 2 65279
>> +poke_file_be()
>> +{
>> +    local img=$1 ofs=$2 len=$3 val=$4 str=''
> 
> And this one's even easier: we get big-endian for free from printf
> output, with a sed post-processing to add \x:
> 
> poke_file_be()
> {
>     local str="$(printf "%0$(($3 * 2))x\n" $4 | sed 's/\(..\)/\\x\1/g')"
>     poke_file "$1" "$2" "$str"
> }

Thanks.  I knew I could count on you. :)

Max


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

Re: [PATCH 3/3] iotests/138: Test leaks/corruptions fixed report

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 4233 bytes --]

On 27.02.20 19:56, Eric Blake wrote:
> On 2/27/20 11:02 AM, Max Reitz wrote:
>> Test that qemu-img check reports the number of leaks and corruptions
>> fixed in its JSON report (after a successful run).
>>
>> 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..25bfbd4cca 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
>> +# (and so qemu-img check does not check their refcount);
> 
> Not this patch's problem, but is that a bug in 'qemu-img check' for not
> validating refcounts on an external data file?  Or is it merely this
> comment wording is not quite perfect?

There are no refcounts for an external data file, because every cluster
is refcounted exactly once and we don’t need refcounts to allocated
clusters (the offset in the data file is the same as the guest offset in
the image).

It kind of is what the comment says, but I suppose we could drop the
part about qemu-img check?

>> +# 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 "$TEST_IMG" "$refblock_ofs" "\x00\x02"
> 
> Why not use your brand-new poke_file_be "$TEST_IMG" "$refblock_ofs" 2 2

Because I didn’t need it at this point.  I only needed it for the next
line, so I wrote it in between. :)

But yes, it does make sense to use it wherever possible now that we have it.

>> +
>> +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
> 
> Yep, the new function makes this task easier.  (You could also just peek
> 1 byte at $((l1_ofs+7)) then write it back out with poke_file
> "$TEST_IMG" $((l1_ofs + 7)) $(printf '\\x%02x' $((val & 0xfe)))", but
> that just doesn't look as nice)
> 
>> +
>> +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"/' \
>> +    | grep -v '^    ' \
>> +    | sed -e "s/\\<$(printf %x $l1_entry)\\>/L1_ENTRY_VALUE/"
> 
> sed | grep | sed can often be done with a single sed:
> 
> ... | sed -e 's/^  \(.*\)-fixed"/\1-fixed"/' \
>        -e '/^    /d' \
>        -e "s/\\..."

Nice.

> Using \\< and \\> in the sed regex is a GNUism; do we want this test to
> run on BSD?

Hm.  I suppose we can just use [^0-9a-f] instead.

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

Thanks!

Max


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

Re: [PATCH 2/3] iotests: Add poke_file_[bl]e functions

[Eric Blake]

On 3/10/20 12:22 PM, Max Reitz wrote:
>>>    +# poke_file_le 'test.img' 512 2 65534
>>> +poke_file_le()
>>> +{
>>
>> I like the interface.  However, the implementation is a bit bloated (but
>> then again, that's why you cc'd me for review ;)
>>
>>> +    local img=$1 ofs=$2 len=$3 val=$4 str=''

Noticing that this is not in yet, I have one more suggestion:

The initial doc comment is not helpful without reading the rest of the 
function: Is 512 the offset or the value being written?  Better might be:

# poke_file_le test.img $offset $width $value


>>> +
>>> +# poke_file_be 'test.img' 512 2 65279
>>> +poke_file_be()

and here, too.

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