Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 22 Nov 2022 08:14:37 +1100
From:   Dave Chinner <david@fromorbit.com>
To:     Catherine Hoang <catherine.hoang@oracle.com>
Cc:     linux-xfs@vger.kernel.org, linux-ext4@vger.kernel.org
Subject: Re: [PATCH v2 1/2] fs: hoist get/set UUID ioctls
Message-ID: <20221121211437.GK3600936@dread.disaster.area>
References: <20221118211408.72796-1-catherine.hoang@oracle.com>
 <20221118211408.72796-2-catherine.hoang@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221118211408.72796-2-catherine.hoang@oracle.com>
Precedence: bulk

On Fri, Nov 18, 2022 at 01:14:07PM -0800, Catherine Hoang wrote:
> Hoist the EXT4_IOC_[GS]ETFSUUID ioctls so that they can be used by all
> filesystems. This allows us to have a common interface for tools such as
> coreutils.
> 
> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/ext4/ext4.h          | 13 ++-----------
>  include/uapi/linux/fs.h | 11 +++++++++++
>  2 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 8d5453852f98..b200302a3732 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -722,8 +722,8 @@ enum {
>  #define EXT4_IOC_GETSTATE		_IOW('f', 41, __u32)
>  #define EXT4_IOC_GET_ES_CACHE		_IOWR('f', 42, struct fiemap)
>  #define EXT4_IOC_CHECKPOINT		_IOW('f', 43, __u32)
> -#define EXT4_IOC_GETFSUUID		_IOR('f', 44, struct fsuuid)
> -#define EXT4_IOC_SETFSUUID		_IOW('f', 44, struct fsuuid)
> +#define EXT4_IOC_GETFSUUID		FS_IOC_GETFSUUID
> +#define EXT4_IOC_SETFSUUID		FS_IOC_SETFSUUID
>  
>  #define EXT4_IOC_SHUTDOWN _IOR ('X', 125, __u32)
>  
> @@ -753,15 +753,6 @@ enum {
>  						EXT4_IOC_CHECKPOINT_FLAG_ZEROOUT | \
>  						EXT4_IOC_CHECKPOINT_FLAG_DRY_RUN)
>  
> -/*
> - * Structure for EXT4_IOC_GETFSUUID/EXT4_IOC_SETFSUUID
> - */
> -struct fsuuid {
> -	__u32       fsu_len;
> -	__u32       fsu_flags;
> -	__u8        fsu_uuid[];
> -};
> -
>  #if defined(__KERNEL__) && defined(CONFIG_COMPAT)
>  /*
>   * ioctl commands in 32 bit emulation
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index b7b56871029c..63b925444592 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -121,6 +121,15 @@ struct fsxattr {
>  	unsigned char	fsx_pad[8];
>  };
>  
> +/*
> + * Structure for FS_IOC_GETFSUUID/FS_IOC_SETFSUUID
> + */
> +struct fsuuid {
> +	__u32       fsu_len;
> +	__u32       fsu_flags;
> +	__u8        fsu_uuid[];
> +};

As I pointed out in my last comments, flex arrays in user APIs are
really unfriendly, because it means the structures have to be
dynamically allocated and can't be put on the stack. This makes the
obvious use of the API (i.e. a local stack struct fsuuid
declaration) dangerous to users.

Also, UUIDs are 16 bytes long - always have been, always will be.
So why does this API need to support -variable length UUIDs-? If
this is intended for use with other types of filesystem IDs (e.g.
GUIDs), then it needs to be named differently and it needs to have
a man page written for it to explain what it contains....

Shouldn't these landmines get fixed before we promote the API to
being a VFS-wide operation?

Also, if this is VFS wide, then why do we need filesystem specific
implementations to retreive the UUID? After all, the VFS superblock
has the public filesystem UUID in it (i.e. sb->s_uuid), and so we
should just have a single ioctl implementation that reads out the
sb->s_uuid. Yes, Setting the UUID is a different matter altogether
because filesystems need to change on-disk stuff, but we don't need
to reimplement retreiving sb->s_uuid in every filesystem...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com