Date:   Sat, 27 Nov 2021 14:23:32 -0700
From:   Andreas Dilger <adilger@...ger.ca>
To:     Lukas Czerner <lczerner@...hat.com>
Cc:     linux-ext4 <linux-ext4@...r.kernel.org>, tytso@....edu
Subject: Re: [PATCH] tune2fs: implement support for set/get label iocts

On Nov 24, 2021, at 6:45 AM, Lukas Czerner <lczerner@...hat.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@...hat.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






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)

Powered by blists - more mailing lists