linux-ext4 - Re: [PATCH 5/7] common: rename _filter_mkfs to _xfs_filter

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 9 Feb 2022 08:18:50 +0000
From:   Shinichiro Kawasaki <shinichiro.kawasaki@....com>
To:     "Darrick J. Wong" <djwong@...nel.org>
CC:     "fstests@...r.kernel.org" <fstests@...r.kernel.org>,
        "linux-btrfs@...r.kernel.org" <linux-btrfs@...r.kernel.org>,
        "linux-xfs@...r.kernel.org" <linux-xfs@...r.kernel.org>,
        "linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>,
        Naohiro Aota <Naohiro.Aota@....com>,
        Johannes Thumshirn <Johannes.Thumshirn@....com>,
        Damien Le Moal <damien.lemoal@...nsource.wdc.com>
Subject: Re: [PATCH 5/7] common: rename _filter_mkfs to _xfs_filter_mkfs

On Feb 08, 2022 / 16:53, Darrick J. Wong wrote:
> On Tue, Feb 08, 2022 at 04:43:16PM -0800, Darrick J. Wong wrote:
> > On Mon, Feb 07, 2022 at 03:55:39PM +0900, Shin'ichiro Kawasaki wrote:
> > > The helper function works only for xfs and used only for xfs except
> > > generic/204. Rename the function to clearly indicate that the function
> > > is only for xfs.
> > > 
> > > Suggested-by: Naohiro Aota <naohiro.aota@....com>
> > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@....com>
> > 
> > <snip the diffstat>
> > 
> > > diff --git a/common/attr b/common/attr
> > > index 35682d7c..964c790a 100644
> > > --- a/common/attr
> > > +++ b/common/attr
> > > @@ -13,7 +13,7 @@ _acl_get_max()
> > >  		# CRC format filesystems have much larger ACL counts. The actual
> > >  		# number is into the thousands, but testing that meany takes too
> > >  		# long, so just test well past the old limit of 25.
> > > -		$XFS_INFO_PROG $TEST_DIR | _filter_mkfs > /dev/null 2> $tmp.info
> > > +		$XFS_INFO_PROG $TEST_DIR | _xfs_filter_mkfs > /dev/null 2> $tmp.info
> > >  		. $tmp.info
> > >  		rm $tmp.info
> > >  		if [ $_fs_has_crcs -eq 0 ]; then
> > > diff --git a/common/filter b/common/filter
> > > index c3db7a56..24fd0650 100644
> > > --- a/common/filter
> > > +++ b/common/filter
> > > @@ -117,7 +117,7 @@ _filter_date()
> > >  
> > >  # prints filtered output on stdout, values (use eval) on stderr
> > >  # Non XFS filesystems always return a 4k block size and a 256 byte inode.
> > > -_filter_mkfs()
> > > +_xfs_filter_mkfs()
> > >  {
> > >      case $FSTYP in
> > >      xfs)
> > > diff --git a/common/xfs b/common/xfs
> > 
> > This renames the generic function to be "only for xfs" but it leaves the
> > non-XFS bits.  Those bits are /really/ problematic (hardcoded
> > isize=256 and dbsize=4096?  Seriously??) and themselves were introduced
> > in commit a4d5b247 ("xfstests: Make 204 work with different block and
> > inode sizes.") oh wow.
> > 
> > I'm sorry that someone left this a mess, but let's try to make it easy
> > to clean up all the other filesystems, please.  Specifically, could you
> > please:
> > 
> > 1. Hoist the XFS-specific code from _filter_mkfs into a new
> >    helper _xfs_filter_mkfs() in common/xfs?
> 
> UGH.  I pressed <Send>, not <Save>.  Picking up from where I left off:
> 
> 2. Make the generic _filter_mkfs function call the XFS-specific one,
> ala:
> 
> _filter_mkfs()
> {
>     case $FSTYP in
>     xfs)
> 	_xfs_filter_mkfs "$@"
> 	;;
>     *)
> 	cat - >/dev/null
> 	perl -e 'print STDERR "dbsize=4096\nisize=256\n"'
> 	return ;;
>     esac
> }
> 
> This way you don't have to make a gigantic treewide change, and we can
> start to make this function work properly for filesystems that have the
> ability to do an offline geometry dump (aka dumpe2fs for ext*).

Thank you for the comment. My thought was a rather negative one: I assumed that
_filter_mkfs would not support other filesystems than xfs. I will add a patch
in v2 series based on your suggestion above, expecting that _filter_mkfs will
support other filesystems in the future.

-- 
Best Regards,
Shin'ichiro Kawasaki