Hi Lukas,
Here are some notes on your ext4 mount API parsing stuff.
>static int note_qf_name(struct fs_context *fc, int qtype,
> struct fs_parameter *param)
>{
>...
> qname = kmemdup_nul(param->string, param->size, GFP_KERNEL);
No need to do this. You're allowed to steal param->string. Just NULL it out
afterwards. It's guaranteed to be NUL-terminated.
ctx->s_qf_names[qtype] = param->string;
param->string = NULL;
>...
>}
> ...
>static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
>{
> struct ext4_fs_context *ctx = fc->fs_private;
> const struct mount_opts *m;
> struct fs_parse_result result;
> kuid_t uid;
> kgid_t gid;
> int token;
>
> token = fs_parse(fc, ext4_param_specs, param, &result);
> if (token < 0)
> return token;
>
>#ifdef CONFIG_QUOTA
> if (token == Opt_usrjquota) {
> if (!*param->string)
> return unnote_qf_name(fc, USRQUOTA);
> else
> return note_qf_name(fc, USRQUOTA, param);
> } else if (token == Opt_grpjquota) {
> if (!*param->string)
> return unnote_qf_name(fc, GRPQUOTA);
> else
> return note_qf_name(fc, GRPQUOTA, param);
> }
>#endif
Merge this into the switch-statement below?
> switch (token) {
> case Opt_noacl:
> case Opt_nouser_xattr:
> ext4_msg(NULL, KERN_WARNING, deprecated_msg, param->key, "3.5");
> break;
> case Opt_removed:
> ext4_msg(NULL, KERN_WARNING, "Ignoring removed %s option",
> param->key);
> return 0;
> case Opt_abort:
> set_mount_flags(ctx, EXT4_MF_FS_ABORTED);
> return 0;
> case Opt_i_version:
> set_flags(ctx, SB_I_VERSION);
> return 0;
> case Opt_lazytime:
> set_flags(ctx, SB_LAZYTIME);
> return 0;
> case Opt_nolazytime:
> clear_flags(ctx, SB_LAZYTIME);
> return 0;
> case Opt_errors:
> case Opt_data:
> case Opt_data_err:
> case Opt_jqfmt:
> token = result.uint_32;
> }
Missing break directive?
> for (m = ext4_mount_opts; m->token != Opt_err; m++)
> if (token == m->token)
> break;
I guess this can't be turned into a direct array lookup given what else
ext4_mount_opts[] is used for.
> ctx->opt_flags |= m->flags;
>
> if (m->token == Opt_err) {
> ext4_msg(NULL, KERN_ERR, "Unrecognized mount option \"%s\" "
> "or missing value", param->key);
> return -EINVAL;
> }
>
> if (m->flags & MOPT_EXPLICIT) {
> if (m->mount_opt & EXT4_MOUNT_DELALLOC) {
> set_mount_opt2(ctx, EXT4_MOUNT2_EXPLICIT_DELALLOC);
> } else if (m->mount_opt & EXT4_MOUNT_JOURNAL_CHECKSUM) {
> set_mount_opt2(ctx,
> EXT4_MOUNT2_EXPLICIT_JOURNAL_CHECKSUM);
> } else
> return -EINVAL;
> }
> if (m->flags & MOPT_CLEAR_ERR)
> clear_mount_opt(ctx, EXT4_MOUNT_ERRORS_MASK);
>
> if (m->flags & MOPT_NOSUPPORT) {
> ext4_msg(NULL, KERN_ERR, "%s option not supported",
> param->key);
> } else if (token == Opt_commit) {
> if (result.uint_32 == 0)
> ctx->s_commit_interval = JBD2_DEFAULT_MAX_COMMIT_AGE;
> else if (result.uint_32 > INT_MAX / HZ) {
> ext4_msg(NULL, KERN_ERR,
> "Invalid commit interval %d, "
> "must be smaller than %d",
> result.uint_32, INT_MAX / HZ);
> return -EINVAL;
You're doing this a lot. It might be worth making a macro something like:
#define ext4_inval(fmt, ...) \
({ ext4_msg(NULL, KERN_ERR, ## __VA_LIST__), -EINVAL })
then you can just do:
return ext4_inval("Invalid commit interval %d, must be smaller than %d",
result.uint_32, INT_MAX / HZ);
> }
> ctx->s_commit_interval = HZ * result.uint_32;
> ctx->spec |= EXT4_SPEC_s_commit_interval;
> } else if (token == Opt_debug_want_extra_isize) {
This whole thing looks like it might be better as a switch-statement.
> }
> return 0;
>}
>
>static int parse_options(struct fs_context *fc, char *options)
>{
>}
I wonder if this could be replaced with a call to generic_parse_monolithic() -
though that calls security_sb_eat_lsm_opts() which you might not want.
David
