[v1] iotests: Fix nonportable use of od --endian

[PATCH] iotests: Fix nonportable use of od --endian

Posted by Eric Blake 5 years, 11 months ago

Tests 261 and 272 fail on RHEL 7 with coreutils 8.22, since od
--endian was not added until coreutils 8.23.  Fix this by manually
constructing the final value one byte at a time.

Fixes: fc8ba423
Reported-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/qemu-iotests/common.rc | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 8a6366c09daf..b77ef3d22cd1 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -56,6 +56,12 @@ poke_file()
 # peek_file_le 'test.img' 512 2 => 65534
 peek_file_le()
 {
-    # Wrap in echo $() to strip spaces
-    echo $(od -j"$2" -N"$3" --endian=little -An -vtu"$3" "$1")
+    local val=0 shift=0 i
+
+    # coreutils' od --endian is not portable, so manually assemble bytes.
+    for i in $(od -j"$2" -N"$3" -An -v -tu1 "$1"); do
+        val=$(( val | (i << shift) ))
+        shift=$((shift + 8))
+    done
+    echo $val
 }

 # peek_file_be 'test.img' 512 2 => 65279
 peek_file_be()
 {
-    # Wrap in echo $() to strip spaces
-    echo $(od -j"$2" -N"$3" --endian=big -An -vtu"$3" "$1")
+    local val=0 i
+
+    # coreutils' od --endian is not portable, so manually assemble bytes.
+    for i in $(od -j"$2" -N"$3" -An -v -tu1 "$1"); do
+        val=$(( (val << 8) | i ))
+    done
+    echo $val
 }

