| Message ID | 20171109013804.14488-5-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/08/2017 07:38 PM, Max Reitz wrote: > 083 has (at least) two issues: I think I hit one of them intermittently yesterday; thanks for diagnosing these (and like you say, there may be more lurking, but we'll whack them separately if we can reproduce and identify them). > > 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. Invoking 'grep | sed' is almost always a waste of a process; sed can do the job alone. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > tests/qemu-iotests/083 | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083 > index 0306f112da..2f6444eeb9 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" This makes the file contain a blank line. Would it be any better as a truly empty file, as in: : > "$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,7 @@ 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=$(grep 'Listening on ' "$TEST_DIR/nbd-fault-injector.out" | sed 's/Listening on \(.*\)$/\1/') Fixing TAB damage while at it - nice. Here's how to do it using just sed, and with less typing: nbd_addr=$(sed -n 's/^Listening on //p' \ "$TEST_DIR/nbd-fault-injector.out")
On 2017-11-09 15:11, Eric Blake wrote: > On 11/08/2017 07:38 PM, Max Reitz wrote: >> 083 has (at least) two issues: > > I think I hit one of them intermittently yesterday; thanks for > diagnosing these (and like you say, there may be more lurking, but we'll > whack them separately if we can reproduce and identify them). > >> >> 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. > > Invoking 'grep | sed' is almost always a waste of a process; sed can do > the job alone. > >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> tests/qemu-iotests/083 | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083 >> index 0306f112da..2f6444eeb9 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" > > This makes the file contain a blank line. Would it be any better as a > truly empty file, as in: > > : > "$TEST_DIR/nbd-fault-injector.out" Although ":>" looks funny, I guess I'd rather stay with the echo... Yes, it may look stupid to you, but I would have to look up what exactly that will do, whereas the echo is clear. And in the end, both work equally fine. >> $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,7 @@ 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=$(grep 'Listening on ' "$TEST_DIR/nbd-fault-injector.out" | sed 's/Listening on \(.*\)$/\1/') > > Fixing TAB damage while at it - nice. > > Here's how to do it using just sed, and with less typing: > > nbd_addr=$(sed -n 's/^Listening on //p' \ > "$TEST_DIR/nbd-fault-injector.out") Oh, nice! Will do, thanks. Max
diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083 index 0306f112da..2f6444eeb9 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,7 @@ 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=$(grep 'Listening on ' "$TEST_DIR/nbd-fault-injector.out" | sed 's/Listening on \(.*\)$/\1/') 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 | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)