[04/23] iotests: Filter $SOCK_DIR
diff mbox series

Message ID 20191010152457.17713-5-mreitz@redhat.com
State New
Headers show
Series
  • iotests: Add and use $SOCK_DIR
Related show

Commit Message

Max Reitz Oct. 10, 2019, 3:24 p.m. UTC
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/common.filter | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Eric Blake Oct. 10, 2019, 6:42 p.m. UTC | #1
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>
Eric Blake Oct. 10, 2019, 7:50 p.m. UTC | #2
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.
Max Reitz Oct. 11, 2019, 7:54 a.m. UTC | #3
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
Max Reitz Oct. 11, 2019, 7:57 a.m. UTC | #4
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
Thomas Huth Oct. 11, 2019, 7:57 a.m. UTC | #5
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

Patch
diff mbox series

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#'
 }