On Thu, Jun 12, 2025 at 5:11 PM NeilBrown <neil@xxxxxxxxxx> wrote: [...] > > + > > +false_out: > > + path_put(path); > > + memset(path, 0, sizeof(*path)); > > + return false; > > +} > > I think the public function should return 0 on success and -error on > failure. That is a well established pattern. Yeah, I think we can use this pattern. > I also think you > shouldn't assume that all callers will want the same flags. __path_walk_parent() only handles two LOOKUP_ flags, so it is a bit weird to allow all the flags. But if folks think this is a good idea, I don't have strong objections to taking various flags. > > And it isn't clear to me why you want to path_put() on failure. In earlier versions, we would keep "path" unchanged when the walk stopped. However, this is not the case in this version (choose_mountpoint() => in_root => return -EXDEV). So I decided to just release it, so that we will not leak a path that the walk should not get to. > > I wonder if there might be other potential users in the kernel. > If so we should consider how well the interface meets their needs. > > autofs, devpts, nfsd, landlock all call follow_up... > maybe they should be using the new interface... > nfsd is the most likely to benefit - particularly nfsd_lookup_parent(). AFAICT, autofs and devpts can just use follow_up(). For nfsd, nfsd_lookup_parent() and nfsd4_encode_pathname4() can use path_walk_parent. And 2/5 covers landlock. I think we can update nfsd in a follow up patch, just to keep this set simpler. Thanks, Song > Just a thought.. [...]
