[Qemu-devel] [PATCH] iotests: Filter 175's allocation information

Max Reitz posted 1 patch 4 years, 10 months ago
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test checkpatch passed
Test asan passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190510211954.29574-1-mreitz@redhat.com
Maintainers: Kevin Wolf <kwolf@redhat.com>, Max Reitz <mreitz@redhat.com>
There is a newer version of this series
tests/qemu-iotests/175     | 15 +++++++++++----
tests/qemu-iotests/175.out |  8 ++++----
2 files changed, 15 insertions(+), 8 deletions(-)
[Qemu-devel] [PATCH] iotests: Filter 175's allocation information
Posted by Max Reitz 4 years, 10 months ago
It is possible for an empty file to take up blocks on a filesystem.
Make iotest 175 take this into account.

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/175     | 15 +++++++++++----
 tests/qemu-iotests/175.out |  8 ++++----
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/tests/qemu-iotests/175 b/tests/qemu-iotests/175
index d0ffc495c2..b5652a3889 100755
--- a/tests/qemu-iotests/175
+++ b/tests/qemu-iotests/175
@@ -28,7 +28,8 @@ status=1	# failure is the default!
 
 _cleanup()
 {
-	_cleanup_test_img
+    _cleanup_test_img
+    rm -f "$TEST_DIR/empty"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
@@ -40,18 +41,24 @@ _supported_fmt raw
 _supported_proto file
 _supported_os Linux
 
-size=1m
+size=$((1 * 1024 * 1024))
+
+touch "$TEST_DIR/empty"
+empty_blocks=$(stat -c '%b' "$TEST_DIR/empty")
 
 echo
 echo "== creating image with default preallocation =="
 _make_test_img $size | _filter_imgfmt
-stat -c "size=%s, blocks=%b" $TEST_IMG
+stat -c "size=%s, blocks=%b" $TEST_IMG \
+    | sed -e "s/blocks=$empty_blocks/nothing allocated/"
 
 for mode in off full falloc; do
     echo
     echo "== creating image with preallocation $mode =="
     IMGOPTS=preallocation=$mode _make_test_img $size | _filter_imgfmt
-    stat -c "size=%s, blocks=%b" $TEST_IMG
+    stat -c "size=%s, blocks=%b" $TEST_IMG \
+        | sed -e "s/blocks=$empty_blocks/nothing allocated/" \
+        | sed -e "s/blocks=$((empty_blocks + size / 512))/everything allocated/"
 done
 
 # success, all done
diff --git a/tests/qemu-iotests/175.out b/tests/qemu-iotests/175.out
index 76c02c6a57..6d9a5ed84e 100644
--- a/tests/qemu-iotests/175.out
+++ b/tests/qemu-iotests/175.out
@@ -2,17 +2,17 @@ QA output created by 175
 
 == creating image with default preallocation ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
-size=1048576, blocks=0
+size=1048576, nothing allocated
 
 == creating image with preallocation off ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 preallocation=off
-size=1048576, blocks=0
+size=1048576, nothing allocated
 
 == creating image with preallocation full ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 preallocation=full
-size=1048576, blocks=2048
+size=1048576, everything allocated
 
 == creating image with preallocation falloc ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 preallocation=falloc
-size=1048576, blocks=2048
+size=1048576, everything allocated
  *** done
-- 
2.21.0


Re: [Qemu-devel] [PATCH] iotests: Filter 175's allocation information
Posted by Nir Soffer 4 years, 10 months ago
On Sat, May 11, 2019 at 12:19 AM Max Reitz <mreitz@redhat.com> wrote:

> It is possible for an empty file to take up blocks on a filesystem.
> Make iotest 175 take this into account.
>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/175     | 15 +++++++++++----
>  tests/qemu-iotests/175.out |  8 ++++----
>  2 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/tests/qemu-iotests/175 b/tests/qemu-iotests/175
> index d0ffc495c2..b5652a3889 100755
> --- a/tests/qemu-iotests/175
> +++ b/tests/qemu-iotests/175
> @@ -28,7 +28,8 @@ status=1      # failure is the default!
>
>  _cleanup()
>  {
> -       _cleanup_test_img
> +    _cleanup_test_img
> +    rm -f "$TEST_DIR/empty"
>  }
>  trap "_cleanup; exit \$status" 0 1 2 3 15
>
> @@ -40,18 +41,24 @@ _supported_fmt raw
>  _supported_proto file
>  _supported_os Linux
>
> -size=1m
> +size=$((1 * 1024 * 1024))

+
> +touch "$TEST_DIR/empty"
> +empty_blocks=$(stat -c '%b' "$TEST_DIR/empty")
>

Maybe extra_blocks?

 echo
>  echo "== creating image with default preallocation =="
>  _make_test_img $size | _filter_imgfmt
> -stat -c "size=%s, blocks=%b" $TEST_IMG
> +stat -c "size=%s, blocks=%b" $TEST_IMG \
> +    | sed -e "s/blocks=$empty_blocks/nothing allocated/"
>
>  for mode in off full falloc; do
>      echo
>      echo "== creating image with preallocation $mode =="
>      IMGOPTS=preallocation=$mode _make_test_img $size | _filter_imgfmt
> -    stat -c "size=%s, blocks=%b" $TEST_IMG
> +    stat -c "size=%s, blocks=%b" $TEST_IMG \
> +        | sed -e "s/blocks=$empty_blocks/nothing allocated/" \
> +        | sed -e "s/blocks=$((empty_blocks + size / 512))/everything
> allocated/"
>

"fully allocated"?

Maybe add a helper like this:

_filter_blocks() {
        # Some file systems sometimes allocate extra blocks
        sed -e "s/blocks=$empty_blocks/nothing allocated/" \
               -e "s/blocks=$((empty_blocks + size / 512))/everything
allocated/"
}

So we can do:

    stat -c "size=%s, blocks=%b" $TEST_IMG | _filter_blocks

And it is also clear why we need to run sed without looking up the commit
message.


>  done
>
>  # success, all done
> diff --git a/tests/qemu-iotests/175.out b/tests/qemu-iotests/175.out
> index 76c02c6a57..6d9a5ed84e 100644
> --- a/tests/qemu-iotests/175.out
> +++ b/tests/qemu-iotests/175.out
> @@ -2,17 +2,17 @@ QA output created by 175
>
>  == creating image with default preallocation ==
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
> -size=1048576, blocks=0
> +size=1048576, nothing allocated
>
>  == creating image with preallocation off ==
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 preallocation=off
> -size=1048576, blocks=0
> +size=1048576, nothing allocated
>
>  == creating image with preallocation full ==
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 preallocation=full
> -size=1048576, blocks=2048
> +size=1048576, everything allocated
>
>  == creating image with preallocation falloc ==
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
> preallocation=falloc
> -size=1048576, blocks=2048
> +size=1048576, everything allocated
>   *** done
> --
> 2.21.0
>

Otherwise looks good.

Nir
Re: [Qemu-devel] [PATCH] iotests: Filter 175's allocation information
Posted by Max Reitz 4 years, 10 months ago
On 10.05.19 23:45, Nir Soffer wrote:
> On Sat, May 11, 2019 at 12:19 AM Max Reitz <mreitz@redhat.com
> <mailto:mreitz@redhat.com>> wrote:
> 
>     It is possible for an empty file to take up blocks on a filesystem.
>     Make iotest 175 take this into account.
> 
>     Reported-by: Thomas Huth <thuth@redhat.com <mailto:thuth@redhat.com>>
>     Signed-off-by: Max Reitz <mreitz@redhat.com <mailto:mreitz@redhat.com>>
>     ---
>      tests/qemu-iotests/175     | 15 +++++++++++----
>      tests/qemu-iotests/175.out |  8 ++++----
>      2 files changed, 15 insertions(+), 8 deletions(-)
> 
>     diff --git a/tests/qemu-iotests/175 b/tests/qemu-iotests/175
>     index d0ffc495c2..b5652a3889 100755
>     --- a/tests/qemu-iotests/175
>     +++ b/tests/qemu-iotests/175
>     @@ -28,7 +28,8 @@ status=1      # failure is the default!
> 
>      _cleanup()
>      {
>     -       _cleanup_test_img
>     +    _cleanup_test_img
>     +    rm -f "$TEST_DIR/empty"
>      }
>      trap "_cleanup; exit \$status" 0 1 2 3 15
> 
>     @@ -40,18 +41,24 @@ _supported_fmt raw
>      _supported_proto file
>      _supported_os Linux
> 
>     -size=1m
>     +size=$((1 * 1024 * 1024))
> 
>     +
>     +touch "$TEST_DIR/empty"
>     +empty_blocks=$(stat -c '%b' "$TEST_DIR/empty")
> 
> 
> Maybe extra_blocks?

Why not.

>      echo
>      echo "== creating image with default preallocation =="
>      _make_test_img $size | _filter_imgfmt
>     -stat -c "size=%s, blocks=%b" $TEST_IMG
>     +stat -c "size=%s, blocks=%b" $TEST_IMG \
>     +    | sed -e "s/blocks=$empty_blocks/nothing allocated/"
> 
>      for mode in off full falloc; do
>          echo
>          echo "== creating image with preallocation $mode =="
>          IMGOPTS=preallocation=$mode _make_test_img $size | _filter_imgfmt
>     -    stat -c "size=%s, blocks=%b" $TEST_IMG
>     +    stat -c "size=%s, blocks=%b" $TEST_IMG \
>     +        | sed -e "s/blocks=$empty_blocks/nothing allocated/" \
>     +        | sed -e "s/blocks=$((empty_blocks + size /
>     512))/everything allocated/"
> 
> 
> "fully allocated"?

