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

[Jori Koolstra]

> Op 20-04-2026 12:29 CEST schreef Amir Goldstein <amir73il@xxxxxxxxx>:
> 
> Please update documentation entry for last_type in
> Documentation/filesystems/path-lookup.rst
> 

Yes, thanks for the reminder!

> > -       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;
> 
> I really dislike all this white space churn just because the enum name
> is so long. If it was long for a good reason - increasing readability -
> I may have conceded, but I don't think that is the case.
> 
> I do not think that 'component_type' is more clear than 'last_type'
> for a developer passing by reading the code.
> If anything, the opposite, because 'last_type' or 'type' variables always
> appear in the same function with 'last' variable, so it is more clear WHICH
> object the type is referring to and matches the LAST_ enum prefix.

OK, I see your point. I have changed it back. The reason I preferred
component_type is that the whole "last" thing is just incidental. There is
nothing dependent on these components being "last", it's just that the
enum values are only used in that context. It would make more sense to
restrict the enum to namei.c and use NORMAL, DOT, or something like that.
But I do agree that isn't worth the churn :) 

> 
> > +       unsigned                depth;
> > +       int                     total_link_count;
> >         struct saved {
> >                 struct path link;
> >                 struct delayed_call done;
> > @@ -2221,7 +2221,7 @@ 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) {
> >                 const char *error = NULL;
> > @@ -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,7 +2963,8 @@ 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)
> > @@ -3009,7 +3010,8 @@ 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)
> > @@ -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,
> > @@ -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);
> > @@ -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:
> > @@ -5383,6 +5385,7 @@ int filename_rmdir(int dfd, struct filename *name)
> >         case LAST_ROOT:
> >                 error = -EBUSY;
> >                 goto exit2;
> > +       case LAST_NORM: ; // OK
> 
> I was not aware that the compiler warns about missing cases for enums -
> cool feature.
> but that needs to be a break; rather than fallthough the end
> and as a matter of taste, I would put the valid case first - not critical.
> 
> >         }
> >
> >         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;
> > @@ -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;
> > diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
> > index d08973b288e5..b12d481f5ba9 100644
> > --- a/fs/smb/server/vfs.c
> > +++ b/fs/smb/server/vfs.c
> > @@ -56,7 +56,8 @@ static int ksmbd_vfs_path_lookup(struct ksmbd_share_config *share_conf,
> >  {
> >         struct qstr last;
> >         const struct path *root_share_path = &share_conf->vfs_path;
> > -       int err, type;
> > +       int err;
> > +       enum component_type type;
> >         struct dentry *d;
> >
> >         if (pathname[0] == '\0') {
> > @@ -668,7 +669,7 @@ int ksmbd_vfs_rename(struct ksmbd_work *work, const struct path *old_path,
> >         struct renamedata rd;
> >         struct ksmbd_share_config *share_conf = work->tcon->share_conf;
> >         struct ksmbd_file *parent_fp;
> > -       int new_type;
> > +       enum component_type new_type;
> >         int err, lookup_flags = LOOKUP_NO_SYMLINKS;
> >
> >         if (ksmbd_override_fsids(work))
> > diff --git a/include/linux/namei.h b/include/linux/namei.h
> > index 58600cf234bc..fa3ae87762b7 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 {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT};
> 
> Please use last_type to match the LAST_ prefix.
> 
> TBH, the thing that bothers me most is why is this enum exposed
> outside of namei.c
> in the first place?
> 
> >
> >  /* pathwalk mode */
> >  #define LOOKUP_FOLLOW          BIT(0)  /* follow links at the end */
> > @@ -70,7 +70,7 @@ static inline void end_removing_path(const struct path *path , struct dentry *de
> >         end_creating_path(path, dentry);
> >  }
> >  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);
> 
> As your patch demonstrates, the only use of enum last_type outside of namei.c is
> in ksmbd calls to vfs_path_parent_lookup().
> 
> This is an "internal" lookup helper that was exposed by commit
> 74d7970febf7e ("ksmbd: fix racy issue from using ->d_parent and ->d_name")
> 
> This commit appears to be using the helper inconsistently in
> its two call sites, because ksmbd_vfs_rename(), is not checking the
> unlikely(type != LAST_NORM) case.
> 
> Perhaps it would be wiser not to expose enum last_type at all and move
> the unlikely(type != LAST_NORM) check into the helper itself in namei.c.
> 
> WDYT?
> 

Agreed. See my email to Al in this thread.

> Thanks,
> Amir.

Thanks,
Jori.