qemu-img: Fix check's leak/corruption fix report

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

Posted by Max Reitz 5 years, 8 months ago

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

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

Posted by Eric Blake 5 years, 8 months ago

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

Posted by Max Reitz 5 years, 8 months ago

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

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

Posted by Eric Blake 5 years, 7 months ago

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