[PATCH] ufs: convert ufs to the new mount API

[PATCH] ufs: convert ufs to the new mount API

[Eric Sandeen]

Convert ufs to the new mount API

ufs_set/clear/test_opt macros are changed to take the full option,
to facilitate setting in parse_param.

ufstype option changes during remount are rejected during parsing,
rather than after the fact as they were before.

Removed a BUG_ON from show_options(); worst case, it prints
a null string where the BUG_ON would have fired.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/ufs/super.c | 280 +++++++++++++++++++++----------------------------
 fs/ufs/ufs.h   |   6 +-
 2 files changed, 125 insertions(+), 161 deletions(-)

diff --git a/fs/ufs/super.c b/fs/ufs/super.c
index 762699c1bcf6..6654dd50c031 100644
--- a/fs/ufs/super.c
+++ b/fs/ufs/super.c
@@ -83,11 +83,11 @@
 #include <linux/blkdev.h>
 #include <linux/backing-dev.h>
 #include <linux/init.h>
-#include <linux/parser.h>
+#include <linux/fs_context.h>
+#include <linux/fs_parser.h>
 #include <linux/buffer_head.h>
 #include <linux/vfs.h>
 #include <linux/log2.h>
-#include <linux/mount.h>
 #include <linux/seq_file.h>
 #include <linux/iversion.h>
 
@@ -342,124 +342,72 @@ void ufs_warning (struct super_block * sb, const char * function,
 	va_end(args);
 }
 
