Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR

[Amir Goldstein <amir73il@gmail.com>]

On Wed, Mar 8, 2017 at 4:59 PM, Zorro Lang <zlang@redhat.com> wrote:
> On Wed, Mar 08, 2017 at 04:26:22PM +0200, Amir Goldstein wrote:
>> On Wed, Mar 8, 2017 at 1:51 PM, Eryu Guan <eguan@redhat.com> 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@redhat.com> 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
>
>
>
> Hmm, I just read this page. This's the first time for me to learn
> about "Regex Boundaries", thanks for your sharing:) So "\B" will
> match:

Heh! it's first time for me too ;-)

>
> [word]\B[word]
> or
> [not-word]\B[not-word]
>
> I think both TEST_* or SCRATCH_* begin with "/", which is not a word.
> (We never use relative path at here, right? ...)
> so if we use "\B$TEST_DIR" we truly expect "[not-word]\B$TEST_DIR"
> ("^" belong to "not a word").
>

That is correct.

>> >>
>> >> 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"
>> }
>
> I just tested this method, generic/411 can work well by this change, even if
> set TEST_DIR=/mnt. But I haven't tested all xfstests cases. If we choose
> this way, it maybe affect lots of cases.
>

But can it really affect lots of cases??

If adding \B can break any test, that means that in some test output,
there exists a pattern [word]$TEST_DIR that is supposed to be
replaced with [word]TEST_DIR.
Even though I find it hard to believe that such case exists, I do not have to
resort to faith. I can use a simple test to verify if [word]TEST_DIR is
expected is golden output:

xfstests-dev$ git grep '\BTEST_DIR' tests/*/*.out
xfstests-dev$ git grep '\BTEST_DEV' tests/*/*.out
xfstests-dev$ git grep '\BSCRATCH_DEV' tests/*/*.out
xfstests-dev$ git grep '\BSCRATCH_MNT' tests/*/*.out

Nothing => Q.E.D


> So there're two ways, change single case and ask all future cases to notice
> this problem, or change common _filter function and make sure all xfstests
> cases can work well. What do you think, Eryu?
>
> Thanks,
> Zorro
>
>> From f365bd5d60d9d5b82e02db17e615380bf37a74de Mon Sep 17 00:00:00 2001
>> From: Amir Goldstein <amir73il@gmail.com>
>> 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@gmail.com>
>> ---
>>  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
>>
>