Message ID | 20221118211408.72796-3-catherine.hoang@oracle.com |
---|---|
State | Not Applicable |
Headers | show |
Series | porting the GETFSUUID ioctl to xfs | expand |
On Fri, Nov 18, 2022 at 01:14:08PM -0800, Catherine Hoang wrote: > Add a new ioctl to retrieve the UUID of a mounted xfs filesystem. This is a > precursor to adding the SETFSUUID ioctl. > > Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com> > Reviewed-by: Allison Henderson <allison.henderson@oracle.com> LGTM, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/xfs_ioctl.c | 36 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index 1f783e979629..cf77715afe9e 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -1865,6 +1865,39 @@ xfs_fs_eofblocks_from_user( > return 0; > } > > +static int > +xfs_ioctl_getuuid( > + struct xfs_mount *mp, > + struct fsuuid __user *ufsuuid) > +{ > + struct fsuuid fsuuid; > + __u8 uuid[UUID_SIZE]; > + > + if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid))) > + return -EFAULT; > + > + if (fsuuid.fsu_len == 0) { > + fsuuid.fsu_len = UUID_SIZE; > + if (copy_to_user(&ufsuuid->fsu_len, &fsuuid.fsu_len, > + sizeof(fsuuid.fsu_len))) > + return -EFAULT; > + return 0; > + } > + > + if (fsuuid.fsu_len < UUID_SIZE || fsuuid.fsu_flags != 0) > + return -EINVAL; > + > + spin_lock(&mp->m_sb_lock); > + memcpy(uuid, &mp->m_sb.sb_uuid, UUID_SIZE); > + spin_unlock(&mp->m_sb_lock); > + > + fsuuid.fsu_len = UUID_SIZE; > + if (copy_to_user(ufsuuid, &fsuuid, sizeof(fsuuid)) || > + copy_to_user(&ufsuuid->fsu_uuid[0], uuid, UUID_SIZE)) > + return -EFAULT; > + return 0; > +} > + > /* > * These long-unused ioctls were removed from the official ioctl API in 5.17, > * but retain these definitions so that we can log warnings about them. > @@ -2153,6 +2186,9 @@ xfs_file_ioctl( > return error; > } > > + case FS_IOC_GETFSUUID: > + return xfs_ioctl_getuuid(mp, arg); > + > default: > return -ENOTTY; > } > -- > 2.25.1 >
On Fri, Nov 18, 2022 at 01:14:08PM -0800, Catherine Hoang wrote: > Add a new ioctl to retrieve the UUID of a mounted xfs filesystem. This is a > precursor to adding the SETFSUUID ioctl. > > Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com> > Reviewed-by: Allison Henderson <allison.henderson@oracle.com> > --- > fs/xfs/xfs_ioctl.c | 36 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index 1f783e979629..cf77715afe9e 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -1865,6 +1865,39 @@ xfs_fs_eofblocks_from_user( > return 0; > } > > +static int > +xfs_ioctl_getuuid( > + struct xfs_mount *mp, > + struct fsuuid __user *ufsuuid) > +{ > + struct fsuuid fsuuid; > + __u8 uuid[UUID_SIZE]; uuid_t, please, not an open coded uuid_t. > + > + if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid))) > + return -EFAULT; I still think this failing to copy the flex array member and then having to declare a local uuid buffer is an ugly wart, not just on the API side of things. > + if (fsuuid.fsu_len == 0) { > + fsuuid.fsu_len = UUID_SIZE; XFS uses sizeof(uuid_t) for the size of it's uuids, not UUID_SIZE. > + if (copy_to_user(&ufsuuid->fsu_len, &fsuuid.fsu_len, > + sizeof(fsuuid.fsu_len))) > + return -EFAULT; > + return 0; > + } > + > + if (fsuuid.fsu_len < UUID_SIZE || fsuuid.fsu_flags != 0) > + return -EINVAL; > + > + spin_lock(&mp->m_sb_lock); > + memcpy(uuid, &mp->m_sb.sb_uuid, UUID_SIZE); > + spin_unlock(&mp->m_sb_lock); Hmmmm. Shouldn't we be promoting xfs_fs_get_uuid() to xfs_super.c (without the pNFS warning!) and calling that here, rather than open coding another "get the XFS superblock UUID" function here? i.e. if (fsuuid.fsu_flags != 0) return -EINVAL; error = xfs_fs_get_uuid(&mp->m_sb, uuid, &fsuuid.fsu_len, NULL); if (error) return -EINVAL; Also, uuid_copy()? Cheers, Dave.
On Tue, Nov 22, 2022 at 08:02:23AM +1100, Dave Chinner wrote: > On Fri, Nov 18, 2022 at 01:14:08PM -0800, Catherine Hoang wrote: > > Add a new ioctl to retrieve the UUID of a mounted xfs filesystem. This is a > > precursor to adding the SETFSUUID ioctl. > > > > Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com> > > Reviewed-by: Allison Henderson <allison.henderson@oracle.com> > > --- > > fs/xfs/xfs_ioctl.c | 36 ++++++++++++++++++++++++++++++++++++ > > 1 file changed, 36 insertions(+) > > > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > > index 1f783e979629..cf77715afe9e 100644 > > --- a/fs/xfs/xfs_ioctl.c > > +++ b/fs/xfs/xfs_ioctl.c > > @@ -1865,6 +1865,39 @@ xfs_fs_eofblocks_from_user( > > return 0; > > } > > > > +static int > > +xfs_ioctl_getuuid( > > + struct xfs_mount *mp, > > + struct fsuuid __user *ufsuuid) > > +{ > > + struct fsuuid fsuuid; > > + __u8 uuid[UUID_SIZE]; > > uuid_t, please, not an open coded uuid_t. > > > + > > + if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid))) > > + return -EFAULT; > > I still think this failing to copy the flex array member and then > having to declare a local uuid buffer is an ugly wart, not just on > the API side of things. > > > + if (fsuuid.fsu_len == 0) { > > + fsuuid.fsu_len = UUID_SIZE; > > XFS uses sizeof(uuid_t) for the size of it's uuids, not UUID_SIZE. > > > + if (copy_to_user(&ufsuuid->fsu_len, &fsuuid.fsu_len, > > + sizeof(fsuuid.fsu_len))) > > + return -EFAULT; > > + return 0; > > + } > > + > > + if (fsuuid.fsu_len < UUID_SIZE || fsuuid.fsu_flags != 0) > > + return -EINVAL; > > + > > + spin_lock(&mp->m_sb_lock); > > + memcpy(uuid, &mp->m_sb.sb_uuid, UUID_SIZE); > > + spin_unlock(&mp->m_sb_lock); > > Hmmmm. Shouldn't we be promoting xfs_fs_get_uuid() to xfs_super.c > (without the pNFS warning!) and calling that here, rather than open > coding another "get the XFS superblock UUID" function here? I disagree that it's worth the effort to create a helper function to wrap four lines of code, particularly since the pnfs code has this extra weird wart of returning the byte offset(?) of the uuid location. > i.e. > if (fsuuid.fsu_flags != 0) > return -EINVAL; > > error = xfs_fs_get_uuid(&mp->m_sb, uuid, &fsuuid.fsu_len, NULL); > if (error) > return -EINVAL; > > Also, uuid_copy()? Why does xfs_fs_get_uuid use memcpy then? Did the compiler reject the u8* -> uuid_t * type conversion? Alternately there's export_uuid(). --D > Cheers, > > Dave. > > -- > Dave Chinner > david@fromorbit.com
On Mon, Nov 21, 2022 at 02:18:47PM -0800, Darrick J. Wong wrote: > On Tue, Nov 22, 2022 at 08:02:23AM +1100, Dave Chinner wrote: > > On Fri, Nov 18, 2022 at 01:14:08PM -0800, Catherine Hoang wrote: > > i.e. > > if (fsuuid.fsu_flags != 0) > > return -EINVAL; > > > > error = xfs_fs_get_uuid(&mp->m_sb, uuid, &fsuuid.fsu_len, NULL); > > if (error) > > return -EINVAL; > > > > Also, uuid_copy()? > > Why does xfs_fs_get_uuid use memcpy then? Did the compiler reject the > u8* -> uuid_t * type conversion? No idea, I've completely forgotten about the reasons for the code being written that way. These days people seem to care about making the compiler do all the type checking and type conversions for us. The use of UUIDs and various types within XFS has been quite ad hoc, so I'm just suggesting that we improve it somewhat. Using types and APIs that mean we don't have to code the length of UUIDs everywhere seems like a Good Idea for newly written code... > Alternately there's export_uuid(). But we don't need that if we use uuid_t for all the local representations of the UUID.... Cheers, Dave.
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index 1f783e979629..cf77715afe9e 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -1865,6 +1865,39 @@ xfs_fs_eofblocks_from_user( return 0; } +static int +xfs_ioctl_getuuid( + struct xfs_mount *mp, + struct fsuuid __user *ufsuuid) +{ + struct fsuuid fsuuid; + __u8 uuid[UUID_SIZE]; + + if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid))) + return -EFAULT; + + if (fsuuid.fsu_len == 0) { + fsuuid.fsu_len = UUID_SIZE; + if (copy_to_user(&ufsuuid->fsu_len, &fsuuid.fsu_len, + sizeof(fsuuid.fsu_len))) + return -EFAULT; + return 0; + } + + if (fsuuid.fsu_len < UUID_SIZE || fsuuid.fsu_flags != 0) + return -EINVAL; + + spin_lock(&mp->m_sb_lock); + memcpy(uuid, &mp->m_sb.sb_uuid, UUID_SIZE); + spin_unlock(&mp->m_sb_lock); + + fsuuid.fsu_len = UUID_SIZE; + if (copy_to_user(ufsuuid, &fsuuid, sizeof(fsuuid)) || + copy_to_user(&ufsuuid->fsu_uuid[0], uuid, UUID_SIZE)) + return -EFAULT; + return 0; +} + /* * These long-unused ioctls were removed from the official ioctl API in 5.17, * but retain these definitions so that we can log warnings about them. @@ -2153,6 +2186,9 @@ xfs_file_ioctl( return error; } + case FS_IOC_GETFSUUID: + return xfs_ioctl_getuuid(mp, arg); + default: return -ENOTTY; }