Re: [PATCH 2/2] xfs: constify xfs_name_dotdot

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Mar 10, 2022 at 09:32:45AM +1100, Dave Chinner wrote:
> On Wed, Mar 09, 2022 at 11:22:47AM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > The symbol xfs_name_dotdot is a global variable that the xfs codebase
> > uses here and there to look up directory dotdot entries.  Currently it's
> > a non-const variable, which means that it's a mutable global variable.
> > So far nobody's abused this to cause problems, but let's use the
> > compiler to enforce that.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/libxfs/xfs_dir2.c |    6 +++++-
> >  fs/xfs/libxfs/xfs_dir2.h |    2 +-
> >  fs/xfs/scrub/parent.c    |    6 ++++--
> >  fs/xfs/xfs_export.c      |    3 ++-
> >  4 files changed, 12 insertions(+), 5 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
> > index a77d931d65a3..cf9fa07e62d5 100644
> > --- a/fs/xfs/libxfs/xfs_dir2.c
> > +++ b/fs/xfs/libxfs/xfs_dir2.c
> > @@ -19,7 +19,11 @@
> >  #include "xfs_error.h"
> >  #include "xfs_trace.h"
> >  
> > -struct xfs_name xfs_name_dotdot = { (unsigned char *)"..", 2, XFS_DIR3_FT_DIR };
> > +const struct xfs_name xfs_name_dotdot = {
> > +	.name	= (unsigned char *)"..",
> > +	.len	= 2,
> > +	.type	= XFS_DIR3_FT_DIR,
> > +};
> 
> *nod*
> 
> > diff --git a/fs/xfs/scrub/parent.c b/fs/xfs/scrub/parent.c
> > index ab182a5cd0c0..e9549d998cdc 100644
> > --- a/fs/xfs/scrub/parent.c
> > +++ b/fs/xfs/scrub/parent.c
> > @@ -131,6 +131,7 @@ xchk_parent_validate(
> >  	xfs_ino_t		dnum,
> >  	bool			*try_again)
> >  {
> > +	struct xfs_name		dotdot = xfs_name_dotdot;
> >  	struct xfs_mount	*mp = sc->mp;
> >  	struct xfs_inode	*dp = NULL;
> >  	xfs_nlink_t		expected_nlink;
> > @@ -230,7 +231,7 @@ xchk_parent_validate(
> >  	expected_nlink = VFS_I(sc->ip)->i_nlink == 0 ? 0 : 1;
> >  
> >  	/* Look up '..' to see if the inode changed. */
> > -	error = xfs_dir_lookup(sc->tp, sc->ip, &xfs_name_dotdot, &dnum, NULL);
> > +	error = xfs_dir_lookup(sc->tp, sc->ip, &dotdot, &dnum, NULL);
> >  	if (!xchk_fblock_process_error(sc, XFS_DATA_FORK, 0, &error))
> >  		goto out_rele;
> >  
> 
> Why can't xfs_dir_lookup() be defined as a const xname for the input
> name? All it does is extract the contents into the da_args fields,
> and pass it to xfs_dir_hashname() which you converted to const in
> the previous patch.

The metadata directory patchset changes xfs_dir_lookup to return the
ftype in the dirent so that it can bail early if the dirent doesn't
point to the filetype that we want.

I wouldn't be /that/ sad to remove that bit of functionality in favor of
constifying the name parameter to xfs_dir_lookup.

Thinking about this more, I think the metadir code could implement its
own lookup dispatch function that /does/ return the dirent ftype, since
the sf/block/node/leaf functions all live in libxfs anyway.

> Or does the compiler then complain at all the other callsites that
> you're passing non-const stuff to const function parameters? i.e. am
> I just pulling on another dangling end of the ball of string at this
> point?

Nope, just djwong-dev blindness. :(

> > @@ -263,6 +264,7 @@ int
> >  xchk_parent(
> >  	struct xfs_scrub	*sc)
> >  {
> > +	struct xfs_name		dotdot = xfs_name_dotdot;
> >  	struct xfs_mount	*mp = sc->mp;
> >  	xfs_ino_t		dnum;
> >  	bool			try_again;
> > @@ -293,7 +295,7 @@ xchk_parent(
> >  	xfs_iunlock(sc->ip, XFS_ILOCK_EXCL | XFS_MMAPLOCK_EXCL);
> >  
> >  	/* Look up '..' */
> > -	error = xfs_dir_lookup(sc->tp, sc->ip, &xfs_name_dotdot, &dnum, NULL);
> > +	error = xfs_dir_lookup(sc->tp, sc->ip, &dotdot, &dnum, NULL);
> >  	if (!xchk_fblock_process_error(sc, XFS_DATA_FORK, 0, &error))
> >  		goto out;
> >  	if (!xfs_verify_dir_ino(mp, dnum)) {
> > diff --git a/fs/xfs/xfs_export.c b/fs/xfs/xfs_export.c
> > index 1064c2342876..8939119191f4 100644
> > --- a/fs/xfs/xfs_export.c
> > +++ b/fs/xfs/xfs_export.c
> > @@ -206,10 +206,11 @@ STATIC struct dentry *
> >  xfs_fs_get_parent(
> >  	struct dentry		*child)
> >  {
> > +	struct xfs_name		dotdot = xfs_name_dotdot;
> >  	int			error;
> >  	struct xfs_inode	*cip;
> >  
> > -	error = xfs_lookup(XFS_I(d_inode(child)), &xfs_name_dotdot, &cip, NULL);
> > +	error = xfs_lookup(XFS_I(d_inode(child)), &dotdot, &cip, NULL);
> >  	if (unlikely(error))
> >  		return ERR_PTR(error);
> 
> This only calls xfs_dir_lookup() with name, so if xfs_dir_lookup()
> can have a const name, so can xfs_lookup()....

Yep.  I'll fix that in the next version.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com