On Tue, Jul 30, 2024 at 03:10:25PM -0400, Josef Bacik wrote:
> On Tue, Jul 30, 2024 at 01:15:54AM -0400, viro@xxxxxxxxxx wrote:
> > From: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> >
> > 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
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > needs to be discussed.
> 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...
