| Message ID | 20191010152457.17713-5-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iotests: Add and use $SOCK_DIR | expand |
On 10/10/19 10:24 AM, Max Reitz wrote: > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > tests/qemu-iotests/common.filter | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter > index 9f418b4881..cd42f5e7e3 100644 > --- a/tests/qemu-iotests/common.filter > +++ b/tests/qemu-iotests/common.filter > @@ -43,7 +43,8 @@ _filter_qom_path() > # replace occurrences of the actual TEST_DIR value with TEST_DIR > _filter_testdir() > { > - $SED -e "s#$TEST_DIR/#TEST_DIR/#g" > + $SED -e "s#$TEST_DIR/#TEST_DIR/#g" \ > + -e "s#$SOCK_DIR/#SOCK_DIR/#g" Do we want to output a literal 'SOCK_DIR' (every test that uses it has to update their expected output), or can we make this also output a literal 'TEST_DIR' (output is a bit more confusing on which dir to look in, but fewer files to touch)? Your preference. Reviewed-by: Eric Blake <eblake@redhat.com>
On 10/10/19 10:24 AM, Max Reitz wrote: > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > tests/qemu-iotests/common.filter | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > @@ -218,7 +221,8 @@ _filter_nbd() > # Filter out the TCP port number since this changes between runs. > $SED -e '/nbd\/.*\.c:/d' \ > -e 's#127\.0\.0\.1:[0-9]*#127.0.0.1:PORT#g' \ > - -e "s#?socket=$TEST_DIR#?socket=TEST_DIR#g" \ > + -e "s#?socket=$SOCK_DIR#?socket=TEST_DIR#g" \ > + -e "s#?socket=$SOCK_DIR#?socket=SOCK_DIR#g" \ > -e 's#\(foo\|PORT/\?\|.sock\): Failed to .*$#\1#' This goes away in 23, but this looks crazy. Don't you really want the first line to replace $TEST_DIR with TEST_DIR (not $SOCK_DIR with TEST_DIR)? Otherwise, bisection is likely to break until all the intermediate patches have made the conversion to stop using TEST_DIR. I already gave R-b, but you'll need to fix this one.
On 10.10.19 20:42, Eric Blake wrote: > On 10/10/19 10:24 AM, Max Reitz wrote: >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> tests/qemu-iotests/common.filter | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/tests/qemu-iotests/common.filter >> b/tests/qemu-iotests/common.filter >> index 9f418b4881..cd42f5e7e3 100644 >> --- a/tests/qemu-iotests/common.filter >> +++ b/tests/qemu-iotests/common.filter >> @@ -43,7 +43,8 @@ _filter_qom_path() >> # replace occurrences of the actual TEST_DIR value with TEST_DIR >> _filter_testdir() >> { >> - $SED -e "s#$TEST_DIR/#TEST_DIR/#g" >> + $SED -e "s#$TEST_DIR/#TEST_DIR/#g" \ >> + -e "s#$SOCK_DIR/#SOCK_DIR/#g" > > Do we want to output a literal 'SOCK_DIR' (every test that uses it has > to update their expected output), or can we make this also output a > literal 'TEST_DIR' (output is a bit more confusing on which dir to look > in, but fewer files to touch)? Your preference. There’s another advantage to filtering it to be TEST_DIR, and that’s the fact that if $TEST_DIR and $SOCK_DIR are the same, we will always replace $SOCK_DIR by TEST_DIR. But I still preferred filtering it to be SOCK_DIR, because that seemed to me like we would have done it had we had a SOCK_DIR from the start. > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks for reviewing! Max
On 10.10.19 21:50, Eric Blake wrote: > On 10/10/19 10:24 AM, Max Reitz wrote: >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> tests/qemu-iotests/common.filter | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> > >> @@ -218,7 +221,8 @@ _filter_nbd() >> # Filter out the TCP port number since this changes between runs. >> $SED -e '/nbd\/.*\.c:/d' \ >> -e 's#127\.0\.0\.1:[0-9]*#127.0.0.1:PORT#g' \ >> - -e "s#?socket=$TEST_DIR#?socket=TEST_DIR#g" \ >> + -e "s#?socket=$SOCK_DIR#?socket=TEST_DIR#g" \ >> + -e "s#?socket=$SOCK_DIR#?socket=SOCK_DIR#g" \ >> -e 's#\(foo\|PORT/\?\|.sock\): Failed to .*$#\1#' > > This goes away in 23, but this looks crazy. Don't you really want the > first line to replace $TEST_DIR with TEST_DIR (not $SOCK_DIR with > TEST_DIR)? Otherwise, bisection is likely to break until all the > intermediate patches have made the conversion to stop using TEST_DIR. > > I already gave R-b, but you'll need to fix this one. Oops, yes. I messed it up. I only intended to add the SOCK_DIR replacement line. (Originally I had 23 merged into this one, and then I noticed it would break bisection, so I tried to pull it out. And failed, as you can see.) Max
On 11/10/2019 09.54, Max Reitz wrote: > On 10.10.19 20:42, Eric Blake wrote: >> On 10/10/19 10:24 AM, Max Reitz wrote: >>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>> --- >>> tests/qemu-iotests/common.filter | 8 ++++++-- >>> 1 file changed, 6 insertions(+), 2 deletions(-) >>> >>> diff --git a/tests/qemu-iotests/common.filter >>> b/tests/qemu-iotests/common.filter >>> index 9f418b4881..cd42f5e7e3 100644 >>> --- a/tests/qemu-iotests/common.filter >>> +++ b/tests/qemu-iotests/common.filter >>> @@ -43,7 +43,8 @@ _filter_qom_path() >>> # replace occurrences of the actual TEST_DIR value with TEST_DIR >>> _filter_testdir() >>> { >>> - $SED -e "s#$TEST_DIR/#TEST_DIR/#g" >>> + $SED -e "s#$TEST_DIR/#TEST_DIR/#g" \ >>> + -e "s#$SOCK_DIR/#SOCK_DIR/#g" >> >> Do we want to output a literal 'SOCK_DIR' (every test that uses it has >> to update their expected output), or can we make this also output a >> literal 'TEST_DIR' (output is a bit more confusing on which dir to look >> in, but fewer files to touch)? Your preference. > > There’s another advantage to filtering it to be TEST_DIR, and that’s the > fact that if $TEST_DIR and $SOCK_DIR are the same, we will always > replace $SOCK_DIR by TEST_DIR. > > But I still preferred filtering it to be SOCK_DIR, because that seemed > to me like we would have done it had we had a SOCK_DIR from the start. I also think that using SOCK_DIR is the better choice. It's a little bit more churn now, but in the long run, it will help to avoid confusion, and I think that's more important. Thomas
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter index 9f418b4881..cd42f5e7e3 100644 --- a/tests/qemu-iotests/common.filter +++ b/tests/qemu-iotests/common.filter @@ -43,7 +43,8 @@ _filter_qom_path() # replace occurrences of the actual TEST_DIR value with TEST_DIR _filter_testdir() { - $SED -e "s#$TEST_DIR/#TEST_DIR/#g" + $SED -e "s#$TEST_DIR/#TEST_DIR/#g" \ + -e "s#$SOCK_DIR/#SOCK_DIR/#g" } # replace occurrences of the actual IMGFMT value with IMGFMT @@ -124,6 +125,7 @@ _filter_img_create() $SED -e "s#$REMOTE_TEST_DIR#TEST_DIR#g" \ -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \ -e "s#$TEST_DIR#TEST_DIR#g" \ + -e "s#$SOCK_DIR#SOCK_DIR#g" \ -e "s#$IMGFMT#IMGFMT#g" \ -e 's#nbd:127.0.0.1:10810#TEST_DIR/t.IMGFMT#g' \ -e "s# encryption=off##g" \ @@ -160,6 +162,7 @@ _filter_img_info() $SED -e "s#$REMOTE_TEST_DIR#TEST_DIR#g" \ -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \ -e "s#$TEST_DIR#TEST_DIR#g" \ + -e "s#$SOCK_DIR#SOCK_DIR#g" \ -e "s#$IMGFMT#IMGFMT#g" \ -e 's#nbd://127.0.0.1:10810$#TEST_DIR/t.IMGFMT#g' \ -e 's#json.*vdisk-id.*vxhs"}}#TEST_DIR/t.IMGFMT#' \ @@ -218,7 +221,8 @@ _filter_nbd() # Filter out the TCP port number since this changes between runs. $SED -e '/nbd\/.*\.c:/d' \ -e 's#127\.0\.0\.1:[0-9]*#127.0.0.1:PORT#g' \ - -e "s#?socket=$TEST_DIR#?socket=TEST_DIR#g" \ + -e "s#?socket=$SOCK_DIR#?socket=TEST_DIR#g" \ + -e "s#?socket=$SOCK_DIR#?socket=SOCK_DIR#g" \ -e 's#\(foo\|PORT/\?\|.sock\): Failed to .*$#\1#' }
Signed-off-by: Max Reitz <mreitz@redhat.com> --- tests/qemu-iotests/common.filter | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)