On Tue, Jul 30, 2024 at 10:12:25PM +0100, Al Viro wrote:

On Tue, Jul 30, 2024 at 03:10:25PM -0400, Josef Bacik wrote:

quoted

On Tue, Jul 30, 2024 at 01:15:54AM -0400, viro@kernel.org wrote:

quoted

From: Al Viro <viro@zeniv.linux.org.uk>

There are four places where we end up adding an extra scope
covering just the range from constructor to destructor;
not sure if that's the best way to handle that.

The functions in question are ovl_write_iter(), ovl_splice_write(),
ovl_fadvise() and ovl_copyfile().

This is very likely *NOT* the final form of that thing - it

    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

quoted

quoted

needs to be discussed.

Fair, I think I misunderstood what you were unhappy with in that code.

quoted

Is this what we want to do from a code cleanliness standpoint?  This feels
pretty ugly to me, I feal like it would be better to have something like

scoped_class(fd_real, real) {
	// code
}

rather than the {} at the same indent level as the underlying block.

I don't feel super strongly about this, but I do feel like we need to either
explicitly say "this is the way/an acceptable way to do this" from a code
formatting standpoint, or we need to come up with a cleaner way of representing
the scoped area.

That's a bit painful in these cases - sure, we can do something like
	scoped_class(fd_real, real)(file) {
		if (fd_empty(fd_real)) {
			ret = fd_error(real);
			break;
		}
		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);  
	}
but that use of break would need to be documented.  And IMO anything like
        scoped_cond_guard (mutex_intr, return -ERESTARTNOINTR,
			   &task->signal->cred_guard_mutex) {
is just distasteful ;-/  Control flow should _not_ be hidden that way;
it's hard on casual reader.

The variant I'd put in there is obviously not suitable for merge - we need
something else, the question is what that something should be...

I went and looked at our c++ codebase to see what they do here, and it appears
that this is the accepted norm for this style of scoped variables

{
	CLASS(fd_real, real_out)(file_out);
	// blah blah
}

Looking at our code guidelines this appears to be the widely accepted norm, and
I don't hate it.  I feel like this is more readable than the scoped_class()
idea, and is honestly the cleanest solution.  Thanks,

Josef