I didn’t like that because that sounds like it only applies to
preallocation=full.

> Maybe add a helper like this:
> 
> _filter_blocks() {
>         # Some file systems sometimes allocate extra blocks
>         sed -e "s/blocks=$empty_blocks/nothing allocated/" \
>                -e "s/blocks=$((empty_blocks + size / 512))/everything
> allocated/"
> }
> 
> So we can do:
> 
>     stat -c "size=%s, blocks=%b" $TEST_IMG | _filter_blocks
> 
> And it is also clear why we need to run sed without looking up the
> commit message.

Makes sense to me, but I find it a bit awkward to make a filter rely on
a data value determined outside of the filter...  I’ll see what I can do
to calm my conscience.

Max

Re: [Qemu-devel] [PATCH] iotests: Filter 175's allocation information
Posted by John Snow 4 years, 10 months ago

On 5/13/19 9:20 AM, Max Reitz wrote:
> On 10.05.19 23:45, Nir Soffer wrote:
>> On Sat, May 11, 2019 at 12:19 AM Max Reitz <mreitz@redhat.com
>> <mailto:mreitz@redhat.com>> wrote:
>>
>>     It is possible for an empty file to take up blocks on a filesystem.
>>     Make iotest 175 take this into account.
>>
>>     Reported-by: Thomas Huth <thuth@redhat.com <mailto:thuth@redhat.com>>
>>     Signed-off-by: Max Reitz <mreitz@redhat.com <mailto:mreitz@redhat.com>>
>>     ---
>>      tests/qemu-iotests/175     | 15 +++++++++++----
>>      tests/qemu-iotests/175.out |  8 ++++----
>>      2 files changed, 15 insertions(+), 8 deletions(-)
>>
>>     diff --git a/tests/qemu-iotests/175 b/tests/qemu-iotests/175
>>     index d0ffc495c2..b5652a3889 100755
>>     --- a/tests/qemu-iotests/175
>>     +++ b/tests/qemu-iotests/175
>>     @@ -28,7 +28,8 @@ status=1      # failure is the default!
>>
>>      _cleanup()
>>      {
>>     -       _cleanup_test_img
>>     +    _cleanup_test_img
>>     +    rm -f "$TEST_DIR/empty"
>>      }
>>      trap "_cleanup; exit \$status" 0 1 2 3 15
>>
>>     @@ -40,18 +41,24 @@ _supported_fmt raw
>>      _supported_proto file
>>      _supported_os Linux
>>
>>     -size=1m
>>     +size=$((1 * 1024 * 1024))
>>
>>     +
>>     +touch "$TEST_DIR/empty"
>>     +empty_blocks=$(stat -c '%b' "$TEST_DIR/empty")
>>
>>
>> Maybe extra_blocks?
> 
> Why not.
> 
>>      echo
>>      echo "== creating image with default preallocation =="
>>      _make_test_img $size | _filter_imgfmt
>>     -stat -c "size=%s, blocks=%b" $TEST_IMG
>>     +stat -c "size=%s, blocks=%b" $TEST_IMG \
>>     +    | sed -e "s/blocks=$empty_blocks/nothing allocated/"
>>
>>      for mode in off full falloc; do
>>          echo
>>          echo "== creating image with preallocation $mode =="
>>          IMGOPTS=preallocation=$mode _make_test_img $size | _filter_imgfmt
>>     -    stat -c "size=%s, blocks=%b" $TEST_IMG
>>     +    stat -c "size=%s, blocks=%b" $TEST_IMG \
>>     +        | sed -e "s/blocks=$empty_blocks/nothing allocated/" \
>>     +        | sed -e "s/blocks=$((empty_blocks + size /
>>     512))/everything allocated/"
>>
>>
>> "fully allocated"?
> 
> I didn’t like that because that sounds like it only applies to
> preallocation=full.
> 
>> Maybe add a helper like this:
>>
>> _filter_blocks() {
>>         # Some file systems sometimes allocate extra blocks
>>         sed -e "s/blocks=$empty_blocks/nothing allocated/" \
>>                -e "s/blocks=$((empty_blocks + size / 512))/everything
>> allocated/"
>> }
>>
>> So we can do:
>>
>>     stat -c "size=%s, blocks=%b" $TEST_IMG | _filter_blocks
>>
>> And it is also clear why we need to run sed without looking up the
>> commit message.
> 
> Makes sense to me, but I find it a bit awkward to make a filter rely on
> a data value determined outside of the filter...  I’ll see what I can do
> to calm my conscience.
> 
> Max
> 