-enum {
-       Opt_type_old = UFS_MOUNT_UFSTYPE_OLD,
-       Opt_type_sunx86 = UFS_MOUNT_UFSTYPE_SUNx86,
-       Opt_type_sun = UFS_MOUNT_UFSTYPE_SUN,
-       Opt_type_sunos = UFS_MOUNT_UFSTYPE_SUNOS,
-       Opt_type_44bsd = UFS_MOUNT_UFSTYPE_44BSD,
-       Opt_type_ufs2 = UFS_MOUNT_UFSTYPE_UFS2,
-       Opt_type_hp = UFS_MOUNT_UFSTYPE_HP,
-       Opt_type_nextstepcd = UFS_MOUNT_UFSTYPE_NEXTSTEP_CD,
-       Opt_type_nextstep = UFS_MOUNT_UFSTYPE_NEXTSTEP,
-       Opt_type_openstep = UFS_MOUNT_UFSTYPE_OPENSTEP,
-       Opt_onerror_panic = UFS_MOUNT_ONERROR_PANIC,
-       Opt_onerror_lock = UFS_MOUNT_ONERROR_LOCK,
-       Opt_onerror_umount = UFS_MOUNT_ONERROR_UMOUNT,
-       Opt_onerror_repair = UFS_MOUNT_ONERROR_REPAIR,
-       Opt_err
+enum { Opt_type, Opt_onerror };
+
+static const struct constant_table ufs_param_ufstype[] = {
+	{"old",		UFS_MOUNT_UFSTYPE_OLD},
+	{"sunx86",	UFS_MOUNT_UFSTYPE_SUNx86},
+	{"sun",		UFS_MOUNT_UFSTYPE_SUN},
+	{"sunos",	UFS_MOUNT_UFSTYPE_SUNOS},
+	{"44bsd",	UFS_MOUNT_UFSTYPE_44BSD},
+	{"ufs2",	UFS_MOUNT_UFSTYPE_UFS2},
+	{"5xbsd",	UFS_MOUNT_UFSTYPE_UFS2},
+	{"hp",		UFS_MOUNT_UFSTYPE_HP},
+	{"nextstep-cd",	UFS_MOUNT_UFSTYPE_NEXTSTEP_CD},
+	{"nextstep",	UFS_MOUNT_UFSTYPE_NEXTSTEP},
+	{"openstep",	UFS_MOUNT_UFSTYPE_OPENSTEP},
+	{}
 };
 
-static const match_table_t tokens = {
-	{Opt_type_old, "ufstype=old"},
-	{Opt_type_sunx86, "ufstype=sunx86"},
-	{Opt_type_sun, "ufstype=sun"},
-	{Opt_type_sunos, "ufstype=sunos"},
-	{Opt_type_44bsd, "ufstype=44bsd"},
-	{Opt_type_ufs2, "ufstype=ufs2"},
-	{Opt_type_ufs2, "ufstype=5xbsd"},
-	{Opt_type_hp, "ufstype=hp"},
-	{Opt_type_nextstepcd, "ufstype=nextstep-cd"},
-	{Opt_type_nextstep, "ufstype=nextstep"},
-	{Opt_type_openstep, "ufstype=openstep"},
-/*end of possible ufs types */
-	{Opt_onerror_panic, "onerror=panic"},
-	{Opt_onerror_lock, "onerror=lock"},
-	{Opt_onerror_umount, "onerror=umount"},
-	{Opt_onerror_repair, "onerror=repair"},
-	{Opt_err, NULL}
+static const struct constant_table ufs_param_onerror[] = {
+	{"panic",	UFS_MOUNT_ONERROR_PANIC},
+	{"lock",	UFS_MOUNT_ONERROR_LOCK},
+	{"umount",	UFS_MOUNT_ONERROR_UMOUNT},
+	{"repair",	UFS_MOUNT_ONERROR_REPAIR},
+	{}
 };
 
-static int ufs_parse_options (char * options, unsigned * mount_options)
+static const struct fs_parameter_spec ufs_param_spec[] = {
+	fsparam_enum	("ufstype",	Opt_type, ufs_param_ufstype),
+	fsparam_enum	("onerror",	Opt_onerror, ufs_param_onerror),
+	{}
+};
+
+struct ufs_fs_context {
+	unsigned int mount_options;
+};
+
+static int ufs_parse_param(struct fs_context *fc, struct fs_parameter *param)
 {
-	char * p;
-	
+	struct ufs_fs_context *ctx = fc->fs_private;
+	int reconfigure = (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE);
+	struct fs_parse_result result;
+	int opt;
+
 	UFSD("ENTER\n");
-	
-	if (!options)
-		return 1;
 
-	while ((p = strsep(&options, ",")) != NULL) {
-		substring_t args[MAX_OPT_ARGS];
-		int token;
-		if (!*p)
-			continue;
+	opt = fs_parse(fc, ufs_param_spec, param, &result);
+	if (opt < 0)
+		return opt;
 
-		token = match_token(p, tokens, args);
-		switch (token) {
-		case Opt_type_old:
-			ufs_clear_opt (*mount_options, UFSTYPE);
-			ufs_set_opt (*mount_options, UFSTYPE_OLD);
-			break;
-		case Opt_type_sunx86:
-			ufs_clear_opt (*mount_options, UFSTYPE);
-			ufs_set_opt (*mount_options, UFSTYPE_SUNx86);
-			break;
-		case Opt_type_sun:
-			ufs_clear_opt (*mount_options, UFSTYPE);
-			ufs_set_opt (*mount_options, UFSTYPE_SUN);
-			break;
-		case Opt_type_sunos:
-			ufs_clear_opt(*mount_options, UFSTYPE);
-			ufs_set_opt(*mount_options, UFSTYPE_SUNOS);
-			break;
-		case Opt_type_44bsd:
-			ufs_clear_opt (*mount_options, UFSTYPE);
-			ufs_set_opt (*mount_options, UFSTYPE_44BSD);
-			break;
-		case Opt_type_ufs2:
-			ufs_clear_opt(*mount_options, UFSTYPE);
-			ufs_set_opt(*mount_options, UFSTYPE_UFS2);
-			break;
-		case Opt_type_hp:
-			ufs_clear_opt (*mount_options, UFSTYPE);
-			ufs_set_opt (*mount_options, UFSTYPE_HP);
-			break;
-		case Opt_type_nextstepcd:
-			ufs_clear_opt (*mount_options, UFSTYPE);
-			ufs_set_opt (*mount_options, UFSTYPE_NEXTSTEP_CD);
-			break;
-		case Opt_type_nextstep:
-			ufs_clear_opt (*mount_options, UFSTYPE);
-			ufs_set_opt (*mount_options, UFSTYPE_NEXTSTEP);
-			break;
-		case Opt_type_openstep:
-			ufs_clear_opt (*mount_options, UFSTYPE);
-			ufs_set_opt (*mount_options, UFSTYPE_OPENSTEP);
-			break;
-		case Opt_onerror_panic:
-			ufs_clear_opt (*mount_options, ONERROR);
-			ufs_set_opt (*mount_options, ONERROR_PANIC);
-			break;
-		case Opt_onerror_lock:
-			ufs_clear_opt (*mount_options, ONERROR);
-			ufs_set_opt (*mount_options, ONERROR_LOCK);
-			break;
-		case Opt_onerror_umount:
-			ufs_clear_opt (*mount_options, ONERROR);
-			ufs_set_opt (*mount_options, ONERROR_UMOUNT);
-			break;
-		case Opt_onerror_repair:
-			pr_err("Unable to do repair on error, will lock lock instead\n");
-			ufs_clear_opt (*mount_options, ONERROR);
-			ufs_set_opt (*mount_options, ONERROR_REPAIR);
-			break;
-		default:
-			pr_err("Invalid option: \"%s\" or missing value\n", p);
-			return 0;
+	switch (opt) {
+	case Opt_type:
+		if (reconfigure &&
+		    (ctx->mount_options & UFS_MOUNT_UFSTYPE) != result.uint_32) {
+			pr_err("ufstype can't be changed during remount\n");
+			return -EINVAL;
 		}
+		ufs_clear_opt(ctx->mount_options, UFS_MOUNT_UFSTYPE);
+		ufs_set_opt(ctx->mount_options, result.uint_32);
+		break;
+	case Opt_onerror:
+		ufs_clear_opt(ctx->mount_options, UFS_MOUNT_ONERROR);
+		ufs_set_opt(ctx->mount_options, result.uint_32);
+		break;
+	default:
+		return -EINVAL;
 	}
-	return 1;
+	return 0;
 }
 
 /*
@@ -764,8 +712,10 @@ static u64 ufs_max_bytes(struct super_block *sb)
 	return res << uspi->s_bshift;
 }
 
-static int ufs_fill_super(struct super_block *sb, void *data, int silent)
+static int ufs_fill_super(struct super_block *sb, struct fs_context *fc)
 {
+	struct ufs_fs_context *ctx = fc->fs_private;
+	int silent = fc->sb_flags & SB_SILENT;
 	struct ufs_sb_info * sbi;
 	struct ufs_sb_private_info * uspi;
 	struct ufs_super_block_first * usb1;
@@ -803,16 +753,9 @@ static int ufs_fill_super(struct super_block *sb, void *data, int silent)
 	mutex_init(&sbi->s_lock);
 	spin_lock_init(&sbi->work_lock);
 	INIT_DELAYED_WORK(&sbi->sync_work, delayed_sync_fs);
-	/*
-	 * Set default mount options
-	 * Parse mount options
-	 */
-	sbi->s_mount_opt = 0;
-	ufs_set_opt (sbi->s_mount_opt, ONERROR_LOCK);
-	if (!ufs_parse_options ((char *) data, &sbi->s_mount_opt)) {
-		pr_err("wrong mount options\n");
-		goto failed;
-	}
+
+	sbi->s_mount_opt = ctx->mount_options;
+
 	if (!(sbi->s_mount_opt & UFS_MOUNT_UFSTYPE)) {
 		if (!silent)
 			pr_err("You didn't specify the type of your ufs filesystem\n\n"
@@ -820,7 +763,7 @@ static int ufs_fill_super(struct super_block *sb, void *data, int silent)
 			"sun|sunx86|44bsd|ufs2|5xbsd|old|hp|nextstep|nextstep-cd|openstep ...\n\n"
 			">>>WARNING<<< Wrong ufstype may corrupt your filesystem, "
 			"default is ufstype=old\n");
-		ufs_set_opt (sbi->s_mount_opt, UFSTYPE_OLD);
+		ufs_set_opt(sbi->s_mount_opt, UFS_MOUNT_UFSTYPE_OLD);
 	}
 
 	uspi = kzalloc(sizeof(struct ufs_sb_private_info), GFP_KERNEL);
@@ -1290,13 +1233,16 @@ static int ufs_fill_super(struct super_block *sb, void *data, int silent)
 	return -ENOMEM;
 }
 
-static int ufs_remount (struct super_block *sb, int *mount_flags, char *data)
+static int ufs_reconfigure(struct fs_context *fc)
 {
 	struct ufs_sb_private_info * uspi;
 	struct ufs_super_block_first * usb1;
 	struct ufs_super_block_third * usb3;
-	unsigned new_mount_opt, ufstype;
-	unsigned flags;
+	struct ufs_fs_context *ctx = fc->fs_private;
+	struct super_block *sb = fc->root->d_sb;
+	unsigned int new_mount_opt;
+	unsigned int ufstype;
+	unsigned int flags;
 
 	sync_filesystem(sb);
 	mutex_lock(&UFS_SB(sb)->s_lock);
@@ -1304,27 +1250,11 @@ static int ufs_remount (struct super_block *sb, int *mount_flags, char *data)
 	flags = UFS_SB(sb)->s_flags;
 	usb1 = ubh_get_usb_first(uspi);
 	usb3 = ubh_get_usb_third(uspi);
-	
-	/*
-	 * Allow the "check" option to be passed as a remount option.
-	 * It is not possible to change ufstype option during remount
-	 */
+
+	new_mount_opt = ctx->mount_options;
 	ufstype = UFS_SB(sb)->s_mount_opt & UFS_MOUNT_UFSTYPE;
-	new_mount_opt = 0;
-	ufs_set_opt (new_mount_opt, ONERROR_LOCK);
-	if (!ufs_parse_options (data, &new_mount_opt)) {
-		mutex_unlock(&UFS_SB(sb)->s_lock);
-		return -EINVAL;
-	}
-	if (!(new_mount_opt & UFS_MOUNT_UFSTYPE)) {
-		new_mount_opt |= ufstype;
-	} else if ((new_mount_opt & UFS_MOUNT_UFSTYPE) != ufstype) {
-		pr_err("ufstype can't be changed during remount\n");
-		mutex_unlock(&UFS_SB(sb)->s_lock);
-		return -EINVAL;
-	}
 
-	if ((bool)(*mount_flags & SB_RDONLY) == sb_rdonly(sb)) {
+	if ((bool)(fc->sb_flags & SB_RDONLY) == sb_rdonly(sb)) {
 		UFS_SB(sb)->s_mount_opt = new_mount_opt;
 		mutex_unlock(&UFS_SB(sb)->s_lock);
 		return 0;
@@ -1333,7 +1263,7 @@ static int ufs_remount (struct super_block *sb, int *mount_flags, char *data)
 	/*
 	 * fs was mouted as rw, remounting ro
 	 */
-	if (*mount_flags & SB_RDONLY) {
+	if (fc->sb_flags & SB_RDONLY) {
 		ufs_put_super_internal(sb);
 		usb1->fs_time = ufs_get_seconds(sb);
 		if ((flags & UFS_ST_MASK) == UFS_ST_SUN
@@ -1377,19 +1307,20 @@ static int ufs_remount (struct super_block *sb, int *mount_flags, char *data)
 static int ufs_show_options(struct seq_file *seq, struct dentry *root)
 {
 	struct ufs_sb_info *sbi = UFS_SB(root->d_sb);
-	unsigned mval = sbi->s_mount_opt & UFS_MOUNT_UFSTYPE;
-	const struct match_token *tp = tokens;
+	unsigned int mval;
+	const struct constant_table *tp;
 
-	while (tp->token != Opt_onerror_panic && tp->token != mval)
+	tp = ufs_param_ufstype;
+	mval = sbi->s_mount_opt & UFS_MOUNT_UFSTYPE;
+	while (tp->value && tp->value != mval)
 		++tp;
-	BUG_ON(tp->token == Opt_onerror_panic);
-	seq_printf(seq, ",%s", tp->pattern);
+	seq_printf(seq, ",ufstype=%s", tp->name);
 
+	tp = ufs_param_onerror;
 	mval = sbi->s_mount_opt & UFS_MOUNT_ONERROR;
-	while (tp->token != Opt_err && tp->token != mval)
+	while (tp->value && tp->value != mval)
 		++tp;
-	BUG_ON(tp->token == Opt_err);
-	seq_printf(seq, ",%s", tp->pattern);
+	seq_printf(seq, ",onerror=%s", tp->name);
 
 	return 0;
 }
@@ -1483,21 +1414,54 @@ static const struct super_operations ufs_super_ops = {
 	.put_super	= ufs_put_super,
 	.sync_fs	= ufs_sync_fs,
 	.statfs		= ufs_statfs,
-	.remount_fs	= ufs_remount,
 	.show_options   = ufs_show_options,
 };
 
-static struct dentry *ufs_mount(struct file_system_type *fs_type,
-	int flags, const char *dev_name, void *data)
+static int ufs_get_tree(struct fs_context *fc)
 {
-	return mount_bdev(fs_type, flags, dev_name, data, ufs_fill_super);
+	return get_tree_bdev(fc, ufs_fill_super);
+}
+
+static void ufs_free_fc(struct fs_context *fc)
+{
+	kfree(fc->fs_private);
+}
+
+static const struct fs_context_operations ufs_context_ops = {
+	.parse_param	= ufs_parse_param,
+	.get_tree	= ufs_get_tree,
+	.reconfigure	= ufs_reconfigure,
+	.free		= ufs_free_fc,
+};
+
+static int ufs_init_fs_context(struct fs_context *fc)
+{
+	struct ufs_fs_context *ctx;
+
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE) {
+		struct super_block *sb = fc->root->d_sb;
+		struct ufs_sb_info *sbi = UFS_SB(sb);
+
+		ctx->mount_options = sbi->s_mount_opt;
+	} else
+		ufs_set_opt(ctx->mount_options, UFS_MOUNT_ONERROR_LOCK);
+
+	fc->fs_private = ctx;
+	fc->ops = &ufs_context_ops;
+
+	return 0;
 }
 
 static struct file_system_type ufs_fs_type = {
 	.owner		= THIS_MODULE,
 	.name		= "ufs",
-	.mount		= ufs_mount,
 	.kill_sb	= kill_block_super,
+	.init_fs_context = ufs_init_fs_context,
+	.parameters	= ufs_param_spec,
 	.fs_flags	= FS_REQUIRES_DEV,
 };
 MODULE_ALIAS_FS("ufs");
diff --git a/fs/ufs/ufs.h b/fs/ufs/ufs.h
index e7df65dd4351..79afc8fe70ae 100644
--- a/fs/ufs/ufs.h
+++ b/fs/ufs/ufs.h
@@ -70,9 +70,9 @@ struct ufs_inode_info {
 #define UFS_MOUNT_UFSTYPE_UFS2		0x00001000
 #define UFS_MOUNT_UFSTYPE_SUNOS		0x00002000
 
-#define ufs_clear_opt(o,opt)	o &= ~UFS_MOUNT_##opt
-#define ufs_set_opt(o,opt)	o |= UFS_MOUNT_##opt
-#define ufs_test_opt(o,opt)	((o) & UFS_MOUNT_##opt)
+#define ufs_clear_opt(o, opt)	(o &= ~(opt))
+#define ufs_set_opt(o, opt)	(o |= (opt))
+#define ufs_test_opt(o, opt)	((o) & opt)
 
 /*
  * Debug code
-- 
2.48.0

Re: [PATCH] ufs: convert ufs to the new mount API

[Al Viro]

On Thu, Jan 16, 2025 at 12:49:32PM -0600, Eric Sandeen wrote:

> +	switch (opt) {
> +	case Opt_type:
> +		if (reconfigure &&
> +		    (ctx->mount_options & UFS_MOUNT_UFSTYPE) != result.uint_32) {
> +			pr_err("ufstype can't be changed during remount\n");
> +			return -EINVAL;
>  		}
> +		ufs_clear_opt(ctx->mount_options, UFS_MOUNT_UFSTYPE);
> +		ufs_set_opt(ctx->mount_options, result.uint_32);
> +		break;

Do we really want to support ufstype=foo,ufstype=bar?

> +static void ufs_free_fc(struct fs_context *fc)
> +{
> +	kfree(fc->fs_private);
> +}

Grr...  That's getting really annoying - we have way too many instances doing
exactly that.  Helper, perhaps?

> -#define ufs_set_opt(o,opt)	o |= UFS_MOUNT_##opt
> -#define ufs_test_opt(o,opt)	((o) & UFS_MOUNT_##opt)
> +#define ufs_clear_opt(o, opt)	(o &= ~(opt))
> +#define ufs_set_opt(o, opt)	(o |= (opt))
> +#define ufs_test_opt(o, opt)	((o) & opt)

I wonder if we would be better off without those macros (note, BTW,
that ufs_test_opt() is not used at all)...

Re: [PATCH] ufs: convert ufs to the new mount API

[Eric Sandeen]

On 1/16/25 1:08 PM, Al Viro wrote:
> On Thu, Jan 16, 2025 at 12:49:32PM -0600, Eric Sandeen wrote:
> 
>> +	switch (opt) {
>> +	case Opt_type:
>> +		if (reconfigure &&
>> +		    (ctx->mount_options & UFS_MOUNT_UFSTYPE) != result.uint_32) {
>> +			pr_err("ufstype can't be changed during remount\n");
>> +			return -EINVAL;
>>  		}
>> +		ufs_clear_opt(ctx->mount_options, UFS_MOUNT_UFSTYPE);
>> +		ufs_set_opt(ctx->mount_options, result.uint_32);
>> +		break;
> 
> Do we really want to support ufstype=foo,ufstype=bar?

well, we already do that today. Old code was:

                switch (token) {
                case Opt_type_old:
                        ufs_clear_opt (*mount_options, UFSTYPE);
                        ufs_set_opt (*mount_options, UFSTYPE_OLD);
                        break; 
                case Opt_type_sunx86:
                        ufs_clear_opt (*mount_options, UFSTYPE);
                        ufs_set_opt (*mount_options, UFSTYPE_SUNx86);
                        break;
...

so I was going for a straight conversion for now so that the behavior
was exactly the same (i.e. keep the last-specified type. I know, it's
weird, who would do that? Still. Don't break userspace? And we've been
burned before.)

> 
>> +static void ufs_free_fc(struct fs_context *fc)
>> +{
>> +	kfree(fc->fs_private);
>> +}
> 
> Grr...  That's getting really annoying - we have way too many instances doing
> exactly that.  Helper, perhaps?

Yeah ... should probably also decide if we should eliminate the default case
for every fs too, we should never hit default: barring programming errors.
And ISTR you asking for another helper a while ago... oh yeah, something like:

static inline bool fc_rdonly(const struct fs_context *fc)
{
	return fc->sb_flags & SB_RDONLY;
}

so at some point maybe we should make a treewide pass on this stuff?

>> -#define ufs_set_opt(o,opt)	o |= UFS_MOUNT_##opt
>> -#define ufs_test_opt(o,opt)	((o) & UFS_MOUNT_##opt)
>> +#define ufs_clear_opt(o, opt)	(o &= ~(opt))
>> +#define ufs_set_opt(o, opt)	(o |= (opt))
>> +#define ufs_test_opt(o, opt)	((o) & opt)
> 
> I wonder if we would be better off without those macros (note, BTW,
> that ufs_test_opt() is not used at all)...

*shrug* I was just going for the minimal transformation w/o any
"also, while we're at it ..." bits. (tho I guess I did remove the
BUG_ON.)

If there are changes you want to see /now/ though I'm happy to do it.

Thanks,
-Eric

Re: [PATCH] ufs: convert ufs to the new mount API

[Al Viro]

On Thu, Jan 16, 2025 at 04:07:44PM -0600, Eric Sandeen wrote:
> On 1/16/25 1:08 PM, Al Viro wrote:
> > On Thu, Jan 16, 2025 at 12:49:32PM -0600, Eric Sandeen wrote:
> > 
> >> +	switch (opt) {
> >> +	case Opt_type:
> >> +		if (reconfigure &&
> >> +		    (ctx->mount_options & UFS_MOUNT_UFSTYPE) != result.uint_32) {
> >> +			pr_err("ufstype can't be changed during remount\n");
> >> +			return -EINVAL;
> >>  		}
> >> +		ufs_clear_opt(ctx->mount_options, UFS_MOUNT_UFSTYPE);
> >> +		ufs_set_opt(ctx->mount_options, result.uint_32);
> >> +		break;
> > 
> > Do we really want to support ufstype=foo,ufstype=bar?
> 
> well, we already do that today. Old code was:
> 
>                 switch (token) {
>                 case Opt_type_old:
>                         ufs_clear_opt (*mount_options, UFSTYPE);
>                         ufs_set_opt (*mount_options, UFSTYPE_OLD);
>                         break; 
>                 case Opt_type_sunx86:
>                         ufs_clear_opt (*mount_options, UFSTYPE);
>                         ufs_set_opt (*mount_options, UFSTYPE_SUNx86);
>                         break;
> ...
> 
> so I was going for a straight conversion for now so that the behavior
> was exactly the same (i.e. keep the last-specified type. I know, it's
> weird, who would do that? Still. Don't break userspace? And we've been
> burned before.)

FWIW, see viro/vfs.git #work.ufs - separating ufs flavour and on-error
flags, dealing with -o ufstype conflicts, then your patch ported on
top of that.

Re: [PATCH] ufs: convert ufs to the new mount API

[Eric Sandeen]

On 1/17/25 2:11 AM, Al Viro wrote:
> On Thu, Jan 16, 2025 at 04:07:44PM -0600, Eric Sandeen wrote:
>> On 1/16/25 1:08 PM, Al Viro wrote:
>>> On Thu, Jan 16, 2025 at 12:49:32PM -0600, Eric Sandeen wrote:
>>>
>>>> +	switch (opt) {
>>>> +	case Opt_type:
>>>> +		if (reconfigure &&
>>>> +		    (ctx->mount_options & UFS_MOUNT_UFSTYPE) != result.uint_32) {
>>>> +			pr_err("ufstype can't be changed during remount\n");
>>>> +			return -EINVAL;
>>>>  		}
>>>> +		ufs_clear_opt(ctx->mount_options, UFS_MOUNT_UFSTYPE);
>>>> +		ufs_set_opt(ctx->mount_options, result.uint_32);
>>>> +		break;
>>>
>>> Do we really want to support ufstype=foo,ufstype=bar?
>>
>> well, we already do that today. Old code was:
>>
>>                 switch (token) {
>>                 case Opt_type_old:
>>                         ufs_clear_opt (*mount_options, UFSTYPE);
>>                         ufs_set_opt (*mount_options, UFSTYPE_OLD);
>>                         break; 
>>                 case Opt_type_sunx86:
>>                         ufs_clear_opt (*mount_options, UFSTYPE);
>>                         ufs_set_opt (*mount_options, UFSTYPE_SUNx86);
>>                         break;
>> ...
>>
>> so I was going for a straight conversion for now so that the behavior
>> was exactly the same (i.e. keep the last-specified type. I know, it's
>> weird, who would do that? Still. Don't break userspace? And we've been
>> burned before.)
> 
> FWIW, see viro/vfs.git #work.ufs - separating ufs flavour and on-error
> flags, dealing with -o ufstype conflicts, then your patch ported on
> top of that.


LGTM; you may want to edit my commit log, because this:

"ufs_set/clear/test_opt macros are changed to take the full option,
to facilitate setting in parse_param.

ufstype option changes during remount are rejected during parsing,
rather than after the fact as they were before."

no longer makes any sense; you did that already in your new patches
so these changes no longer exist in my patch.

Acked-by: Eric Sandeen <sandeen@redhat.com>

Thanks,
-Eric

Re: [PATCH] ufs: convert ufs to the new mount API

[Al Viro]

On Fri, Jan 17, 2025 at 08:58:36AM -0600, Eric Sandeen wrote:

> LGTM; you may want to edit my commit log, because this:
> 
> "ufs_set/clear/test_opt macros are changed to take the full option,
> to facilitate setting in parse_param.
> 
> ufstype option changes during remount are rejected during parsing,
> rather than after the fact as they were before."
> 
> no longer makes any sense; you did that already in your new patches
> so these changes no longer exist in my patch.

Done.