| Message ID | 20171109203025.27493-5-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | iotests: Make some tests less flaky | expand |
On 11/09/2017 02:30 PM, Max Reitz wrote: > 083 has (at least) two issues: > > 1. By launching the nbd-fault-injector in background, it may not be > scheduled until the first grep on its output file is executed. > However, until then, that file may not have been created yet -- so it > either does not exist yet (thus making the grep emit an error), or it > does exist but contains stale data (thus making the rest of the test > case work connect to a wrong address). > Fix this by explicitly overwriting the output file before executing > nbd-fault-injector. > > 2. The nbd-fault-injector prints things other than "Listening on...". > It also prints a "Closing connection" message from time to time. We > currently invoke sed on the whole file in the hope of it only > containing the "Listening on..." line yet. That hope is sometimes > shattered by the brutal reality of race conditions, so invoke grep > before sed. Comment is now stale; s/invoke grep before sed/make the sed script more robust/ Reviewed-by: Eric Blake <eblake@redhat.com>
On 2017-11-09 21:58, Eric Blake wrote: > On 11/09/2017 02:30 PM, Max Reitz wrote: >> 083 has (at least) two issues: >> >> 1. By launching the nbd-fault-injector in background, it may not be >> scheduled until the first grep on its output file is executed. >> However, until then, that file may not have been created yet -- so it >> either does not exist yet (thus making the grep emit an error), or it >> does exist but contains stale data (thus making the rest of the test >> case work connect to a wrong address). >> Fix this by explicitly overwriting the output file before executing >> nbd-fault-injector. >> >> 2. The nbd-fault-injector prints things other than "Listening on...". >> It also prints a "Closing connection" message from time to time. We >> currently invoke sed on the whole file in the hope of it only >> containing the "Listening on..." line yet. That hope is sometimes >> shattered by the brutal reality of race conditions, so invoke grep >> before sed. > > Comment is now stale; s/invoke grep before sed/make the sed script more > robust/ *sigh* It appears my hope of easily fixing patches is often shattered by the brutal reality, too. > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks! I'll fix the sentence when applying the series. Max
On Thu 09 Nov 2017 09:30:24 PM CET, Max Reitz wrote: > + echo > "$TEST_DIR/nbd-fault-injector.out" > $PYTHON nbd-fault-injector.py $extra_args "$nbd_addr" "$TEST_DIR/nbd-fault-injector.conf" >"$TEST_DIR/nbd-fault-injector.out" 2>&1 & It seems that in this patch you're indenting with spaces but this file uses tabs. Berto
On 2017-11-10 11:02, Alberto Garcia wrote: > On Thu 09 Nov 2017 09:30:24 PM CET, Max Reitz wrote: >> + echo > "$TEST_DIR/nbd-fault-injector.out" >> $PYTHON nbd-fault-injector.py $extra_args "$nbd_addr" "$TEST_DIR/nbd-fault-injector.conf" >"$TEST_DIR/nbd-fault-injector.out" 2>&1 & > > It seems that in this patch you're indenting with spaces but this file > uses tabs. Yes, but tabs are wrong. :-) Max
On Fri 10 Nov 2017 04:18:15 PM CET, Max Reitz wrote: > On 2017-11-10 11:02, Alberto Garcia wrote: >> On Thu 09 Nov 2017 09:30:24 PM CET, Max Reitz wrote: >>> + echo > "$TEST_DIR/nbd-fault-injector.out" >>> $PYTHON nbd-fault-injector.py $extra_args "$nbd_addr" "$TEST_DIR/nbd-fault-injector.conf" >"$TEST_DIR/nbd-fault-injector.out" 2>&1 & >> >> It seems that in this patch you're indenting with spaces but this file >> uses tabs. > > Yes, but tabs are wrong. :-) I actually agree with you, but don't mix them in the file :-) Berto
On 2017-11-10 16:51, Alberto Garcia wrote: > On Fri 10 Nov 2017 04:18:15 PM CET, Max Reitz wrote: >> On 2017-11-10 11:02, Alberto Garcia wrote: >>> On Thu 09 Nov 2017 09:30:24 PM CET, Max Reitz wrote: >>>> + echo > "$TEST_DIR/nbd-fault-injector.out" >>>> $PYTHON nbd-fault-injector.py $extra_args "$nbd_addr" "$TEST_DIR/nbd-fault-injector.conf" >"$TEST_DIR/nbd-fault-injector.out" 2>&1 & >>> >>> It seems that in this patch you're indenting with spaces but this file >>> uses tabs. >> >> Yes, but tabs are wrong. :-) > > I actually agree with you, but don't mix them in the file :-) I can whistle and say here, too, that Eric liked it. O:-) And I really don't want to use tabs in the second hunk of this patch, as it's broken over two lines. Max
On 11/10/2017 11:29 AM, Max Reitz wrote: >>>> It seems that in this patch you're indenting with spaces but this file >>>> uses tabs. >>> >>> Yes, but tabs are wrong. :-) >> >> I actually agree with you, but don't mix them in the file :-) > > I can whistle and say here, too, that Eric liked it. O:-) I don't really pay attention to which files have pre-existing TABs. You're right that preserving whole-file TABs is a bit nicer from consistency than reformatting a file wholesale; but then you have to tell checkpatch that preserving TABs was intentional. Mixed mode indentation is not as consistent, but at least keeps checkpatch happy without effort, and may make it easier for a patch down the road to finally do wholesale conversion of the rest of the file to avoid TABs. So when it comes to a file with existing TABs, I'm okay whether the patch preserves TABs (with documentation that it is doing so intentionally) or switches to mixed-mode spaces.
diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083 index 0306f112da..3c1adbf0fb 100755 --- a/tests/qemu-iotests/083 +++ b/tests/qemu-iotests/083 @@ -86,6 +86,7 @@ EOF rm -f "$TEST_DIR/nbd.sock" + echo > "$TEST_DIR/nbd-fault-injector.out" $PYTHON nbd-fault-injector.py $extra_args "$nbd_addr" "$TEST_DIR/nbd-fault-injector.conf" >"$TEST_DIR/nbd-fault-injector.out" 2>&1 & # Wait for server to be ready @@ -94,7 +95,8 @@ EOF done # Extract the final address (port number has now been assigned in tcp case) - nbd_addr=$(sed 's/Listening on \(.*\)$/\1/' "$TEST_DIR/nbd-fault-injector.out") + nbd_addr=$(sed -n 's/^Listening on //p' \ + "$TEST_DIR/nbd-fault-injector.out") if [ "$proto" = "tcp" ]; then nbd_url="nbd+tcp://$nbd_addr/$export_name"
083 has (at least) two issues: 1. By launching the nbd-fault-injector in background, it may not be scheduled until the first grep on its output file is executed. However, until then, that file may not have been created yet -- so it either does not exist yet (thus making the grep emit an error), or it does exist but contains stale data (thus making the rest of the test case work connect to a wrong address). Fix this by explicitly overwriting the output file before executing nbd-fault-injector. 2. The nbd-fault-injector prints things other than "Listening on...". It also prints a "Closing connection" message from time to time. We currently invoke sed on the whole file in the hope of it only containing the "Listening on..." line yet. That hope is sometimes shattered by the brutal reality of race conditions, so invoke grep before sed. Signed-off-by: Max Reitz <mreitz@redhat.com> --- tests/qemu-iotests/083 | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)