On 12.03.20 23:19, John Snow wrote:
> 
> 
> On 3/11/20 10:07 AM, Max Reitz wrote:
>> data_file does not work with v2, and we probably want 026 to keep
>> working for v2 images.  Thus, open a new file for v3-exclusive error
>> path test cases.
>>
>> Fixes: 81311255f217859413c94f2cd9cebf2684bbda94
>>        (“iotests/026: Test EIO on allocation in a data-file”)
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
> Let me start this reply with something good, or at least something
> that's not bad. It's value neutral at worst.
> 
> Reviewed-by: John Snow <jsnow@redhat.com>
> Tested-by: John Snow <jsnow@redhat.com>

Thanks. :)

> Now, let's get cracking on some prime nonsense.
> 
> I assume this patch is still 'pending'. Here's a complete tangent
> unrelated to your patch in every single way:

Reasonable, but it’s a bit of a shame you bury it here.  I feel like
this makes it unlikely to reach the people you want to reach.

> What's the best way to use patchew to see series that are "pending" in
> some way? I'd like to:
> 
> - Search only the block list (to:qemu-block@nongnu.org. I assume this
> catches CCs too.)
> - Exclude series that are merged (-is:merged)
> - Exclude obsoleted series (-is:obsolete)
> 
> This gets a bit closer to things that are interesting in some way --
> give or take some fuzziness with patchew's detection of "merged" or
> "obsoleted" sometimes.
> 
> - Exclude pull requests. (-is:pull seems broken, actually.)
> - Exclude reviewed series (-is:reviewed -- what does patchew consider
> 'reviewed'? does this mean fully reviewed, or any reviews?)
> 
> This gives me something a bit more useful.
> 
> - Exclude 'expired' series. I use 30 days as a mental model for this. It
> might be nice to formalize this and mark patches that received no
> replies and didn't detect any other state change as "expired" and send
> an autoreply from the bot.
> 
> (I.e., patches that are complete, applied, passed CI, were not
> obsoleted, did not appear to be merged, and received no replies from
> anyone except the patch author)
> 
> 
> ("Hi, this patch received no replies from anyone except the author (you)
> for 30 days. The series is being dropped from the pending queue and is
> being marked expired. If the patches are still important, please rebase
> them and re-send to the list.
> 
> Please use scripts/get_maintainers.pl to identify candidate maintainers
> and reviewers and make sure they are CC'd.
> 
> This series appears to touch files owned by the following maintainers:
> - Blah
> - Etc
> - And so on
> 
> For more information on the contribution process, please visit:
> <wiki links to contribution guides, etc>")
> 
> We don't have anything like that, so age:<30d suffices. Alright, this
> list is starting to look *pretty* decent.
> 
> project:QEMU to:qemu-block@nongnu.org not:obsolete not:merged
> -is:reviewed age:<30d
> 
> Lastly, maybe we can exclude series that don't have replies yet.

Maybe Patchew should ping these after 14 days or so.

That’s about the only thing I can contribute, because I don’t really use
Patchew myself...  I keep patches in a subfolder of my inbox on unread;
and I keep my own pending series in a separate folder.  (Before I did
that, I was much more prone to forgetting my own patches rather than
someone else’s.)

Max

> It's
> not clear to patchew which replies are:
> 
> - Unrelated comments, like this one here
> - Requests for a change
> - A question for the submitter
> - A softly-worded N-A-C-K
> 
> and without a concept of designated reviewer, perhaps lack of replies is
> good evidence that the series is untouched and needs someone to 'pick it
> up'; (-has:replies)
> 
> https://patchew.org/search?q=project%3AQEMU+to%3Aqemu-block%40nongnu.org+not%3Aobsolete+not%3Amerged+-is%3Areviewed+age%3A%3C30d+-has%3Areplies
> 
> Alright, that's pretty good, actually.
> 
> OK, yes, this patch still needs love as far as patchew understands.
> 
>> ---
>>  tests/qemu-iotests/026             | 31 -----------
>>  tests/qemu-iotests/026.out         |  6 --
>>  tests/qemu-iotests/026.out.nocache |  6 --
>>  tests/qemu-iotests/289             | 89 ++++++++++++++++++++++++++++++
>>  tests/qemu-iotests/289.out         |  8 +++
>>  tests/qemu-iotests/group           |  1 +
>>  6 files changed, 98 insertions(+), 43 deletions(-)
>>  create mode 100755 tests/qemu-iotests/289
>>  create mode 100644 tests/qemu-iotests/289.out
>>
>> diff --git a/tests/qemu-iotests/026 b/tests/qemu-iotests/026
>> index b05a4692cf..b9713eb591 100755
>> --- a/tests/qemu-iotests/026
>> +++ b/tests/qemu-iotests/026
>> @@ -240,37 +240,6 @@ $QEMU_IO -c "write 0 $CLUSTER_SIZE" "$BLKDBG_TEST_IMG" | _filter_qemu_io
>>  
>>  _check_test_img
>>  
>> -echo
>> -echo === Avoid freeing external data clusters on failure ===
>> -echo
>> -
>> -# Similar test as the last one, except we test what happens when there
>> -# is an error when writing to an external data file instead of when
>> -# writing to a preallocated zero cluster
>> -_make_test_img -o "data_file=$TEST_IMG.data_file" $CLUSTER_SIZE
>> -
>> -# Put blkdebug above the data-file, and a raw node on top of that so
>> -# that blkdebug will see a write_aio event and emit an error
>> -$QEMU_IO -c "write 0 $CLUSTER_SIZE" \
>> -    "json:{
>> -         'driver': 'qcow2',
>> -         'file': { 'driver': 'file', 'filename': '$TEST_IMG' },
>> -         'data-file': {
>> -             'driver': 'raw',
>> -             'file': {
>> -                 'driver': 'blkdebug',
>> -                 'config': '$TEST_DIR/blkdebug.conf',
>> -                 'image': {
>> -                     'driver': 'file',
>> -                     'filename': '$TEST_IMG.data_file'
>> -                 }
>> -             }
>> -         }
>> -     }" \
>> -    | _filter_qemu_io
>> -
>> -_check_test_img
>> -
>>  # success, all done
>>  echo "*** done"
>>  rm -f $seq.full
>> diff --git a/tests/qemu-iotests/026.out b/tests/qemu-iotests/026.out
>> index c1b3b58482..83989996ff 100644
>> --- a/tests/qemu-iotests/026.out
>> +++ b/tests/qemu-iotests/026.out
>> @@ -653,10 +653,4 @@ wrote 1024/1024 bytes at offset 0
>>  1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>  write failed: Input/output error
>>  No errors were found on the image.
>> -
>> -=== Avoid freeing external data clusters on failure ===
>> -
>> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1024 data_file=TEST_DIR/t.IMGFMT.data_file
>> -write failed: Input/output error
>> -No errors were found on the image.
>>  *** done
>> diff --git a/tests/qemu-iotests/026.out.nocache b/tests/qemu-iotests/026.out.nocache
>> index 8d5001648a..9359d26d7e 100644
>> --- a/tests/qemu-iotests/026.out.nocache
>> +++ b/tests/qemu-iotests/026.out.nocache
>> @@ -661,10 +661,4 @@ wrote 1024/1024 bytes at offset 0
>>  1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>  write failed: Input/output error
>>  No errors were found on the image.
>> -
>> -=== Avoid freeing external data clusters on failure ===
>> -
>> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1024 data_file=TEST_DIR/t.IMGFMT.data_file
>> -write failed: Input/output error
>> -No errors were found on the image.
>>  *** done
>> diff --git a/tests/qemu-iotests/289 b/tests/qemu-iotests/289
>> new file mode 100755
>> index 0000000000..1c11d4030e
>> --- /dev/null
>> +++ b/tests/qemu-iotests/289
>> @@ -0,0 +1,89 @@
>> +#!/usr/bin/env bash
>> +#
>> +# qcow2 v3-exclusive error path testing
>> +# (026 tests paths common to v2 and v3)
>> +#
>> +# Copyright (C) 2020 Red Hat, Inc.
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 2 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +#
>> +
>> +seq=$(basename $0)
>> +echo "QA output created by $seq"
>> +
>> +status=1	# failure is the default!
>> +
>> +_cleanup()
>> +{
>> +    _cleanup_test_img
>> +    rm "$TEST_DIR/blkdebug.conf"
>> +    rm -f "$TEST_IMG.data_file"
>> +}
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +# get standard environment, filters and checks
>> +. ./common.rc
>> +. ./common.filter
>> +. ./common.pattern
>> +
>> +_supported_fmt qcow2
>> +_supported_proto file
>> +# This is a v3-exclusive test;
>> +# As for data_file, error paths often very much depend on whether
>> +# there is an external data file or not; so we create one exactly when
>> +# we want to test it
>> +_unsupported_imgopts 'compat=0.10' data_file
>> +
>> +echo
>> +echo === Avoid freeing external data clusters on failure ===
>> +echo
>> +
>> +cat > "$TEST_DIR/blkdebug.conf" <<EOF
>> +[inject-error]
>> +event = "write_aio"
>> +errno = "5"
>> +once = "on"
>> +EOF
>> +
>> +# Test what happens when there is an error when writing to an external
>> +# data file instead of when writing to a preallocated zero cluster
>> +_make_test_img -o "data_file=$TEST_IMG.data_file" 64k
>> +
>> +# Put blkdebug above the data-file, and a raw node on top of that so
>> +# that blkdebug will see a write_aio event and emit an error.  This
>> +# will then trigger the alloc abort code, which we want to test here.
>> +$QEMU_IO -c "write 0 64k" \
>> +    "json:{
>> +         'driver': 'qcow2',
>> +         'file': { 'driver': 'file', 'filename': '$TEST_IMG' },
>> +         'data-file': {
>> +             'driver': 'raw',
>> +             'file': {
>> +                 'driver': 'blkdebug',
>> +                 'config': '$TEST_DIR/blkdebug.conf',
>> +                 'image': {
>> +                     'driver': 'file',
>> +                     'filename': '$TEST_IMG.data_file'
>> +                 }
>> +             }
>> +         }
>> +     }" \
>> +    | _filter_qemu_io
>> +
>> +_check_test_img
>> +
>> +# success, all done
>> +echo "*** done"
>> +rm -f $seq.full
>> +status=0
>> diff --git a/tests/qemu-iotests/289.out b/tests/qemu-iotests/289.out
>> new file mode 100644
>> index 0000000000..e54e2629d4
>> --- /dev/null
>> +++ b/tests/qemu-iotests/289.out
>> @@ -0,0 +1,8 @@
>> +QA output created by 289
>> +
>> +=== Avoid freeing external data clusters on failure ===
>> +
>> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536 data_file=TEST_DIR/t.IMGFMT.data_file
>> +write failed: Input/output error
>> +No errors were found on the image.
>> +*** done
>> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
>> index 559edc139a..a898fe2c26 100644
>> --- a/tests/qemu-iotests/group
>> +++ b/tests/qemu-iotests/group
>> @@ -294,3 +294,4 @@
>>  284 rw
>>  286 rw quick
>>  288 quick
>> +289 rw quick
>>
>