Received-SPF: pass (google.com: domain of linux-kernel+bounces-61719-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 12 Feb 2024 08:39:29 -0500
From: Kent Overstreet <kent.overstreet@linux.dev>
To: Brian Foster <bfoster@redhat.com>
Cc: Dave Chinner <david@fromorbit.com>, brauner@kernel.org, 
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, Jan Kara <jack@suse.cz>, 
	Dave Chinner <dchinner@redhat.com>, "Darrick J. Wong" <djwong@kernel.org>, 
	Theodore Ts'o <tytso@mit.edu>
Subject: Re: [PATCH v2 3/7] fs: FS_IOC_GETUUID
Message-ID: <dfnric7xlns3u2hc4s4zfeotxpksmvnkvr5dv7hogvothdb3un@b5icg6irxyr5>
References: <20240206201858.952303-1-kent.overstreet@linux.dev>
 <20240206201858.952303-4-kent.overstreet@linux.dev>
 <ZcKsIbRRfeXfCObl@dread.disaster.area>
 <cm4wbdmpuq6mlyfqrb3qqwyysa3qao6t5sc2eq3ykmgb4ptpab@qkyberqtvrtt>
 <ZcN+8iOBR97t451x@bfoster>
 <krc2udjtkvylugzuledk7hre7rizmiajrgkiwvwcmsxtgxobyz@miqndphw7uhi>
 <ZcoTROgZiKOfp3iM@bfoster>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZcoTROgZiKOfp3iM@bfoster>

On Mon, Feb 12, 2024 at 07:47:00AM -0500, Brian Foster wrote:
> 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?"

If you feel this affected by this, how are you going to cope with Rust?