Re: [PATCH] fs: Fix typo in comment of link_path_walk()

[Al Viro <viro@zeniv.linux.org.uk>]

On Sat, May 10, 2025 at 08:44:12PM +0800, I Hsin Cheng wrote:
> On Sat, May 10, 2025 at 01:08:35PM +0100, Al Viro wrote:
> > On Sat, May 10, 2025 at 06:46:32PM +0800, I Hsin Cheng wrote:
> > > Fix "NUL" to "NULL".
> > > 
> > > Fixes: 200e9ef7ab51 ("vfs: split up name hashing in link_path_walk() into helper function")
> > 
> > Not a typo.  And think for a second about the meaning of
> > so "fixed" sentence - NUL and '/' are mutually exclusive
> > alternatives; both are characters.  NULL is a pointer and
> > makes no sense whatsoever in that context.
> 
> Ohh thanks for the head up. I'll keep this in mind, really big thanks!

FWIW, the logics could be cleaner; what happens there is that having
consumed a pathname component, we want to find two things:
	* was it the last one?
	* if not, where does the next one start?
If the next character is the end of string (NUL, zero byte), everything's
obvious - it was the last component.  If not, the next character must
be '/' (anything else would've been a part of component we've just read
through), and usually that would mean that there are other components
coming after it.  However, there might be more than one slash (/usr//bin
means the same thing as /usr/bin), so we need to skip however many slashes
there might be in order to find the beginning of the next component.
What's more, /usr/bin/ is also valid and means the same thing as /usr/bin,
so it's still possible that component we've read was the last one - we
won't know until we skip all slashes and see if we've reached the end
of string.

A possible way to clarify that might be something like

/* given a pointer just past the end of component find the next one */
static inline const char *next_component(const char *s)
{
	if (!*s)		// end of string
		return NULL;
	// the only other thing possible here is '/'; skip it, along with
	// any extra slashes (if any - rare, but legal) right after it.
	do {
		s++;
	} while (unlikely(*s == '/'));
	if (unlikely(!*s))	// slash(es), then end of string
		return NULL;
	return s;		// beginning of the next component
}

with
		name = next_component(name);
		if (!name) {
			/* we are at the end of... */
                        if (!depth) {
				/* ... the pathname or trailing symlink; done */
                                nd->dir_vfsuid = i_uid_into_vfsuid(idmap, nd->inode);
                                nd->dir_mode = nd->inode->i_mode;
                                nd->flags &= ~LOOKUP_PARENT;
                                return 0;
                        }
                        /* ... the last component of nested symlink */
                        name = nd->stack[--depth].name;
                        link = walk_component(nd, 0);
		} else {
			/* not the last component */
			link = walk_component(nd, WALK_MORE);
		}

in the corresponding place in link_path_walk().  Not sure if it's better
that way, to be honest - taking that chunk into an inline helper would
force reader to go looking for the definition of that helper and that
just might be more disruptive than the current variant.  Hell knows...