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@xxxxxxxxxx>
> ---
>
> 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@xxxxxxxx>
SUSE Labs, CR
