On Mon, Jan 16, 2023 at 08:18:07PM +0000, Al Viro wrote: > On Fri, Jan 13, 2023 at 02:05:56PM +0400, Konstantin Komarov wrote: > > > Hello. > > > > We have added a patch with this check just before the New Year. (here https://lore.kernel.org/lkml/ee705b24-865b-26ff-157d-4cb2a303a962@xxxxxxxxxxxxxxxxxxxx/) > > See upthread for the reasons why that's wrong. Incidentally, > mixing logical change with a pile of whitespace changes is > bad idea - it's very easy for reviewers to miss... > > Other observation from the cursory look through your namei.c: > ntfs_create_inode() has no reason to return inode; the reference > it creates goes into dentry. Make it return int, the callers will > be happier. While we are at it, use d_instantiate_new() instead > of d_instantiate() + unlock_new_inode() there. > > Incidentally, control flow in there is harder to follow that it > needs to be: > * everything that reaches out{3,4,5,6,7} is guaranteed > to have err != 0; > * fallthrough into out2 is guaranteed to have err != 0; > direct branch to it - err == 0. > * direct branch to out1 is guaranteed to have err != 0. > > I would suggest something along the lines of the following (completely > untested) delta; the callers are clearly better off that way and > failure paths are separated from the success one - they didn't share > anywhere near enough to have it worth bothering. While we are at it - what's the point passing the symlink body length to ntfs_create_inode()? We could calculate it there just as well - it's used only for symlinks (unsurprisingly) and you've got uncomfortably many arguments as it is...
