Re: [PATCH 11/19] switch simple users of fdget() to CLASS(fd, ...)

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

On Fri, Jun 07, 2024 at 05:10:43PM +0100, Al Viro wrote:
> On Fri, Jun 07, 2024 at 05:26:53PM +0200, Christian Brauner wrote:
> > On Fri, Jun 07, 2024 at 02:59:49AM +0100, Al Viro wrote:
> > > low-hanging fruit; that's another likely source of conflicts
> > > over the cycle and it might make a lot of sense to split;
> > > fortunately, it can be split pretty much on per-function
> > > basis - chunks are independent from each other.
> > > 
> > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > > ---
> > 
> > I can pick conversions from you for files where I already have changes
> > in the tree anyway or already have done conversion as part of other
> > patches.
> 
> Some notes:
> 
> 	This kind of conversions depends upon fdput(empty) being not just
> a no-op, but a no-op visible to compiler.  Representation change arranges
> for that.
> 
> 	CLASS(...) has some misfeatures or nearly C++ level of tastelessness;
> for this series I decided to try it and see how it goes, but... AFAICS in
> a lot of cases it's the wrong answer.
> 
> 	1.  Variable declarations belong in the beginning of block.
> CLASS use invites violating that; to make things worse, in-block goto
> bypassing such declaration is only caught by current clang.  Two
> examples got caught by Intel buildbot in this patch, actually - one
> in fs/select.c, another in ocfs2.

Considerably more than two, actually.  For example, this
        inode_lock(inode);
        /* Update mode */
        ovl_copyattr(inode);
        ret = file_remove_privs(file);
        if (ret)
                goto out_unlock;

        CLASS(fd_real, real)(file);
        if (fd_empty(real)) {
                ret = fd_error(real);
                goto out_unlock;
        }

        old_cred = ovl_override_creds(file_inode(file)->i_sb);
        ret = vfs_fallocate(fd_file(real), mode, offset, len);
        revert_creds(old_cred);

        /* Update size */
        ovl_file_modified(file);

out_unlock:
        inode_unlock(inode);

steps into the same problem.

Hell knows - it feels like mixing __cleanup-based stuff with anything
explicit leads to massive headache.  And I *really* hate to have
e.g. inode_unlock() hidden in __cleanup in a random subset of places.
Unlike dropping file references (if we do that a bit later, nothing
would really care), the loss of explicit control over the places where
inode lock is dropped is asking for serious trouble.

Any suggestions?  Linus, what's your opinion on the use of CLASS...
stuff?