Re: [PATCH v2 3/7] fs: FS_IOC_GETUUID

[Brian Foster <bfoster@redhat.com>]

On Thu, Feb 08, 2024 at 04:57:02PM -0500, Kent Overstreet wrote:
> On Wed, Feb 07, 2024 at 08:05:29AM -0500, Brian Foster wrote:
> > On Tue, Feb 06, 2024 at 05:37:22PM -0500, Kent Overstreet wrote:
> > > On Wed, Feb 07, 2024 at 09:01:05AM +1100, Dave Chinner wrote:
> > > > On Tue, Feb 06, 2024 at 03:18:51PM -0500, Kent Overstreet wrote:
> > > > > +static int ioctl_getfsuuid(struct file *file, void __user *argp)
> > > > > +{
> > > > > +	struct super_block *sb = file_inode(file)->i_sb;
> > > > > +
> > > > > +	if (!sb->s_uuid_len)
> > > > > +		return -ENOIOCTLCMD;
> > > > > +
> > > > > +	struct fsuuid2 u = { .len = sb->s_uuid_len, };
> > > > > +	memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len);
> > > > > +
> > > > > +	return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0;
> > > > > +}
> > > > 
> > > > Can we please keep the declarations separate from the code? I always
> > > > find this sort of implicit scoping of variables both difficult to
> > > > read (especially in larger functions) and a landmine waiting to be
> > > > tripped over. This could easily just be:
> > > > 
> > > > static int ioctl_getfsuuid(struct file *file, void __user *argp)
> > > > {
> > > > 	struct super_block *sb = file_inode(file)->i_sb;
> > > > 	struct fsuuid2 u = { .len = sb->s_uuid_len, };
> > > > 
> > > > 	....
> > > > 
> > > > and then it's consistent with all the rest of the code...
> > > 
> > > The way I'm doing it here is actually what I'm transitioning my own code
> > > to - the big reason being that always declaring variables at the tops of
> > > functions leads to separating declaration and initialization, and worse
> > > it leads people to declaring a variable once and reusing it for multiple
> > > things (I've seen that be a source of real bugs too many times).
> > > 
> > 
> > I still think this is of questionable value. I know I've mentioned
> > similar concerns to Dave's here on the bcachefs list, but still have not
> > really seen any discussion other than a bit of back and forth on the
> > handful of generally accepted (in the kernel) uses of this sort of thing
> > for limiting scope in loops/branches and such.
> > 
> > I was skimming through some more recent bcachefs patches the other day
> > (the journal write pipelining stuff) where I came across one or two
> > medium length functions where this had proliferated, and I found it kind
> > of annoying TBH. It starts to almost look like there are casts all over
> > the place and it's a bit more tedious to filter out logic from the
> > additional/gratuitous syntax, IMO.
> > 
> > That's still just my .02, but there was also previous mention of
> > starting/having discussion on this sort of style change. Is that still
> > the plan? If so, before or after proliferating it throughout the
> > bcachefs code? ;) I am curious if there are other folks in kernel land
> > who think this makes enough sense that they'd plan to adopt it. Hm?
> 
> That was the discussion :)
> 
> bcachefs is my codebase, so yes, I intend to do it there. I really think
> this is an instance where you and Dave are used to the way C has
> historically forced us to do things; our brains get wired to read code a
> certain way and changes are jarring.
> 

Heh, fair enough. That's certainly your prerogative. I'm certainly not
trying to tell you what to do or not with bcachefs. That's at least
direct enough that it's clear it's not worth debating too much. ;)

> But take a step back; if we were used to writing code the way I'm doing
> it, and you were arguing for putting declarations at the tops of
> functions, what would the arguments be?
> 

I think my thought process would be similar. I.e., is the proposed
benefit of such a change worth the tradeoffs?

> I would say you're just breaking up the flow of ideas for no reason; a
> chain of related statements now includes a declaration that isn't with
> the actual logic.
> 
> And bugs due to variable reuse, missed initialization - there's real
> reasons not to do it that way.
> 

And were I in that position, I don't think I would reduce a decision
that affects readability/reviewability of my subsystem to a nontrivial
degree (for other people, at least) to that single aspect. This would be
the answer to the question: "is this worth considering?"

Brian