Re: [PATCH] vfs: replace ints with enum component_type for LAST_XXX

[Amir Goldstein]

On Tue, Mar 31, 2026 at 7:37 PM Jori Koolstra <jkoolstra@xxxxxxxxx> wrote:
>
> Several functions in namei.c take an "int *type" parameter, such as
> filename_parentat(). To know what values this can take you have to find
> the anonymous struct that defines the LAST_XXX values. I would argue
> that the readability of the code is improved by making this an explicit
> type. Also, when we have this "enum component_type" is does not really
> make sense anymore to talk about LAST_XXX, but we should just use XXX,
> as there is nothing specific about the component type showing up at the
> end of a path.
>
> Signed-off-by: Jori Koolstra <jkoolstra@xxxxxxxxx>
> ---
>  fs/namei.c            | 75 ++++++++++++++++++++++---------------------
>  include/linux/namei.h |  4 +--
>  2 files changed, 41 insertions(+), 38 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 9e5500dad14f..1eb9db055292 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -721,15 +721,15 @@ EXPORT_SYMBOL(path_put);
>
>  #define EMBEDDED_LEVELS 2
>  struct nameidata {
> -       struct path     path;
> -       struct qstr     last;
> -       struct path     root;
> -       struct inode    *inode; /* path.dentry.d_inode */
> -       unsigned int    flags, state;
> -       unsigned        seq, next_seq, m_seq, r_seq;
> -       int             last_type;
> -       unsigned        depth;
> -       int             total_link_count;
> +       struct path             path;
> +       struct qstr             last;
> +       struct path             root;
> +       struct inode            *inode; /* path.dentry.d_inode */
> +       unsigned int            flags, state;
> +       unsigned                seq, next_seq, m_seq, r_seq;
> +       enum component_type     last_type;
> +       unsigned                depth;
> +       int                     total_link_count;
>         struct saved {
>                 struct path link;
>                 struct delayed_call done;
> @@ -2221,9 +2221,9 @@ static struct dentry *follow_dotdot(struct nameidata *nd)
>         return dget(nd->path.dentry);
>  }
>
> -static const char *handle_dots(struct nameidata *nd, int type)
> +static const char *handle_dots(struct nameidata *nd, enum component_type type)
>  {
> -       if (type == LAST_DOTDOT) {
> +       if (type == DOTDOT) {
>                 const char *error = NULL;
>                 struct dentry *parent;
>
> @@ -2267,7 +2267,7 @@ static __always_inline const char *walk_component(struct nameidata *nd, int flag
>          * to be able to know about the current root directory and
>          * parent relationships.
>          */
> -       if (unlikely(nd->last_type != LAST_NORM)) {
> +       if (unlikely(nd->last_type != NORMAL)) {
>                 if (unlikely(nd->depth) && !(flags & WALK_MORE))
>                         put_link(nd);
>                 return handle_dots(nd, nd->last_type);
> @@ -2577,7 +2577,7 @@ static int link_path_walk(const char *name, struct nameidata *nd)
>         int depth = 0; // depth <= nd->depth
>         int err;
>
> -       nd->last_type = LAST_ROOT;
> +       nd->last_type = ROOT;
>         nd->flags |= LOOKUP_PARENT;
>         if (IS_ERR(name))
>                 return PTR_ERR(name);
> @@ -2607,16 +2607,16 @@ static int link_path_walk(const char *name, struct nameidata *nd)
>
>                 switch(lastword) {
>                 case LAST_WORD_IS_DOTDOT:
> -                       nd->last_type = LAST_DOTDOT;
> +                       nd->last_type = DOTDOT;
>                         nd->state |= ND_JUMPED;
>                         break;
>
>                 case LAST_WORD_IS_DOT:
> -                       nd->last_type = LAST_DOT;
> +                       nd->last_type = DOT;
>                         break;
>
>                 default:
> -                       nd->last_type = LAST_NORM;
> +                       nd->last_type = NORMAL;
>                         nd->state &= ~ND_JUMPED;
>
>                         struct dentry *parent = nd->path.dentry;
> @@ -2780,7 +2780,7 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
>
>  static inline const char *lookup_last(struct nameidata *nd)
>  {
> -       if (nd->last_type == LAST_NORM && nd->last.name[nd->last.len])
> +       if (nd->last_type == NORMAL && nd->last.name[nd->last.len])
>                 nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY;
>
>         return walk_component(nd, WALK_TRAILING);
> @@ -2869,7 +2869,7 @@ static int path_parentat(struct nameidata *nd, unsigned flags,
>  /* Note: this does not consume "name" */
>  static int __filename_parentat(int dfd, struct filename *name,
>                                unsigned int flags, struct path *parent,
> -                              struct qstr *last, int *type,
> +                              struct qstr *last, enum component_type *type,
>                                const struct path *root)
>  {
>         int retval;
> @@ -2894,7 +2894,7 @@ static int __filename_parentat(int dfd, struct filename *name,
>
>  static int filename_parentat(int dfd, struct filename *name,
>                              unsigned int flags, struct path *parent,
> -                            struct qstr *last, int *type)
> +                            struct qstr *last, enum component_type *type)
>  {
>         return __filename_parentat(dfd, name, flags, parent, last, type, NULL);
>  }
> @@ -2963,12 +2963,13 @@ static struct dentry *__start_removing_path(int dfd, struct filename *name,
>         struct path parent_path __free(path_put) = {};
>         struct dentry *d;
>         struct qstr last;
> -       int type, error;
> +       enum component_type type;
> +       int error;
>
>         error = filename_parentat(dfd, name, 0, &parent_path, &last, &type);
>         if (error)
>                 return ERR_PTR(error);
> -       if (unlikely(type != LAST_NORM))
> +       if (unlikely(type != NORMAL))
>                 return ERR_PTR(-EINVAL);
>         /* don't fail immediately if it's r/o, at least try to report other errors */
>         error = mnt_want_write(parent_path.mnt);
> @@ -3009,12 +3010,13 @@ struct dentry *kern_path_parent(const char *name, struct path *path)
>         CLASS(filename_kernel, filename)(name);
>         struct dentry *d;
>         struct qstr last;
> -       int type, error;
> +        enum component_type type;
> +       int error;
>
>         error = filename_parentat(AT_FDCWD, filename, 0, &parent_path, &last, &type);
>         if (error)
>                 return ERR_PTR(error);
> -       if (unlikely(type != LAST_NORM))
> +       if (unlikely(type != NORMAL))
>                 return ERR_PTR(-EINVAL);
>
>         d = lookup_noperm_unlocked(&last, parent_path.dentry);
> @@ -3057,7 +3059,7 @@ EXPORT_SYMBOL(kern_path);
>   * @root: pointer to struct path of the base directory
>   */
>  int vfs_path_parent_lookup(struct filename *filename, unsigned int flags,
> -                          struct path *parent, struct qstr *last, int *type,
> +                          struct path *parent, struct qstr *last, enum component_type *type,
>                            const struct path *root)
>  {
>         return  __filename_parentat(AT_FDCWD, filename, flags, parent, last,
> @@ -4550,7 +4552,7 @@ static const char *open_last_lookups(struct nameidata *nd,
>
>         nd->flags |= op->intent;
>
> -       if (nd->last_type != LAST_NORM) {
> +       if (nd->last_type != NORMAL) {
>                 if (nd->depth)
>                         put_link(nd);
>                 return handle_dots(nd, nd->last_type);
> @@ -4903,7 +4905,7 @@ static struct dentry *filename_create(int dfd, struct filename *name,
>         bool want_dir = lookup_flags & LOOKUP_DIRECTORY;
>         unsigned int reval_flag = lookup_flags & LOOKUP_REVAL;
>         unsigned int create_flags = LOOKUP_CREATE | LOOKUP_EXCL;
> -       int type;
> +       enum component_type type;
>         int error;
>
>         error = filename_parentat(dfd, name, reval_flag, path, &last, &type);
> @@ -4914,7 +4916,7 @@ static struct dentry *filename_create(int dfd, struct filename *name,
>          * Yucky last component or no last component at all?
>          * (foo/., foo/.., /////)
>          */
> -       if (unlikely(type != LAST_NORM))
> +       if (unlikely(type != NORMAL))
>                 goto out;
>
>         /* don't fail immediately if it's r/o, at least try to report other errors */
> @@ -5365,7 +5367,7 @@ int filename_rmdir(int dfd, struct filename *name)
>         struct dentry *dentry;
>         struct path path;
>         struct qstr last;
> -       int type;
> +       enum component_type type;
>         unsigned int lookup_flags = 0;
>         struct delegated_inode delegated_inode = { };
>  retry:
> @@ -5374,15 +5376,16 @@ int filename_rmdir(int dfd, struct filename *name)
>                 return error;
>
>         switch (type) {
> -       case LAST_DOTDOT:
> +       case DOTDOT:
>                 error = -ENOTEMPTY;
>                 goto exit2;
> -       case LAST_DOT:
> +       case DOT:
>                 error = -EINVAL;
>                 goto exit2;
> -       case LAST_ROOT:
> +       case ROOT:
>                 error = -EBUSY;
>                 goto exit2;
> +       case NORMAL: ; // OK
>         }
>
>         error = mnt_want_write(path.mnt);
> @@ -5507,7 +5510,7 @@ int filename_unlinkat(int dfd, struct filename *name)
>         struct dentry *dentry;
>         struct path path;
>         struct qstr last;
> -       int type;
> +       enum component_type type;
>         struct inode *inode;
>         struct delegated_inode delegated_inode = { };
>         unsigned int lookup_flags = 0;
> @@ -5517,7 +5520,7 @@ int filename_unlinkat(int dfd, struct filename *name)
>                 return error;
>
>         error = -EISDIR;
> -       if (type != LAST_NORM)
> +       if (type != NORMAL)
>                 goto exit_path_put;
>
>         error = mnt_want_write(path.mnt);
> @@ -6074,7 +6077,7 @@ int filename_renameat2(int olddfd, struct filename *from,
>         struct renamedata rd;
>         struct path old_path, new_path;
>         struct qstr old_last, new_last;
> -       int old_type, new_type;
> +       enum component_type old_type, new_type;
>         struct delegated_inode delegated_inode = { };
>         unsigned int lookup_flags = 0;
>         bool should_retry = false;
> @@ -6103,12 +6106,12 @@ int filename_renameat2(int olddfd, struct filename *from,
>                 goto exit2;
>
>         error = -EBUSY;
> -       if (old_type != LAST_NORM)
> +       if (old_type != NORMAL)
>                 goto exit2;
>
>         if (flags & RENAME_NOREPLACE)
>                 error = -EEXIST;
> -       if (new_type != LAST_NORM)
> +       if (new_type != NORMAL)
>                 goto exit2;
>
>         error = mnt_want_write(old_path.mnt);
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index 58600cf234bc..e79ff97063e9 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -16,7 +16,7 @@ enum { MAX_NESTED_LINKS = 8 };
>  /*
>   * Type of the last component on LOOKUP_PARENT
>   */
> -enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT};
> +enum component_type {NORMAL, ROOT, DOT, DOTDOT};
>

Nah, I don't think it is a good idea to poluate the global namespace
with constant as generic as NORMAL, ROOT

At the very least this needs a namespace prefix such as
NDT_ (nameidata type)

But then I don't think it is worth the churn to rename LAST_ to NDT_
so I would drop the constants name change altogether.

Thanks,
Amir.