| Message ID | 20211124134542.22270-1-lczerner@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | tune2fs: implement support for set/get label iocts | expand |
On Nov 24, 2021, at 6:45 AM, Lukas Czerner <lczerner@redhat.com> wrote: > > Implement support for FS_IOC_SETFSLABEL and FS_IOC_GETFSLABEL ioctls. > Try to use the ioctls if possible even before we open the file system > since we don't need it. Only fall back to the old method in the case the > file system is not mounted, is mounted read only in the set label case, > or the ioctls are not suppported by the kernel. > > The new ioctls can also be supported by file system drivers other than > ext4. As a result tune2fs and e2label will work for those file systems > as well as long as the file system is mounted. Note that we still truncate > the label exceeds the supported lenghth on extN file system family, while > we keep the label intact for others. > > Update tune2fs and e2label as well. > > Signed-off-by: Lukas Czerner <lczerner@redhat.com> > --- > lib/ext2fs/ext2fs.h | 1 + > lib/ext2fs/ismounted.c | 5 +++ > misc/e2label.8.in | 7 ++- > misc/tune2fs.8.in | 8 +++- > misc/tune2fs.c | 96 ++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 114 insertions(+), 3 deletions(-) > > diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h > index 0ee0e7d0..68f9c1fe 100644 > --- a/lib/ext2fs/ext2fs.h > +++ b/lib/ext2fs/ext2fs.h > @@ -531,6 +531,7 @@ typedef struct ext2_struct_inode_scan *ext2_inode_scan; > #define EXT2_MF_READONLY 4 > #define EXT2_MF_SWAP 8 > #define EXT2_MF_BUSY 16 > +#define EXT2_MF_EXTFS 32 > > /* > * Ext2/linux mode flags. We define them here so that we don't need > diff --git a/lib/ext2fs/ismounted.c b/lib/ext2fs/ismounted.c > index aee7d726..c73273b8 100644 > --- a/lib/ext2fs/ismounted.c > +++ b/lib/ext2fs/ismounted.c > @@ -207,6 +207,11 @@ is_root: > close(fd); > (void) unlink(TEST_FILE); > } > + > + if (!strcmp(mnt->mnt_type, "ext4") || > + !strcmp(mnt->mnt_type, "ext3") || > + !strcmp(mnt->mnt_type, "ext2")) IMHO, using "!strcmp(...)" reads like "not matching the string ...", so I prefer to use "strcmp(...) == 0". > + *mount_flags |= EXT2_MF_EXTFS; > retval = 0; > errout: > endmntent (f); > diff --git a/misc/e2label.8.in b/misc/e2label.8.in > index 1dc96199..fa5294c4 100644 > --- a/misc/e2label.8.in > +++ b/misc/e2label.8.in > @@ -33,7 +33,12 @@ Ext2 volume labels can be at most 16 characters long; if > .I volume-label > is longer than 16 characters, > .B e2label > -will truncate it and print a warning message. > +will truncate it and print a warning message. For other file systems that > +support online label manipulation and are mounted > +.B e2label > +will work as well, but it will not attempt to truncate the > +.I volume-label > +at all. > .PP > It is also possible to set the volume label using the > .B \-L > diff --git a/misc/tune2fs.8.in b/misc/tune2fs.8.in > index 1e026e5f..628dcdc0 100644 > --- a/misc/tune2fs.8.in > +++ b/misc/tune2fs.8.in > @@ -457,8 +457,12 @@ Ext2 file system labels can be at most 16 characters long; if > .I volume-label > is longer than 16 characters, > .B tune2fs > -will truncate it and print a warning. The volume label can be used > -by > +will truncate it and print a warning. For other file systems that > +support online label manipulation and are mounted > +.B tune2fs > +will work as well, but it will not attempt to truncate the > +.I volume-label > +at all. The volume label can be used by > .BR mount (8), > .BR fsck (8), > and > diff --git a/misc/tune2fs.c b/misc/tune2fs.c > index 71a8e99b..6c162ba5 100644 > --- a/misc/tune2fs.c > +++ b/misc/tune2fs.c > @@ -52,6 +52,9 @@ extern int optind; > #include <sys/types.h> > #include <libgen.h> > #include <limits.h> > +#ifdef HAVE_SYS_IOCTL_H > +#include <sys/ioctl.h> > +#endif > > #include "ext2fs/ext2_fs.h" > #include "ext2fs/ext2fs.h" > @@ -70,6 +73,15 @@ extern int optind; > #define QOPT_ENABLE (1) > #define QOPT_DISABLE (-1) > > +#ifndef FS_IOC_SETFSLABEL > +#define FSLABEL_MAX 256 > +#define FS_IOC_SETFSLABEL _IOW(0x94, 50, char[FSLABEL_MAX]) > +#endif > + > +#ifndef FS_IOC_GETFSLABEL > +#define FS_IOC_GETFSLABEL _IOR(0x94, 49, char[FSLABEL_MAX]) > +#endif > + > extern int ask_yn(const char *string, int def); > > const char *program_name = "tune2fs"; > @@ -2997,6 +3009,75 @@ fs_update_journal_user(struct ext2_super_block *sb, __u8 old_uuid[UUID_SIZE]) > return 0; > } > > +/* > + * Use FS_IOC_SETFSLABEL or FS_IOC_GETFSLABEL to set/get file system label > + * Return: 0 on success > + * 1 on error > + * -1 when the old method should be used > + */ > +int handle_fslabel(int setlabel) { > + errcode_t ret; > + int mnt_flags, fd; > + char label[FSLABEL_MAX]; > + int maxlen = FSLABEL_MAX - 1; > + char mntpt[PATH_MAX + 1]; > + > + ret = ext2fs_check_mount_point(device_name, &mnt_flags, > + mntpt, sizeof(mntpt)); > + if (ret) { > + com_err(device_name, ret, _("while checking mount status")); > + return 1; > + } > + if (!(mnt_flags & EXT2_MF_MOUNTED) || > + (setlabel && (mnt_flags & EXT2_MF_READONLY))) > + return -1; > + > + if (!mntpt[0]) { > + fprintf(stderr,_("Unknown mount point for %s\n"), device_name); > + return 1; > + } > + > + fd = open(mntpt, O_RDONLY); Opening read-only to change the label is a bit strange? It would be better to open in write mode, and verify in the kernel that this is the case: fd = open(mntpt, setlabel ? O_WRONLY : O_RDONLY); > + if (fd < 0) { > + com_err(mntpt, errno, _("while opening mount point")); > + return 1; > + } > + > + /* Get fs label */ > + if (!setlabel) { > + if (ioctl(fd, FS_IOC_GETFSLABEL, &label)) { > + close(fd); > + if (errno == ENOTTY) > + return -1; > + com_err(mntpt, errno, _("while trying to get fs label")); > + return 1; > + } > + close(fd); > + printf("%.*s\n", EXT2_LEN_STR(label)); > + return 0; > + } > + > + /* If it's extN file system, truncate the label to appropriate size */ > + if (mnt_flags & EXT2_MF_EXTFS) > + maxlen = EXT2_LABEL_LEN; > + if (strlen(new_label) > maxlen) { > + fputs(_("Warning: label too long, truncating.\n"), > + stderr); > + new_label[maxlen] = '\0'; > + } > + > + /* Set fs label */ > + if (ioctl(fd, FS_IOC_SETFSLABEL, new_label)) { > + close(fd); > + if (errno == ENOTTY) > + return -1; > + com_err(mntpt, errno, _("while trying to set fs label")); > + return 1; > + } > + close(fd); > + return 0; > +} > + > #ifndef BUILD_AS_LIB > int main(int argc, char **argv) > #else > @@ -3038,6 +3119,21 @@ int tune2fs_main(int argc, char **argv) > #endif > io_ptr = unix_io_manager; > > + /* > + * Try the get/set fs label using ioctls before we even attempt > + * to open the file system. > + */ > + if (L_flag || print_label) { > + rc = handle_fslabel(L_flag); > + if (rc != -1) { > +#ifndef BUILD_AS_LIB > + exit(rc); > +#endif > + return rc; > + } > + rc = 0; > + } > + > retry_open: > if ((open_flag & EXT2_FLAG_RW) == 0 || f_flag) > open_flag |= EXT2_FLAG_SKIP_MMP; > -- > 2.31.1 > Cheers, Andreas
On Sat, Nov 27, 2021 at 02:23:32PM -0700, Andreas Dilger wrote: > On Nov 24, 2021, at 6:45 AM, Lukas Czerner <lczerner@redhat.com> wrote: > > > > Implement support for FS_IOC_SETFSLABEL and FS_IOC_GETFSLABEL ioctls. > > Try to use the ioctls if possible even before we open the file system > > since we don't need it. Only fall back to the old method in the case the > > file system is not mounted, is mounted read only in the set label case, > > or the ioctls are not suppported by the kernel. > > > > The new ioctls can also be supported by file system drivers other than > > ext4. As a result tune2fs and e2label will work for those file systems > > as well as long as the file system is mounted. Note that we still truncate > > the label exceeds the supported lenghth on extN file system family, while > > we keep the label intact for others. > > > > Update tune2fs and e2label as well. > > > > Signed-off-by: Lukas Czerner <lczerner@redhat.com> > > --- > > lib/ext2fs/ext2fs.h | 1 + > > lib/ext2fs/ismounted.c | 5 +++ > > misc/e2label.8.in | 7 ++- > > misc/tune2fs.8.in | 8 +++- > > misc/tune2fs.c | 96 ++++++++++++++++++++++++++++++++++++++++++ > > 5 files changed, 114 insertions(+), 3 deletions(-) > > > > diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h > > index 0ee0e7d0..68f9c1fe 100644 > > --- a/lib/ext2fs/ext2fs.h > > +++ b/lib/ext2fs/ext2fs.h > > @@ -531,6 +531,7 @@ typedef struct ext2_struct_inode_scan *ext2_inode_scan; > > #define EXT2_MF_READONLY 4 > > #define EXT2_MF_SWAP 8 > > #define EXT2_MF_BUSY 16 > > +#define EXT2_MF_EXTFS 32 > > > > /* > > * Ext2/linux mode flags. We define them here so that we don't need > > diff --git a/lib/ext2fs/ismounted.c b/lib/ext2fs/ismounted.c > > index aee7d726..c73273b8 100644 > > --- a/lib/ext2fs/ismounted.c > > +++ b/lib/ext2fs/ismounted.c > > @@ -207,6 +207,11 @@ is_root: > > close(fd); > > (void) unlink(TEST_FILE); > > } > > + > > + if (!strcmp(mnt->mnt_type, "ext4") || > > + !strcmp(mnt->mnt_type, "ext3") || > > + !strcmp(mnt->mnt_type, "ext2")) > > IMHO, using "!strcmp(...)" reads like "not matching the string ...", so I prefer > to use "strcmp(...) == 0". Hi Andreas, thanks for thre review! Ok, I can change that. > > > + *mount_flags |= EXT2_MF_EXTFS; > > retval = 0; > > errout: > > endmntent (f); > > diff --git a/misc/e2label.8.in b/misc/e2label.8.in > > index 1dc96199..fa5294c4 100644 > > --- a/misc/e2label.8.in > > +++ b/misc/e2label.8.in > > @@ -33,7 +33,12 @@ Ext2 volume labels can be at most 16 characters long; if > > .I volume-label > > is longer than 16 characters, > > .B e2label > > -will truncate it and print a warning message. > > +will truncate it and print a warning message. For other file systems that > > +support online label manipulation and are mounted > > +.B e2label > > +will work as well, but it will not attempt to truncate the > > +.I volume-label > > +at all. > > .PP > > It is also possible to set the volume label using the > > .B \-L > > diff --git a/misc/tune2fs.8.in b/misc/tune2fs.8.in > > index 1e026e5f..628dcdc0 100644 > > --- a/misc/tune2fs.8.in > > +++ b/misc/tune2fs.8.in > > @@ -457,8 +457,12 @@ Ext2 file system labels can be at most 16 characters long; if > > .I volume-label > > is longer than 16 characters, > > .B tune2fs > > -will truncate it and print a warning. The volume label can be used > > -by > > +will truncate it and print a warning. For other file systems that > > +support online label manipulation and are mounted > > +.B tune2fs > > +will work as well, but it will not attempt to truncate the > > +.I volume-label > > +at all. The volume label can be used by > > .BR mount (8), > > .BR fsck (8), > > and > > diff --git a/misc/tune2fs.c b/misc/tune2fs.c > > index 71a8e99b..6c162ba5 100644 > > --- a/misc/tune2fs.c > > +++ b/misc/tune2fs.c > > @@ -52,6 +52,9 @@ extern int optind; > > #include <sys/types.h> > > #include <libgen.h> > > #include <limits.h> > > +#ifdef HAVE_SYS_IOCTL_H > > +#include <sys/ioctl.h> > > +#endif > > > > #include "ext2fs/ext2_fs.h" > > #include "ext2fs/ext2fs.h" > > @@ -70,6 +73,15 @@ extern int optind; > > #define QOPT_ENABLE (1) > > #define QOPT_DISABLE (-1) > > > > +#ifndef FS_IOC_SETFSLABEL > > +#define FSLABEL_MAX 256 > > +#define FS_IOC_SETFSLABEL _IOW(0x94, 50, char[FSLABEL_MAX]) > > +#endif > > + > > +#ifndef FS_IOC_GETFSLABEL > > +#define FS_IOC_GETFSLABEL _IOR(0x94, 49, char[FSLABEL_MAX]) > > +#endif > > + > > extern int ask_yn(const char *string, int def); > > > > const char *program_name = "tune2fs"; > > @@ -2997,6 +3009,75 @@ fs_update_journal_user(struct ext2_super_block *sb, __u8 old_uuid[UUID_SIZE]) > > return 0; > > } > > > > +/* > > + * Use FS_IOC_SETFSLABEL or FS_IOC_GETFSLABEL to set/get file system label > > + * Return: 0 on success > > + * 1 on error > > + * -1 when the old method should be used > > + */ > > +int handle_fslabel(int setlabel) { > > + errcode_t ret; > > + int mnt_flags, fd; > > + char label[FSLABEL_MAX]; > > + int maxlen = FSLABEL_MAX - 1; > > + char mntpt[PATH_MAX + 1]; > > + > > + ret = ext2fs_check_mount_point(device_name, &mnt_flags, > > + mntpt, sizeof(mntpt)); > > + if (ret) { > > + com_err(device_name, ret, _("while checking mount status")); > > + return 1; > > + } > > + if (!(mnt_flags & EXT2_MF_MOUNTED) || > > + (setlabel && (mnt_flags & EXT2_MF_READONLY))) > > + return -1; > > + > > + if (!mntpt[0]) { > > + fprintf(stderr,_("Unknown mount point for %s\n"), device_name); > > + return 1; > > + } > > + > > + fd = open(mntpt, O_RDONLY); > > Opening read-only to change the label is a bit strange? It would be better > to open in write mode, and verify in the kernel that this is the case: > > fd = open(mntpt, setlabel ? O_WRONLY : O_RDONLY); I am not convinced about this. Sure it may feel strange, but: - we're not operating on the file itself but the file system in general and that needs to be rw mounted; kernel will check that - no other fslabel implementation requires the file to be opened for writing. - we don't even require file to be opened to writing for the most of our own special ioctls if they don't deal with the file itself such as EXT4_IOC_MOVE_EXT and EXT4_IOC_SWAP_BOOT - btrfs-progs uses O_RDONLY of setting label, fstrim uses O_RDONLY for FITRIM and I am sure there are plenty more examples. So AFAICT the standard seems to be not to require it and just open O_RDONLY if we really want a handle of a file system, not the file itself. I don't really care either way, but I am not willing to change what to me seems to be a standard way of doing this. So if you insist I'll change the code here, but I won't change it on the kernel side to require FMODE_WRITE. Thanks again for the review. -Lukas > > > + if (fd < 0) { > > + com_err(mntpt, errno, _("while opening mount point")); > > + return 1; > > + } > > + > > + /* Get fs label */ > > + if (!setlabel) { > > + if (ioctl(fd, FS_IOC_GETFSLABEL, &label)) { > > + close(fd); > > + if (errno == ENOTTY) > > + return -1; > > + com_err(mntpt, errno, _("while trying to get fs label")); > > + return 1; > > + } > > + close(fd); > > + printf("%.*s\n", EXT2_LEN_STR(label)); > > + return 0; > > + } > > + > > + /* If it's extN file system, truncate the label to appropriate size */ > > + if (mnt_flags & EXT2_MF_EXTFS) > > + maxlen = EXT2_LABEL_LEN; > > + if (strlen(new_label) > maxlen) { > > + fputs(_("Warning: label too long, truncating.\n"), > > + stderr); > > + new_label[maxlen] = '\0'; > > + } > > + > > + /* Set fs label */ > > + if (ioctl(fd, FS_IOC_SETFSLABEL, new_label)) { > > + close(fd); > > + if (errno == ENOTTY) > > + return -1; > > + com_err(mntpt, errno, _("while trying to set fs label")); > > + return 1; > > + } > > + close(fd); > > + return 0; > > +} > > + > > #ifndef BUILD_AS_LIB > > int main(int argc, char **argv) > > #else > > @@ -3038,6 +3119,21 @@ int tune2fs_main(int argc, char **argv) > > #endif > > io_ptr = unix_io_manager; > > > > + /* > > + * Try the get/set fs label using ioctls before we even attempt > > + * to open the file system. > > + */ > > + if (L_flag || print_label) { > > + rc = handle_fslabel(L_flag); > > + if (rc != -1) { > > +#ifndef BUILD_AS_LIB > > + exit(rc); > > +#endif > > + return rc; > > + } > > + rc = 0; > > + } > > + > > retry_open: > > if ((open_flag & EXT2_FLAG_RW) == 0 || f_flag) > > open_flag |= EXT2_FLAG_SKIP_MMP; > > -- > > 2.31.1 > > > > > Cheers, Andreas > > > > >
> On Nov 29, 2021, at 2:36 AM, Lukas Czerner <lczerner@redhat.com> wrote: > > On Sat, Nov 27, 2021 at 02:23:32PM -0700, Andreas Dilger wrote: >> On Nov 24, 2021, at 6:45 AM, Lukas Czerner <lczerner@redhat.com> wrote: >>> >>> Implement support for FS_IOC_SETFSLABEL and FS_IOC_GETFSLABEL ioctls. >>> Try to use the ioctls if possible even before we open the file system >>> since we don't need it. Only fall back to the old method in the case the >>> file system is not mounted, is mounted read only in the set label case, >>> or the ioctls are not suppported by the kernel. >>> >>> The new ioctls can also be supported by file system drivers other than >>> ext4. As a result tune2fs and e2label will work for those file systems >>> as well as long as the file system is mounted. Note that we still truncate >>> the label exceeds the supported lenghth on extN file system family, while >>> we keep the label intact for others. >>> >>> Update tune2fs and e2label as well. >>> >>> Signed-off-by: Lukas Czerner <lczerner@redhat.com> >>> --- >>> lib/ext2fs/ext2fs.h | 1 + >>> lib/ext2fs/ismounted.c | 5 +++ >>> misc/e2label.8.in | 7 ++- >>> misc/tune2fs.8.in | 8 +++- >>> misc/tune2fs.c | 96 ++++++++++++++++++++++++++++++++++++++++++ >>> 5 files changed, 114 insertions(+), 3 deletions(-) >>> >>> diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h >>> index 0ee0e7d0..68f9c1fe 100644 >>> --- a/lib/ext2fs/ext2fs.h >>> +++ b/lib/ext2fs/ext2fs.h >>> @@ -531,6 +531,7 @@ typedef struct ext2_struct_inode_scan *ext2_inode_scan; >>> #define EXT2_MF_READONLY 4 >>> #define EXT2_MF_SWAP 8 >>> #define EXT2_MF_BUSY 16 >>> +#define EXT2_MF_EXTFS 32 >>> >>> /* >>> * Ext2/linux mode flags. We define them here so that we don't need >>> diff --git a/lib/ext2fs/ismounted.c b/lib/ext2fs/ismounted.c >>> index aee7d726..c73273b8 100644 >>> --- a/lib/ext2fs/ismounted.c >>> +++ b/lib/ext2fs/ismounted.c >>> @@ -207,6 +207,11 @@ is_root: >>> close(fd); >>> (void) unlink(TEST_FILE); >>> } >>> + >>> + if (!strcmp(mnt->mnt_type, "ext4") || >>> + !strcmp(mnt->mnt_type, "ext3") || >>> + !strcmp(mnt->mnt_type, "ext2")) >> >> IMHO, using "!strcmp(...)" reads like "not matching the string ...", so I prefer >> to use "strcmp(...) == 0". > > Hi Andreas, thanks for thre review! > > Ok, I can change that. > >> >>> + *mount_flags |= EXT2_MF_EXTFS; >>> retval = 0; >>> errout: >>> endmntent (f); >>> diff --git a/misc/e2label.8.in b/misc/e2label.8.in >>> index 1dc96199..fa5294c4 100644 >>> --- a/misc/e2label.8.in >>> +++ b/misc/e2label.8.in >>> @@ -33,7 +33,12 @@ Ext2 volume labels can be at most 16 characters long; if >>> .I volume-label >>> is longer than 16 characters, >>> .B e2label >>> -will truncate it and print a warning message. >>> +will truncate it and print a warning message. For other file systems that >>> +support online label manipulation and are mounted >>> +.B e2label >>> +will work as well, but it will not attempt to truncate the >>> +.I volume-label >>> +at all. >>> .PP >>> It is also possible to set the volume label using the >>> .B \-L >>> diff --git a/misc/tune2fs.8.in b/misc/tune2fs.8.in >>> index 1e026e5f..628dcdc0 100644 >>> --- a/misc/tune2fs.8.in >>> +++ b/misc/tune2fs.8.in >>> @@ -457,8 +457,12 @@ Ext2 file system labels can be at most 16 characters long; if >>> .I volume-label >>> is longer than 16 characters, >>> .B tune2fs >>> -will truncate it and print a warning. The volume label can be used >>> -by >>> +will truncate it and print a warning. For other file systems that >>> +support online label manipulation and are mounted >>> +.B tune2fs >>> +will work as well, but it will not attempt to truncate the >>> +.I volume-label >>> +at all. The volume label can be used by >>> .BR mount (8), >>> .BR fsck (8), >>> and >>> diff --git a/misc/tune2fs.c b/misc/tune2fs.c >>> index 71a8e99b..6c162ba5 100644 >>> --- a/misc/tune2fs.c >>> +++ b/misc/tune2fs.c >>> @@ -52,6 +52,9 @@ extern int optind; >>> #include <sys/types.h> >>> #include <libgen.h> >>> #include <limits.h> >>> +#ifdef HAVE_SYS_IOCTL_H >>> +#include <sys/ioctl.h> >>> +#endif >>> >>> #include "ext2fs/ext2_fs.h" >>> #include "ext2fs/ext2fs.h" >>> @@ -70,6 +73,15 @@ extern int optind; >>> #define QOPT_ENABLE (1) >>> #define QOPT_DISABLE (-1) >>> >>> +#ifndef FS_IOC_SETFSLABEL >>> +#define FSLABEL_MAX 256 >>> +#define FS_IOC_SETFSLABEL _IOW(0x94, 50, char[FSLABEL_MAX]) >>> +#endif >>> + >>> +#ifndef FS_IOC_GETFSLABEL >>> +#define FS_IOC_GETFSLABEL _IOR(0x94, 49, char[FSLABEL_MAX]) >>> +#endif >>> + >>> extern int ask_yn(const char *string, int def); >>> >>> const char *program_name = "tune2fs"; >>> @@ -2997,6 +3009,75 @@ fs_update_journal_user(struct ext2_super_block *sb, __u8 old_uuid[UUID_SIZE]) >>> return 0; >>> } >>> >>> +/* >>> + * Use FS_IOC_SETFSLABEL or FS_IOC_GETFSLABEL to set/get file system label >>> + * Return: 0 on success >>> + * 1 on error >>> + * -1 when the old method should be used >>> + */ >>> +int handle_fslabel(int setlabel) { >>> + errcode_t ret; >>> + int mnt_flags, fd; >>> + char label[FSLABEL_MAX]; >>> + int maxlen = FSLABEL_MAX - 1; >>> + char mntpt[PATH_MAX + 1]; >>> + >>> + ret = ext2fs_check_mount_point(device_name, &mnt_flags, >>> + mntpt, sizeof(mntpt)); >>> + if (ret) { >>> + com_err(device_name, ret, _("while checking mount status")); >>> + return 1; >>> + } >>> + if (!(mnt_flags & EXT2_MF_MOUNTED) || >>> + (setlabel && (mnt_flags & EXT2_MF_READONLY))) >>> + return -1; >>> + >>> + if (!mntpt[0]) { >>> + fprintf(stderr,_("Unknown mount point for %s\n"), device_name); >>> + return 1; >>> + } >>> + >>> + fd = open(mntpt, O_RDONLY); >> >> Opening read-only to change the label is a bit strange? It would be better >> to open in write mode, and verify in the kernel that this is the case: >> >> fd = open(mntpt, setlabel ? O_WRONLY : O_RDONLY); > > I am not convinced about this. Sure it may feel strange, but: > > - we're not operating on the file itself but the file system in general > and that needs to be rw mounted; kernel will check that > - no other fslabel implementation requires the file to be opened for > writing. > - we don't even require file to be opened to writing for the most of our > own special ioctls if they don't deal with the file itself such as > EXT4_IOC_MOVE_EXT and EXT4_IOC_SWAP_BOOT > - btrfs-progs uses O_RDONLY of setting label, fstrim uses O_RDONLY for > FITRIM and I am sure there are plenty more examples. > > So AFAICT the standard seems to be not to require it and just open > O_RDONLY if we really want a handle of a file system, not the file > itself. I don't really care either way, but I am not willing to change > what to me seems to be a standard way of doing this. > > So if you insist I'll change the code here, but I won't change it on the > kernel side to require FMODE_WRITE. I see in the kernel patch that it is checking for CAP_SYS_ADMIN, so that should be enough protection. You can add my: Reviewed-by: Andreas Dilger <adilger@dilger.ca> Cheers, Andreas
On Wed, 24 Nov 2021 14:45:42 +0100, Lukas Czerner wrote: > Implement support for FS_IOC_SETFSLABEL and FS_IOC_GETFSLABEL ioctls. > Try to use the ioctls if possible even before we open the file system > since we don't need it. Only fall back to the old method in the case the > file system is not mounted, is mounted read only in the set label case, > or the ioctls are not suppported by the kernel. > > The new ioctls can also be supported by file system drivers other than > ext4. As a result tune2fs and e2label will work for those file systems > as well as long as the file system is mounted. Note that we still truncate > the label exceeds the supported lenghth on extN file system family, while > we keep the label intact for others. > > [...] Applied, thanks! [1/1] tune2fs: implement support for set/get label iocts (no commit info) Best regards,
diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h index 0ee0e7d0..68f9c1fe 100644 --- a/lib/ext2fs/ext2fs.h +++ b/lib/ext2fs/ext2fs.h @@ -531,6 +531,7 @@ typedef struct ext2_struct_inode_scan *ext2_inode_scan; #define EXT2_MF_READONLY 4 #define EXT2_MF_SWAP 8 #define EXT2_MF_BUSY 16 +#define EXT2_MF_EXTFS 32 /* * Ext2/linux mode flags. We define them here so that we don't need diff --git a/lib/ext2fs/ismounted.c b/lib/ext2fs/ismounted.c index aee7d726..c73273b8 100644 --- a/lib/ext2fs/ismounted.c +++ b/lib/ext2fs/ismounted.c @@ -207,6 +207,11 @@ is_root: close(fd); (void) unlink(TEST_FILE); } + + if (!strcmp(mnt->mnt_type, "ext4") || + !strcmp(mnt->mnt_type, "ext3") || + !strcmp(mnt->mnt_type, "ext2")) + *mount_flags |= EXT2_MF_EXTFS; retval = 0; errout: endmntent (f); diff --git a/misc/e2label.8.in b/misc/e2label.8.in index 1dc96199..fa5294c4 100644 --- a/misc/e2label.8.in +++ b/misc/e2label.8.in @@ -33,7 +33,12 @@ Ext2 volume labels can be at most 16 characters long; if .I volume-label is longer than 16 characters, .B e2label -will truncate it and print a warning message. +will truncate it and print a warning message. For other file systems that +support online label manipulation and are mounted +.B e2label +will work as well, but it will not attempt to truncate the +.I volume-label +at all. .PP It is also possible to set the volume label using the .B \-L diff --git a/misc/tune2fs.8.in b/misc/tune2fs.8.in index 1e026e5f..628dcdc0 100644 --- a/misc/tune2fs.8.in +++ b/misc/tune2fs.8.in @@ -457,8 +457,12 @@ Ext2 file system labels can be at most 16 characters long; if .I volume-label is longer than 16 characters, .B tune2fs -will truncate it and print a warning. The volume label can be used -by +will truncate it and print a warning. For other file systems that +support online label manipulation and are mounted +.B tune2fs +will work as well, but it will not attempt to truncate the +.I volume-label +at all. The volume label can be used by .BR mount (8), .BR fsck (8), and diff --git a/misc/tune2fs.c b/misc/tune2fs.c index 71a8e99b..6c162ba5 100644 --- a/misc/tune2fs.c +++ b/misc/tune2fs.c @@ -52,6 +52,9 @@ extern int optind; #include <sys/types.h> #include <libgen.h> #include <limits.h> +#ifdef HAVE_SYS_IOCTL_H +#include <sys/ioctl.h> +#endif #include "ext2fs/ext2_fs.h" #include "ext2fs/ext2fs.h" @@ -70,6 +73,15 @@ extern int optind; #define QOPT_ENABLE (1) #define QOPT_DISABLE (-1) +#ifndef FS_IOC_SETFSLABEL +#define FSLABEL_MAX 256 +#define FS_IOC_SETFSLABEL _IOW(0x94, 50, char[FSLABEL_MAX]) +#endif + +#ifndef FS_IOC_GETFSLABEL +#define FS_IOC_GETFSLABEL _IOR(0x94, 49, char[FSLABEL_MAX]) +#endif + extern int ask_yn(const char *string, int def); const char *program_name = "tune2fs"; @@ -2997,6 +3009,75 @@ fs_update_journal_user(struct ext2_super_block *sb, __u8 old_uuid[UUID_SIZE]) return 0; } +/* + * Use FS_IOC_SETFSLABEL or FS_IOC_GETFSLABEL to set/get file system label + * Return: 0 on success + * 1 on error + * -1 when the old method should be used + */ +int handle_fslabel(int setlabel) { + errcode_t ret; + int mnt_flags, fd; + char label[FSLABEL_MAX]; + int maxlen = FSLABEL_MAX - 1; + char mntpt[PATH_MAX + 1]; + + ret = ext2fs_check_mount_point(device_name, &mnt_flags, + mntpt, sizeof(mntpt)); + if (ret) { + com_err(device_name, ret, _("while checking mount status")); + return 1; + } + if (!(mnt_flags & EXT2_MF_MOUNTED) || + (setlabel && (mnt_flags & EXT2_MF_READONLY))) + return -1; + + if (!mntpt[0]) { + fprintf(stderr,_("Unknown mount point for %s\n"), device_name); + return 1; + } + + fd = open(mntpt, O_RDONLY); + if (fd < 0) { + com_err(mntpt, errno, _("while opening mount point")); + return 1; + } + + /* Get fs label */ + if (!setlabel) { + if (ioctl(fd, FS_IOC_GETFSLABEL, &label)) { + close(fd); + if (errno == ENOTTY) + return -1; + com_err(mntpt, errno, _("while trying to get fs label")); + return 1; + } + close(fd); + printf("%.*s\n", EXT2_LEN_STR(label)); + return 0; + } + + /* If it's extN file system, truncate the label to appropriate size */ + if (mnt_flags & EXT2_MF_EXTFS) + maxlen = EXT2_LABEL_LEN; + if (strlen(new_label) > maxlen) { + fputs(_("Warning: label too long, truncating.\n"), + stderr); + new_label[maxlen] = '\0'; + } + + /* Set fs label */ + if (ioctl(fd, FS_IOC_SETFSLABEL, new_label)) { + close(fd); + if (errno == ENOTTY) + return -1; + com_err(mntpt, errno, _("while trying to set fs label")); + return 1; + } + close(fd); + return 0; +} + #ifndef BUILD_AS_LIB int main(int argc, char **argv) #else @@ -3038,6 +3119,21 @@ int tune2fs_main(int argc, char **argv) #endif io_ptr = unix_io_manager; + /* + * Try the get/set fs label using ioctls before we even attempt + * to open the file system. + */ + if (L_flag || print_label) { + rc = handle_fslabel(L_flag); + if (rc != -1) { +#ifndef BUILD_AS_LIB + exit(rc); +#endif + return rc; + } + rc = 0; + } + retry_open: if ((open_flag & EXT2_FLAG_RW) == 0 || f_flag) open_flag |= EXT2_FLAG_SKIP_MMP;
Implement support for FS_IOC_SETFSLABEL and FS_IOC_GETFSLABEL ioctls. Try to use the ioctls if possible even before we open the file system since we don't need it. Only fall back to the old method in the case the file system is not mounted, is mounted read only in the set label case, or the ioctls are not suppported by the kernel. The new ioctls can also be supported by file system drivers other than ext4. As a result tune2fs and e2label will work for those file systems as well as long as the file system is mounted. Note that we still truncate the label exceeds the supported lenghth on extN file system family, while we keep the label intact for others. Update tune2fs and e2label as well. Signed-off-by: Lukas Czerner <lczerner@redhat.com> --- lib/ext2fs/ext2fs.h | 1 + lib/ext2fs/ismounted.c | 5 +++ misc/e2label.8.in | 7 ++- misc/tune2fs.8.in | 8 +++- misc/tune2fs.c | 96 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 114 insertions(+), 3 deletions(-)