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@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
