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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx
