Re: [PATCH] udf: convert to new mount API

[Jan Kara <jack@suse.cz>]

On Fri 09-02-24 13:43:09, Eric Sandeen wrote:
> Convert the UDF filesystem to the new mount API.
> 
> UDF is slightly unique in that it always preserves prior mount
> options across a remount, so that's handled by udf_init_options().

Well, ext4 does this as well AFAICT. See e.g. ext4_apply_options() and how
it does:

        sbi->s_mount_opt &= ~ctx->mask_s_mount_opt;
        sbi->s_mount_opt |= ctx->vals_s_mount_opt;
        sbi->s_mount_opt2 &= ~ctx->mask_s_mount_opt2;
        sbi->s_mount_opt2 |= ctx->vals_s_mount_opt2;
        sb->s_flags &= ~ctx->mask_s_flags;
        sb->s_flags |= ctx->vals_s_flags;

so it really modifies the current superblock state, not overwrites it.

> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> Tested by running through xfstests, as well as some targeted testing of
> remount behavior.
> 
> NB: I did not convert i.e any udf_err() to errorf(fc, ) because the former
> does some nice formatting, not sure how or if you'd like to handle that, Jan?

Which one would you like to convert? I didn't find any obvious
candidates... Or do you mean the messages in udf_fill_super() when we find
on-disk inconsistencies or similar? I guess we can leave that for later
because propagating 'fc' into all the validation functions will be a lot of
churn.

> +static void udf_init_options(struct fs_context *fc, struct udf_options *uopt)
> +{
> +	if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE) {
> +		struct super_block *sb = fc->root->d_sb;
> +		struct udf_sb_info *sbi = UDF_SB(sb);
> +
> +		uopt->flags = sbi->s_flags;
> +		uopt->uid   = sbi->s_uid;
> +		uopt->gid   = sbi->s_gid;
> +		uopt->umask = sbi->s_umask;
> +		uopt->fmode = sbi->s_fmode;
> +		uopt->dmode = sbi->s_dmode;
> +		uopt->nls_map = NULL;
> +	} else {
> +		uopt->flags = (1 << UDF_FLAG_USE_AD_IN_ICB) | (1 << UDF_FLAG_STRICT);
> +		/* By default we'll use overflow[ug]id when UDF inode [ug]id == -1 */

Nit: Please wrap these two lines.

> +		uopt->uid = make_kuid(current_user_ns(), overflowuid);
> +		uopt->gid = make_kgid(current_user_ns(), overflowgid);
> +		uopt->umask = 0;
> +		uopt->fmode = UDF_INVALID_MODE;
> +		uopt->dmode = UDF_INVALID_MODE;
> +		uopt->nls_map = NULL;
> +		uopt->session = 0xFFFFFFFF;
> +	}
> +}
> +
> +static int udf_init_fs_context(struct fs_context *fc)
> +{
> +	struct udf_options *uopt;
> +
> +	uopt = kzalloc(sizeof(*uopt), GFP_KERNEL);
> +	if (!uopt)
> +		return -ENOMEM;
> +
> +	udf_init_options(fc, uopt);
> +
> +	fc->fs_private = uopt;
> +	fc->ops = &udf_context_ops;
> +
> +	return 0;
> +}
> +
> +static void udf_free_fc(struct fs_context *fc)
> +{
> +	kfree(fc->fs_private);
> +}

So I think we need to unload uopt->nls_map in case we eventually failed the
mount (which means we also need to zero uopt->nls_map if we copy it to the
sbi).

> +static const struct fs_parameter_spec udf_param_spec[] = {
> +	fsparam_flag	("novrs",		Opt_novrs),
> +	fsparam_flag	("nostrict",		Opt_nostrict),
> +	fsparam_u32	("bs",			Opt_bs),
> +	fsparam_flag	("unhide",		Opt_unhide),
> +	fsparam_flag	("undelete",		Opt_undelete),
> +	fsparam_flag	("noadinicb",		Opt_noadinicb),
> +	fsparam_flag	("adinicb",		Opt_adinicb),

We could actually use the fs_param_neg_with_no for the above. I don't
insist but it's an interesting exercise :)

> +	fsparam_flag	("shortad",		Opt_shortad),
> +	fsparam_flag	("longad",		Opt_longad),
> +	fsparam_string	("gid",			Opt_gid),
> +	fsparam_string	("uid",			Opt_uid),
> +	fsparam_u32	("umask",		Opt_umask),
> +	fsparam_u32	("session",		Opt_session),
> +	fsparam_u32	("lastblock",		Opt_lastblock),
> +	fsparam_u32	("anchor",		Opt_anchor),
> +	fsparam_u32	("volume",		Opt_volume),
> +	fsparam_u32	("partition",		Opt_partition),
> +	fsparam_u32	("fileset",		Opt_fileset),
> +	fsparam_u32	("rootdir",		Opt_rootdir),
> +	fsparam_flag	("utf8",		Opt_utf8),
> +	fsparam_string	("iocharset",		Opt_iocharset),
> +	fsparam_u32	("mode",		Opt_fmode),
> +	fsparam_u32	("dmode",		Opt_dmode),
> +	{}
> + };
> + 
> +static int udf_parse_param(struct fs_context *fc, struct fs_parameter *param)

...

> +	unsigned int n;
> +	struct udf_options *uopt = fc->fs_private;
> +	struct fs_parse_result result;
> +	int token;
> +	bool remount = (fc->purpose & FS_CONTEXT_FOR_RECONFIGURE);
> +
> +	token = fs_parse(fc, udf_param_spec, param, &result);
> +	if (token < 0)
> +		return token;
> +
> +	switch (token) {
> +	case Opt_novrs:
> +		uopt->novrs = 1;

I guess we can make this just an ordinary flag as a prep patch?

> +		break;
> +	case Opt_bs:
> +		n = result.uint_32;;
				  ^^ one is enough ;)

> +		if (n != 512 && n != 1024 && n != 2048 && n != 4096)
> +			return -EINVAL;
> +		uopt->blocksize = n;
> +		uopt->flags |= (1 << UDF_FLAG_BLOCKSIZE_SET);
> +		break;
> +	case Opt_unhide:
> +		uopt->flags |= (1 << UDF_FLAG_UNHIDE);
> +		break;
> +	case Opt_undelete:
> +		uopt->flags |= (1 << UDF_FLAG_UNDELETE);
> +		break;

These two are nops so we should deprecate them and completely ignore them.
I'm writing it here mostly as a reminder to myself as a work item after the
conversion is done :)

> +	case Opt_noadinicb:
> +		uopt->flags &= ~(1 << UDF_FLAG_USE_AD_IN_ICB);
> +		break;
> +	case Opt_adinicb:
> +		uopt->flags |= (1 << UDF_FLAG_USE_AD_IN_ICB);
> +		break;
> +	case Opt_shortad:
> +		uopt->flags |= (1 << UDF_FLAG_USE_SHORT_AD);
> +		break;
> +	case Opt_longad:
> +		uopt->flags &= ~(1 << UDF_FLAG_USE_SHORT_AD);
> +		break;
> +	case Opt_gid:
> +		if (0 == kstrtoint(param->string, 10, &uv)) {
Nit:
		    ^^ I prefer "kstrtoint() == 0"

Otherwise looks good.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR