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

[Dave Chinner <david@fromorbit.com>]

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.

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?

> @@ -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()....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com