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

[Jori Koolstra]

Hi Al,

> Op 22-04-2026 05:39 CEST schreef Al Viro <viro@xxxxxxxxxxxxxxxxxx>:
> 
>  
> On Sun, Apr 19, 2026 at 06:16:16PM +0200, Jori Koolstra 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.
> 
> To find _what_, again?
> 
> > I would argue
> > that the readability of the code is improved by making this an explicit
> > type.
> 
> Do argue, then.  How does that improve readability?

It is certainly in part a matter of taste, so I certainly won't say this
is "wrong."

There is the matter of type safety, and the (possible) benefits this entails
such as automatic case coverage checking for switch-statements. However, I don't
doubt for a second you aren't aware of this, so that probably won't convince
you :) . We indeed also don't do this for flag-types, but at least that
is a kernel-wide established pattern (now I am curious how the Rust people
deal with flags).

What I find somewhat unfortunate is that if I am looking at
__filename_parentat(), for instance, the signature does not give me any
clue what the purpose is of int *type. Then I look through the code and see

		*type = nd.last_type;

and I have to jump to struct nameidata and it still tells me nothing. I have
to grep last_type in nameidata to see it is used in conjunction with the 
anonymous enum.

I can only tell that it would have slightly decreased cognitive load and jumping
around for me when I first looked into namei.c. How that compares to some churn I
can't say.

> 
> What I see is a lot of churn.  Incidentally, I'm not at all sure that we
> should expose those outside of fs/namei.c - if you look at the sole place
> where any of those are used anywhere else you'll see this:
> 	err = vfs_path_parent_lookup(filename, flags,
> 				     path, &last, &type,
> 				     root_share_path);
> 	if (err)
> 		return err;
> 
> 	if (unlikely(type != LAST_NORM)) {
> 		path_put(path);
> 		return -ENOENT;
> 	}
> with the only other caller of vfs_path_parent_lookup() being
> 	err = vfs_path_parent_lookup(to, lookup_flags | LOOKUP_BENEATH,
> 				     &new_path, &new_last, &new_type,
> 				     &share_conf->vfs_path);
> 	if (err)
> 		goto out1;
> and having no uses of new_type whatsoever.  Smells like missing
> check _and_ wrong calling conventions...
> 
> Do we ever want to allow anything other thant LAST_NORM in any of
> the users?

I agree with you and Amir. The naming is also a bit confusing. Maybe, I am
confusing myself but path_parentat() seems to return the parent component of the
pathname passed into nd, not the parent of the resolved path, and these do not
have to be the same if last_type isn't normal. I presume that is why they put
that check into vfs.c.

So what do you think is the best place to move 

 	if (unlikely(type != LAST_NORM)) {
 		path_put(path);
 		return -ENOENT;
 	}

to? path_parentat() or filename_parentat()?

And thanks a lot for taking the time to review, Al, I appreciate it :)

Thanks,
Jori.