-# peek_file_raw 'test.img' 512 2 => '\xff\xfe'
+# peek_file_raw 'test.img' 512 2 => '\xff\xfe'. Do not use if the raw data
+# is likely to contain \0 or trailing \n.
 peek_file_raw()
 {
     dd if="$1" bs=1 skip="$2" count="$3" status=none
-- 
2.24.1

Re: [PATCH] iotests: Fix nonportable use of od --endian

Posted by Max Reitz 5 years, 11 months ago

On 19.02.20 15:41, Eric Blake wrote:
> Tests 261 and 272 fail on RHEL 7 with coreutils 8.22, since od
> --endian was not added until coreutils 8.23.  Fix this by manually
> constructing the final value one byte at a time.
> 
> Fixes: fc8ba423
> Reported-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/qemu-iotests/common.rc | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 8a6366c09daf..b77ef3d22cd1 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -56,6 +56,12 @@ poke_file()

I don’t know how you did it, but something in your workflow broke this
patch.  This should be -56,18 +56,30.

(Note that git accepts this patch without error, but it drops everything
after the first part.  I noticed because your squash-in failed to apply.
 I suppose I might have noticed later when reviewing, because only
peek_file_le() was touched, but who knows.)

>  # peek_file_le 'test.img' 512 2 => 65534
>  peek_file_le()
>  {
> -    # Wrap in echo $() to strip spaces
> -    echo $(od -j"$2" -N"$3" --endian=little -An -vtu"$3" "$1")
> +    local val=0 shift=0 i
> +
> +    # coreutils' od --endian is not portable, so manually assemble bytes.
> +    for i in $(od -j"$2" -N"$3" -An -v -tu1 "$1"); do

“i” is a weird name for something that isn’t an index.

> +        val=$(( val | (i << shift) ))
> +        shift=$((shift + 8))
> +    done
> +    echo $val
>  }
> 

Regarding the broken patch format again: There are no spaces here...

>  # peek_file_be 'test.img' 512 2 => 65279
>  peek_file_be()
>  {
> -    # Wrap in echo $() to strip spaces
> -    echo $(od -j"$2" -N"$3" --endian=big -An -vtu"$3" "$1")
> +    local val=0 i
> +
> +    # coreutils' od --endian is not portable, so manually assemble bytes.
> +    for i in $(od -j"$2" -N"$3" -An -v -tu1 "$1"); do
> +        val=$(( (val << 8) | i ))
> +    done
> +    echo $val
>  }
> 

...and here like there normally would be.

With the patch format fixed, and your proposed to-be-squashed patch
squashed in, there isn’t anything wrong, so:

Reviewed-by: Max Reitz <mreitz@redhat.com>

But if I’m already about to squash something in, would you allow me to
rename all instances of “i” to e.g. “byte”?

Max

> -# peek_file_raw 'test.img' 512 2 => '\xff\xfe'
> +# peek_file_raw 'test.img' 512 2 => '\xff\xfe'. Do not use if the raw data
> +# is likely to contain \0 or trailing \n.
>  peek_file_raw()
>  {
>      dd if="$1" bs=1 skip="$2" count="$3" status=none
>

Re: [PATCH] iotests: Fix nonportable use of od --endian

Posted by Eric Blake 5 years, 11 months ago

On 2/25/20 11:43 AM, Max Reitz wrote:
> On 19.02.20 15:41, Eric Blake wrote:
>> Tests 261 and 272 fail on RHEL 7 with coreutils 8.22, since od
>> --endian was not added until coreutils 8.23.  Fix this by manually
>> constructing the final value one byte at a time.
>>
>> Fixes: fc8ba423
>> Reported-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>   tests/qemu-iotests/common.rc | 22 +++++++++++++++++-----
>>   1 file changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
>> index 8a6366c09daf..b77ef3d22cd1 100644
>> --- a/tests/qemu-iotests/common.rc
>> +++ b/tests/qemu-iotests/common.rc
>> @@ -56,6 +56,12 @@ poke_file()
> 
> I don’t know how you did it, but something in your workflow broke this
> patch.  This should be -56,18 +56,30.

I'm not sure, either.  Would me sending a v2 make it easier?

> 
> (Note that git accepts this patch without error, but it drops everything
> after the first part.  I noticed because your squash-in failed to apply.
>   I suppose I might have noticed later when reviewing, because only
> peek_file_le() was touched, but who knows.)
> 

> 
> With the patch format fixed, and your proposed to-be-squashed patch
> squashed in, there isn’t anything wrong, so:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
> But if I’m already about to squash something in, would you allow me to
> rename all instances of “i” to e.g. “byte”?

I'll do that on a v2.


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

Re: [PATCH] iotests: Fix nonportable use of od --endian

Posted by Eric Blake 5 years, 11 months ago

On 2/19/20 8:41 AM, Eric Blake wrote:
> Tests 261 and 272 fail on RHEL 7 with coreutils 8.22, since od
> --endian was not added until coreutils 8.23.  Fix this by manually
> constructing the final value one byte at a time.
> 
> Fixes: fc8ba423
> Reported-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   tests/qemu-iotests/common.rc | 22 +++++++++++++++++-----
>   1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 8a6366c09daf..b77ef3d22cd1 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -56,6 +56,12 @@ poke_file()
>   # peek_file_le 'test.img' 512 2 => 65534
>   peek_file_le()
>   {
> -    # Wrap in echo $() to strip spaces
> -    echo $(od -j"$2" -N"$3" --endian=little -An -vtu"$3" "$1")
> +    local val=0 shift=0 i
> +
> +    # coreutils' od --endian is not portable, so manually assemble bytes.
> +    for i in $(od -j"$2" -N"$3" -An -v -tu1 "$1"); do
> +        val=$(( val | (i << shift) ))
> +        shift=$((shift + 8))
> +    done
> +    echo $val

Reports large 8-byte values as negative, whereas the pre-patch version 
reported them as positive.  To preserve behavior, we should squash in:

diff --git i/tests/qemu-iotests/common.rc w/tests/qemu-iotests/common.rc
index 8966ad5cde78..a596856d4d8c 100644
--- i/tests/qemu-iotests/common.rc
+++ w/tests/qemu-iotests/common.rc
@@ -63,7 +63,7 @@ peek_file_le()
          val=$(( val | (i << shift) ))
          shift=$((shift + 8))
      done
-    echo $val
+    printf %llu $val
  }

  # peek_file_be 'test.img' 512 2 => 65279
@@ -75,7 +75,7 @@ peek_file_be()
      for i in $(od -j"$2" -N"$3" -An -v -tu1 "$1"); do
          val=$(( (val << 8) | i ))
      done
-    echo $val
+    printf %llu $val
  }

  # peek_file_raw 'test.img' 512 2 => '\xff\xfe'. Do not use if the raw data



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