You can always parameterize the filter so that the relationship is
explicit, no? Does that still feel icky?

--js

Re: [Qemu-devel] [PATCH] iotests: Filter 175's allocation information
Posted by Max Reitz 4 years, 10 months ago
On 13.05.19 17:33, John Snow wrote:
> 
> 
> On 5/13/19 9:20 AM, Max Reitz wrote:
>> On 10.05.19 23:45, Nir Soffer wrote:
>>> On Sat, May 11, 2019 at 12:19 AM Max Reitz <mreitz@redhat.com
>>> <mailto:mreitz@redhat.com>> wrote:
>>>
>>>     It is possible for an empty file to take up blocks on a filesystem.
>>>     Make iotest 175 take this into account.
>>>
>>>     Reported-by: Thomas Huth <thuth@redhat.com <mailto:thuth@redhat.com>>
>>>     Signed-off-by: Max Reitz <mreitz@redhat.com <mailto:mreitz@redhat.com>>
>>>     ---
>>>      tests/qemu-iotests/175     | 15 +++++++++++----
>>>      tests/qemu-iotests/175.out |  8 ++++----
>>>      2 files changed, 15 insertions(+), 8 deletions(-)
>>>
>>>     diff --git a/tests/qemu-iotests/175 b/tests/qemu-iotests/175
>>>     index d0ffc495c2..b5652a3889 100755
>>>     --- a/tests/qemu-iotests/175
>>>     +++ b/tests/qemu-iotests/175
>>>     @@ -28,7 +28,8 @@ status=1      # failure is the default!
>>>
>>>      _cleanup()
>>>      {
>>>     -       _cleanup_test_img
>>>     +    _cleanup_test_img
>>>     +    rm -f "$TEST_DIR/empty"
>>>      }
>>>      trap "_cleanup; exit \$status" 0 1 2 3 15
>>>
>>>     @@ -40,18 +41,24 @@ _supported_fmt raw
>>>      _supported_proto file
>>>      _supported_os Linux
>>>
>>>     -size=1m
>>>     +size=$((1 * 1024 * 1024))
>>>
>>>     +
>>>     +touch "$TEST_DIR/empty"
>>>     +empty_blocks=$(stat -c '%b' "$TEST_DIR/empty")
>>>
>>>
>>> Maybe extra_blocks?
>>
>> Why not.
>>
>>>      echo
>>>      echo "== creating image with default preallocation =="
>>>      _make_test_img $size | _filter_imgfmt
>>>     -stat -c "size=%s, blocks=%b" $TEST_IMG
>>>     +stat -c "size=%s, blocks=%b" $TEST_IMG \
>>>     +    | sed -e "s/blocks=$empty_blocks/nothing allocated/"
>>>
>>>      for mode in off full falloc; do
>>>          echo
>>>          echo "== creating image with preallocation $mode =="
>>>          IMGOPTS=preallocation=$mode _make_test_img $size | _filter_imgfmt
>>>     -    stat -c "size=%s, blocks=%b" $TEST_IMG
>>>     +    stat -c "size=%s, blocks=%b" $TEST_IMG \
>>>     +        | sed -e "s/blocks=$empty_blocks/nothing allocated/" \
>>>     +        | sed -e "s/blocks=$((empty_blocks + size /
>>>     512))/everything allocated/"
>>>
>>>
>>> "fully allocated"?
>>
>> I didn’t like that because that sounds like it only applies to
>> preallocation=full.
>>
>>> Maybe add a helper like this:
>>>
>>> _filter_blocks() {
>>>         # Some file systems sometimes allocate extra blocks
>>>         sed -e "s/blocks=$empty_blocks/nothing allocated/" \
>>>                -e "s/blocks=$((empty_blocks + size / 512))/everything
>>> allocated/"
>>> }
>>>
>>> So we can do:
>>>
>>>     stat -c "size=%s, blocks=%b" $TEST_IMG | _filter_blocks
>>>
>>> And it is also clear why we need to run sed without looking up the
>>> commit message.
>>
>> Makes sense to me, but I find it a bit awkward to make a filter rely on
>> a data value determined outside of the filter...  I’ll see what I can do
>> to calm my conscience.
>>
>> Max
>>
> 
> You can always parameterize the filter so that the relationship is
> explicit, no? Does that still feel icky?

Hmmm. :-)  Sounds good, thanks.

I was thinking of just a comment.  “Needs variable $foo set.”  But a
parameter works nicely, yes.

Max