[v2,3/7] fs: FS_IOC_GETUUID

  Add a new generic ioctls for querying the filesystem UUID.

These are lifted versions of the ext4 ioctls, with one change: we're not
using a flexible array member, because UUIDs will never be more than 16
bytes.

This patch adds a generic implementation of FS_IOC_GETFSUUID, which
reads from super_block->s_uuid. We're not lifting SETFSUUID from ext4 -
that can be done on offline filesystems by the people who need it,
trying to do it online is just asking for too much trouble.

Cc: Christian Brauner <brauner@kernel.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Dave Chinner <dchinner@redhat.com>
Cc: "Darrick J. Wong" <djwong@kernel.org>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: linux-fsdevel@vger.kernel.or
Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
---
 fs/ioctl.c              | 16 ++++++++++++++++
 include/uapi/linux/fs.h | 17 +++++++++++++++++
 2 files changed, 33 insertions(+)
  

On 2/6/24 12:18, Kent Overstreet wrote:
> Add a new generic ioctls for querying the filesystem UUID.
> 
> These are lifted versions of the ext4 ioctls, with one change: we're not
> using a flexible array member, because UUIDs will never be more than 16
> bytes.
> 
> This patch adds a generic implementation of FS_IOC_GETFSUUID, which
> reads from super_block->s_uuid. We're not lifting SETFSUUID from ext4 -
> that can be done on offline filesystems by the people who need it,
> trying to do it online is just asking for too much trouble.
> 
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Dave Chinner <dchinner@redhat.com>
> Cc: "Darrick J. Wong" <djwong@kernel.org>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: linux-fsdevel@vger.kernel.or
> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> ---
>  fs/ioctl.c              | 16 ++++++++++++++++
>  include/uapi/linux/fs.h | 17 +++++++++++++++++
>  2 files changed, 33 insertions(+)
> 


> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index 48ad69f7722e..16a6ecadfd8d 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -64,6 +64,19 @@ struct fstrim_range {
>  	__u64 minlen;
>  };
>  
> +/*
> + * We include a length field because some filesystems (vfat) have an identifier
> + * that we do want to expose as a UUID, but doesn't have the standard length.
> + *
> + * We use a fixed size buffer beacuse this interface will, by fiat, never

                                 because

> + * support "UUIDs" longer than 16 bytes; we don't want to force all downstream
> + * users to have to deal with that.
> + */
> +struct fsuuid2 {
> +	__u8	len;
> +	__u8	uuid[16];
> +};
> +
>  /* extent-same (dedupe) ioctls; these MUST match the btrfs ioctl definitions */
>  #define FILE_DEDUPE_RANGE_SAME		0
>  #define FILE_DEDUPE_RANGE_DIFFERS	1
> @@ -190,6 +203,9 @@ struct fsxattr {
>   * (see uapi/linux/blkzoned.h)
>   */
>  
> +/* Returns the external filesystem UUID, the same one blkid returns */
> +#define FS_IOC_GETFSUUID		_IOR(0x12, 142, struct fsuuid2)
> +
>  #define BMAP_IOCTL 1		/* obsolete - kept for compatibility */
>  #define FIBMAP	   _IO(0x00,1)	/* bmap access */
>  #define FIGETBSZ   _IO(0x00,2)	/* get the block size used for bmap */
> @@ -198,6 +214,7 @@ struct fsxattr {
>  #define FITRIM		_IOWR('X', 121, struct fstrim_range)	/* Trim */
>  #define FICLONE		_IOW(0x94, 9, int)
>  #define FICLONERANGE	_IOW(0x94, 13, struct file_clone_range)
> +
>  #define FIDEDUPERANGE	_IOWR(0x94, 54, struct file_dedupe_range)

Why the additional blank line? (nit)

>  
>  #define FSLABEL_MAX 256	/* Max chars for the interface; each fs may differ */
  

On Tue, Feb 06, 2024 at 03:18:51PM -0500, Kent Overstreet wrote:
> Add a new generic ioctls for querying the filesystem UUID.
> 
> These are lifted versions of the ext4 ioctls, with one change: we're not
> using a flexible array member, because UUIDs will never be more than 16
> bytes.
> 
> This patch adds a generic implementation of FS_IOC_GETFSUUID, which
> reads from super_block->s_uuid. We're not lifting SETFSUUID from ext4 -
> that can be done on offline filesystems by the people who need it,
> trying to do it online is just asking for too much trouble.
> 
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Dave Chinner <dchinner@redhat.com>
> Cc: "Darrick J. Wong" <djwong@kernel.org>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: linux-fsdevel@vger.kernel.or
> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> ---
>  fs/ioctl.c              | 16 ++++++++++++++++
>  include/uapi/linux/fs.h | 17 +++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 76cf22ac97d7..046c30294a82 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -763,6 +763,19 @@ static int ioctl_fssetxattr(struct file *file, void __user *argp)
>  	return err;
>  }
>  
> +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...

> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index 48ad69f7722e..16a6ecadfd8d 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -64,6 +64,19 @@ struct fstrim_range {
>  	__u64 minlen;
>  };
>  
> +/*
> + * We include a length field because some filesystems (vfat) have an identifier
> + * that we do want to expose as a UUID, but doesn't have the standard length.
> + *
> + * We use a fixed size buffer beacuse this interface will, by fiat, never
> + * support "UUIDs" longer than 16 bytes; we don't want to force all downstream
> + * users to have to deal with that.
> + */
> +struct fsuuid2 {
> +	__u8	len;
> +	__u8	uuid[16];
> +};
> +
>  /* extent-same (dedupe) ioctls; these MUST match the btrfs ioctl definitions */
>  #define FILE_DEDUPE_RANGE_SAME		0
>  #define FILE_DEDUPE_RANGE_DIFFERS	1
> @@ -190,6 +203,9 @@ struct fsxattr {
>   * (see uapi/linux/blkzoned.h)
>   */
>  
> +/* Returns the external filesystem UUID, the same one blkid returns */
> +#define FS_IOC_GETFSUUID		_IOR(0x12, 142, struct fsuuid2)
> +

Can you add a comment somewhere in the file saying that new VFS
ioctls should use the "0x12" namespace in the range 142-255, and
mention that BLK ioctls should be kept within the 0x12 {0-141}
range?

Probably also document this clearly in
Documentation/userspace-api/ioctl/ioctl-number.rst, too?

-Dave.
  

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).

But I won't push that in this patch, we can just keep the style
consistent for now.

> > +/* Returns the external filesystem UUID, the same one blkid returns */
> > +#define FS_IOC_GETFSUUID		_IOR(0x12, 142, struct fsuuid2)
> > +
> 
> Can you add a comment somewhere in the file saying that new VFS
> ioctls should use the "0x12" namespace in the range 142-255, and
> mention that BLK ioctls should be kept within the 0x12 {0-141}
> range?

Well, if we're going to try to keep the BLK_ and FS_IOC_ ioctls in
separate ranges, then FS_IOC_ needs to move to something else becasue
otherwise BLK_ won't have a way to expand.

So what else -

ioctl-number.rst has a bunch of other ranges listed for fs.h, but 0x12
appears to be the only one without conflicts - all the other ranges seem
to have originated with other filesystems.

So perhaps I will take Darrick's nak (0x15) suggestion after all.
  

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).
> 
> But I won't push that in this patch, we can just keep the style
> consistent for now.
> 
> > > +/* Returns the external filesystem UUID, the same one blkid returns */
> > > +#define FS_IOC_GETFSUUID		_IOR(0x12, 142, struct fsuuid2)
> > > +
> > 
> > Can you add a comment somewhere in the file saying that new VFS
> > ioctls should use the "0x12" namespace in the range 142-255, and
> > mention that BLK ioctls should be kept within the 0x12 {0-141}
> > range?
> 
> Well, if we're going to try to keep the BLK_ and FS_IOC_ ioctls in
> separate ranges, then FS_IOC_ needs to move to something else becasue
> otherwise BLK_ won't have a way to expand.

The BLK range can grow downwards towards zero, I think. It starts at
93 and goes to 136, so there's heaps of space for it to grow from 92
to 0....

> So what else -
> 
> ioctl-number.rst has a bunch of other ranges listed for fs.h, but 0x12
> appears to be the only one without conflicts - all the other ranges seem
> to have originated with other filesystems.

*nod*

> So perhaps I will take Darrick's nak (0x15) suggestion after all.

That works, too.

-Dave.
  

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?

Brian

> But I won't push that in this patch, we can just keep the style
> consistent for now.
> 
> > > +/* Returns the external filesystem UUID, the same one blkid returns */
> > > +#define FS_IOC_GETFSUUID		_IOR(0x12, 142, struct fsuuid2)
> > > +
> > 
> > Can you add a comment somewhere in the file saying that new VFS
> > ioctls should use the "0x12" namespace in the range 142-255, and
> > mention that BLK ioctls should be kept within the 0x12 {0-141}
> > range?
> 
> Well, if we're going to try to keep the BLK_ and FS_IOC_ ioctls in
> separate ranges, then FS_IOC_ needs to move to something else becasue
> otherwise BLK_ won't have a way to expand.
> 
> So what else -
> 
> ioctl-number.rst has a bunch of other ranges listed for fs.h, but 0x12
> appears to be the only one without conflicts - all the other ranges seem
> to have originated with other filesystems.
> 
> So perhaps I will take Darrick's nak (0x15) suggestion after all.
>
  

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.

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 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.
  

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
  

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?
  

On Mon, Feb 12, 2024 at 08:39:29AM -0500, Kent Overstreet wrote:
> 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?
> 

Well I'm still a Rust newbie, but I've been exposed to some of the basic
syntax and semantics and it hasn't been a problem yet. I'll keep my
fingers crossed, I guess.

Brian