Message ID | 20210218085752.24123-1-xifeng@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v3] common/rc: add filter in _test_inode_flag | expand |
On Thu, Feb 18, 2021 at 04:57:52PM +0800, XiaoLi Feng wrote: > From: Xiaoli Feng <xifeng@redhat.com> > > Add _filter_testdir_and_scratch to avoid _test_inode_flag failed when > mount point contains flag string. > > Signed-off-by: Xiaoli Feng <xifeng@redhat.com> > --- > common/rc | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/common/rc b/common/rc > index 649b1cfd..473188c1 100644 > --- a/common/rc > +++ b/common/rc > @@ -3112,7 +3112,7 @@ _test_inode_flag() > local flag=$1 > local file=$2 > > - if $XFS_IO_PROG -r -c 'lsattr -v' "$file" | grep -q "$flag" ; then > + if $XFS_IO_PROG -r -c 'lsattr -v' "$file" | _filter_testdir_and_scratch | grep -q "$flag" ; then I can't understand that. Could you give us an example about why we need this filter before "quiet grep"? I think this line doesn't have any output (except xfs_io fails), so why a filter is needed? Thanks, Zorro > return 0 > fi > return 1 > -- > 2.18.1 >
----- 原始邮件 ----- > 发件人: "Zorro Lang" <zlang@redhat.com> > 收件人: "XiaoLi Feng" <xifeng@redhat.com> > 抄送: fstests@vger.kernel.org > 发送时间: 星期四, 2021年 2 月 18日 下午 9:41:34 > 主题: Re: [PATCH v3] common/rc: add filter in _test_inode_flag > > On Thu, Feb 18, 2021 at 04:57:52PM +0800, XiaoLi Feng wrote: > > From: Xiaoli Feng <xifeng@redhat.com> > > > > Add _filter_testdir_and_scratch to avoid _test_inode_flag failed when > > mount point contains flag string. > > > > Signed-off-by: Xiaoli Feng <xifeng@redhat.com> > > --- > > common/rc | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/common/rc b/common/rc > > index 649b1cfd..473188c1 100644 > > --- a/common/rc > > +++ b/common/rc > > @@ -3112,7 +3112,7 @@ _test_inode_flag() > > local flag=$1 > > local file=$2 > > > > - if $XFS_IO_PROG -r -c 'lsattr -v' "$file" | grep -q "$flag" ; then > > + if $XFS_IO_PROG -r -c 'lsattr -v' "$file" | _filter_testdir_and_scratch | > > grep -q "$flag" ; then > > I can't understand that. Could you give us an example about why we need this > filter before "quiet grep"? I think this line doesn't have any output > (except xfs_io fails), so why a filter is needed? Oh, I just saw your reply for V1 patch review: "When test generic/608 for dax on xfs, "_check_xflag $t_file 0" is always failed if the $f_file has dax string. Yes, here should also include filter for TEST_DIR." The _check_xflag() function as below: _check_xflag() { local target=$1 local exp_xflag=$2 if [ $exp_xflag -eq 0 ]; then _test_inode_flag dax $target && echo "$target has unexpected FS_XFLAG_DAX flag" else _test_inode_flag dax $target || echo "$target doesn't have expected FS_XFLAG_DAX flag" fi } Sure, if the $1 is something likes "/mnt/scratch/file", later echo "$target ...." will print "/mnt/scratch". If you're trying to fix this issue, I think you should filter the "$target" in or behind _check_xflag(), not in _test_inode_flag() which has no output. Thanks, Zorro > > Thanks, > Zorro > > > return 0 > > fi > > return 1 > > -- > > 2.18.1 > > >
Hi, ----- Original Message ----- > From: "Zorro Lang" <zlang@redhat.com> > To: "XiaoLi Feng" <xifeng@redhat.com> > Cc: fstests@vger.kernel.org > Sent: Thursday, February 18, 2021 9:41:35 PM > Subject: Re: [PATCH v3] common/rc: add filter in _test_inode_flag > > On Thu, Feb 18, 2021 at 04:57:52PM +0800, XiaoLi Feng wrote: > > From: Xiaoli Feng <xifeng@redhat.com> > > > > Add _filter_testdir_and_scratch to avoid _test_inode_flag failed when > > mount point contains flag string. > > > > Signed-off-by: Xiaoli Feng <xifeng@redhat.com> > > --- > > common/rc | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/common/rc b/common/rc > > index 649b1cfd..473188c1 100644 > > --- a/common/rc > > +++ b/common/rc > > @@ -3112,7 +3112,7 @@ _test_inode_flag() > > local flag=$1 > > local file=$2 > > > > - if $XFS_IO_PROG -r -c 'lsattr -v' "$file" | grep -q "$flag" ; then > > + if $XFS_IO_PROG -r -c 'lsattr -v' "$file" | _filter_testdir_and_scratch | > > grep -q "$flag" ; then > > I can't understand that. Could you give us an example about why we need this > filter before "quiet grep"? I think this line doesn't have any output > (except xfs_io fails), so why a filter is needed? Yes. this line doesn't have any output. But the return value is different. I send this patch to fix the issue: If testdir or scratch_dir has "$flag". Then it will always return 0. Even if this file doesn't have this "$flag". Example: xfs_io -r -c 'lsattr -v' /daxmnt/fileB [] /daxmnt/fileB # xfs_io -r -c 'lsattr -v' /daxmnt/fileB | grep -q "dax" # echo $? 0 fileB doesn't have dax flag. But _test_inode_flag will always return 0(expect 1). Thanks. > > Thanks, > Zorro > > > return 0 > > fi > > return 1 > > -- > > 2.18.1 > > > >
On Fri, Feb 19, 2021 at 11:00:18AM -0500, Xiaoli Feng wrote: > Hi, > > ----- Original Message ----- > > From: "Zorro Lang" <zlang@redhat.com> > > To: "XiaoLi Feng" <xifeng@redhat.com> > > Cc: fstests@vger.kernel.org > > Sent: Thursday, February 18, 2021 9:41:35 PM > > Subject: Re: [PATCH v3] common/rc: add filter in _test_inode_flag > > > > On Thu, Feb 18, 2021 at 04:57:52PM +0800, XiaoLi Feng wrote: > > > From: Xiaoli Feng <xifeng@redhat.com> > > > > > > Add _filter_testdir_and_scratch to avoid _test_inode_flag failed when > > > mount point contains flag string. > > > > > > Signed-off-by: Xiaoli Feng <xifeng@redhat.com> > > > --- > > > common/rc | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/common/rc b/common/rc > > > index 649b1cfd..473188c1 100644 > > > --- a/common/rc > > > +++ b/common/rc > > > @@ -3112,7 +3112,7 @@ _test_inode_flag() > > > local flag=$1 > > > local file=$2 > > > > > > - if $XFS_IO_PROG -r -c 'lsattr -v' "$file" | grep -q "$flag" ; then > > > + if $XFS_IO_PROG -r -c 'lsattr -v' "$file" | _filter_testdir_and_scratch | > > > grep -q "$flag" ; then > > > > I can't understand that. Could you give us an example about why we need this > > filter before "quiet grep"? I think this line doesn't have any output > > (except xfs_io fails), so why a filter is needed? > > Yes. this line doesn't have any output. But the return value is different. I send > this patch to fix the issue: If testdir or scratch_dir has "$flag". Then it will > always return 0. Even if this file doesn't have this "$flag". > > Example: > xfs_io -r -c 'lsattr -v' /daxmnt/fileB > [] /daxmnt/fileB > # xfs_io -r -c 'lsattr -v' /daxmnt/fileB | grep -q "dax" > # echo $? > 0 > > fileB doesn't have dax flag. But _test_inode_flag will always return 0(expect 1). Oh, got it. So the 'grep -q "dax"' is a litte inaccurate. If the mountpoint or file name, or any sub-string in the $file contains "dax", this function will return 0. If that's the issue you're fixing, I think only filter SCRATCH_MNT and TEST_MNT isn't enough. If we only care about the "attr" part output, how about filter out junk (path name), only keep the [attr]? Due to looks like the output format of 'lsattr -v' is: "[$attr] $filename" (correct me if I'm wrong). So if we only keep the attr part, that can avoid the 'grep' find "dax" from $filename. Thanks, Zorro > > Thanks. > > > > > Thanks, > > Zorro > > > > > return 0 > > > fi > > > return 1 > > > -- > > > 2.18.1 > > > > > > >
diff --git a/common/rc b/common/rc index 649b1cfd..473188c1 100644 --- a/common/rc +++ b/common/rc @@ -3112,7 +3112,7 @@ _test_inode_flag() local flag=$1 local file=$2 - if $XFS_IO_PROG -r -c 'lsattr -v' "$file" | grep -q "$flag" ; then + if $XFS_IO_PROG -r -c 'lsattr -v' "$file" | _filter_testdir_and_scratch | grep -q "$flag" ; then return 0 fi return 1