On Wed, Mar 8, 2017 at 1:51 PM, Eryu Guan <eguan@xxxxxxxxxx> wrote:
> On Wed, Mar 08, 2017 at 12:01:33PM +0200, Amir Goldstein wrote:
>> On Wed, Mar 8, 2017 at 9:26 AM, Zorro Lang <zlang@xxxxxxxxxx> wrote:
>> > Darrick found generic/411 golden output mismatch if use
>> > TEST_DIR=/mnt. Because g/411 use some test path named
>> > /mnt/XXXX/mnt1/mnt2, _filter_test_dir will replace all
>> > "/mnt" things to "TEST_DIR".
>> >
>> > For stop this failure, change all directory names to be
>> > "$seq-XXX", that's less likely to be mistaken for TEST_*
>> > and SCRATCH_*.
>> >
>>
>> Although you have a right to choose whichever names you
>> want top use for your test, this is papering over a bug.
>>
>> I re-read the docuemtnation for \B:
>> http://www.rexegg.com/regex-boundaries.html#bengines
>>
>> To my understanding, the expression "\B$TEST_DIR" will
>> match every instance of $TEST_DIR, where preceding character
>> is NOT a letter, number or underscore.
>> This is because $TEST_DIR must start with '/', which is not
>> a letter, number or underscore.
>>
>> I think it should be safe to fix _filter_test_dir and _filter_scratch.
>
> I agreed, according to the document above, \B matches all positions
> where \b doesn't match. And \b matches positions where "one side is a
> word character and the other side is not", so \B matches "neither side
> is a word character" and "both sides are a word character".
>
> This is also because we canonicalized all mount points, there's no path
> like //mnt/mnt1 is allowed in fstests. And this leads me to wonder if we
> should canonicalize all test devices (if they're block device), to avoid
> something like //dev/sda5? The double "/" will break the \B match.
>
//dev/sda5 does to break \B match.
\B$TEST_DEV match, as you wrote, means that character before
$TEST_DEV *is not* a word character, because $TEST_DEV begins
with a non-word character. it doesn't matter the the second character
is a non-word as well.
Attached a patch that I tested -g quick on xfs and overlayfs
and did not see any regressions, but you better test it with a wider
variety of fs types and a larger group of tests.
> Further more, if we decide to use \B to improve _filter_test_dir and
> _filter_scratch, it appears to me that the fix from commit 4e965d8
> ("fstests: fix test and scratch filters for overlapping DEV/MNT paths")
> can be discarded, the order is not a problem anymore.
>
Well, not exactly.
The case that commit 4e965d8 fixes is:
TEST_DEV=/mnt
TEST_DIR=/mnt/ovl-mnt
So \B$TEST_DEV will still match the prefix of $TEST_DIR and this is
wrong.
However, while $TEST_DEV can really be a prefix of $TEST_DIR
(and will always be a prefix with new -overlay)
I doubt that $TEST_DIR can ever be a prefix of $TEST_DEV.
So instead of the full blown test in commit 4e965d8, it would
suffice to just match $TEST_DIR before $TEST_DEV
(i.e. keep the if case and drop the else case).
So before you test \B with all fs types, you may revert commit 4e965d8,
but please make sure that the resulting filters are in the correct order:
_filter_test_dir()
{
# TEST_DEV may be a prefix of TEST_DIR (e.g. /mnt and /mnt/ovl-mnt)
# substitute TEST_DIR first
sed -e "s,\B$TEST_DIR,TEST_DIR,g" \
-e "s,\B$TEST_DEV,TEST_DEV,g"
}
_filter_scratch()
{
# SCRATCH_DEV may be a prefix of SCRATCH_MNT
# substitute SCRATCH_MNT first
sed -e "s,\B$SCRATCH_MNT,SCRATCH_MNT,g" \
-e "s,\B$SCRATCH_DEV,SCRATCH_DEV,g" \
-e "/.use_space/d"
}
From f365bd5d60d9d5b82e02db17e615380bf37a74de Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@xxxxxxxxx>
Date: Wed, 8 Mar 2017 12:38:22 +0200
Subject: [PATCH] filter: match $TEST_* $SCRATCH_* in beginning of path string
For example, if $TEST_DIR=/mnt, only replace instacnes of /mnt that
are in the beginning of a path string, e.g.:
"/mnt/mntA/mntB:/mnt/mntC" => "TEST_DIR/mntA/mntB:TEST_DIR/mntC"
Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
---
common/filter | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/common/filter b/common/filter
index 1ceb346..2509483 100644
--- a/common/filter
+++ b/common/filter
@@ -283,13 +283,13 @@ _filter_test_dir()
if ( echo $TEST_DIR | grep -q $TEST_DEV ); then
# TEST_DEV is substr of TEST_DIR (e.g. /mnt and /mnt/ovl-mnt)
# substitute TEST_DIR first
- sed -e "s,$TEST_DIR,TEST_DIR,g" \
- -e "s,$TEST_DEV,TEST_DEV,g"
+ sed -e "s,\B$TEST_DIR,TEST_DIR,g" \
+ -e "s,\B$TEST_DEV,TEST_DEV,g"
else
# TEST_DIR maybe a substr of TEST_DIR (e.g. /vdc and /dev/vdc)
# substitute TEST_DEV first
- sed -e "s,$TEST_DEV,TEST_DEV,g" \
- -e "s,$TEST_DIR,TEST_DIR,g"
+ sed -e "s,\B$TEST_DEV,TEST_DEV,g" \
+ -e "s,\B$TEST_DIR,TEST_DIR,g"
fi
}
@@ -297,13 +297,13 @@ _filter_scratch()
{
if ( echo $SCRATCH_MNT | grep -q $SCRATCH_DEV ); then
# SCRATCH_DEV is substr of SCRATCH_MNT
- sed -e "s,$SCRATCH_MNT,SCRATCH_MNT,g" \
- -e "s,$SCRATCH_DEV,SCRATCH_DEV,g" \
+ sed -e "s,\B$SCRATCH_MNT,SCRATCH_MNT,g" \
+ -e "s,\B$SCRATCH_DEV,SCRATCH_DEV,g" \
-e "/.use_space/d"
else
# SCRATCH_MNT maybe a substr of SCRATCH_DEV
- sed -e "s,$SCRATCH_DEV,SCRATCH_DEV,g" \
- -e "s,$SCRATCH_MNT,SCRATCH_MNT,g" \
+ sed -e "s,\B$SCRATCH_DEV,SCRATCH_DEV,g" \
+ -e "s,\B$SCRATCH_MNT,SCRATCH_MNT,g" \
-e "/.use_space/d"
fi
}
--
2.7.4
