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@xxxxxxxxxx>
> Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> Reviewed-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx
