Re: [PATCH 19/25] xfs: scrub directory metadata

[Dave Chinner <david@fromorbit.com>]

On Fri, Oct 06, 2017 at 12:45:39PM -0700, Darrick J. Wong wrote:
> On Fri, Oct 06, 2017 at 06:07:14PM +1100, Dave Chinner wrote:
> > On Tue, Oct 03, 2017 at 01:42:49PM -0700, Darrick J. Wong wrote:
> > > +/* Scrub a directory entry. */
> > > +
> > > +struct xfs_scrub_dir_ctx {
> > > +	struct dir_context		dc;
> > > +	struct xfs_scrub_context	*sc;
> > > +};
> > 
> > These two character variable names make the code really hard to
> > understand, especially when....
> > 
> > > +
> > > +/* Check that an inode's mode matches a given DT_ type. */
> > > +STATIC int
> > > +xfs_scrub_dir_check_ftype(
> > > +	struct xfs_scrub_dir_ctx	*sdc,
> > 
> > ... we now have sc, dc, and sdc all intertwined. Especially as some
> > of these functions seem to invert the calling convention of the rest
> > of the scrub code in that the scrub context is the primary object
> > that is passed around and everything is attached to the sc....
> 
> The change in calling conventions is due to the fact that we're using
> the VFS filldir iterator, which doesn't have a facility for passing
> through a (void *).  To reuse that code,  we therefore must use this
> annoying xfs_scrub_dir_ctx wrapper so that we can pass our own context
> through to the _actor function.
> 
> Now, I /could/ make it a little clearer simply by doing this instead:
> 
> /*
>  * Scrub a single directory entry.
>  *
>  * We use the VFS directory iterator (i.e. readdir) to call this
>  * function for every directory entry in a directory.  Once we're here,
>  * we check the inode number to make sure it's sane, then we check that
>  * we can look up this filename.  Finally, we check the ftype.
>  */
> STATIC inline int
> xfs_scrub_dir_entry(
> 	struct xfs_scrub_context	*sc,
> 	const char			*name,
> 	int				namelen,
> 	loff_t				pos,
> 	u64				ino,
> 	unsigned			type)
> {
> 	/* do actual dirent scrubbing here */
> }
> 
> /* Adapter for VFS filldir iterator function. */
> STATIC int
> xfs_scrub_readdir_actor(
> 	struct dir_context		*dir_iter,
> 	const char			*name,
> 	int				namelen,
> 	loff_t				pos,
> 	u64				ino,
> 	unsigned			type)
> {
> 	struct xfs_scrub_dir_ctx	*sdc;
> 
> 	sdc = container_of(dir_iter, struct xfs_scrub_dir_ctx, dir_iter);
> 	return xfs_scrub_dir_entry(sdc->sc, name, namelen, pos, ino, type);
> }
> 
> At least that would contain the xfs_scrub_dir_ctx nastiness to a smaller
> part of the code.  Better?

Yeah, that does help, and with the comments you've added it makes it
easier to wrap my feeble brain around. Thanks!

-Dave.

-- 
Dave Chinner
david@fromorbit.com