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
On Tue, Apr 28, 2020 at 03:04:42PM +0100, David Howells wrote:
> Hi Lukas,
>
> Here are some notes on your ext4 mount API parsing stuff.
Er... is this a response to Lukas' patchset "ext4: new mount API
conversion" from 6 Nov 2019?
--D
> >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
>
Darrick J. Wong <[email protected]> wrote:
> > Here are some notes on your ext4 mount API parsing stuff.
>
> Er... is this a response to Lukas' patchset "ext4: new mount API
> conversion" from 6 Nov 2019?
Lukas says that's out of date.
David
On Tue, Apr 28, 2020 at 04:59:03PM +0100, David Howells wrote:
> Darrick J. Wong <[email protected]> wrote:
>
> > > Here are some notes on your ext4 mount API parsing stuff.
> >
> > Er... is this a response to Lukas' patchset "ext4: new mount API
> > conversion" from 6 Nov 2019?
>
> Lukas says that's out of date.
>
> David
Yeah, I asked David off-list to help me track the issue I was seeing.
But since he already replied here I went ahead and sent the new version
of the patch set. Sorry for the confusion.
Thanks David,
-Lukas
On Tue, Apr 28, 2020 at 03:04:42PM +0100, David Howells wrote:
> 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;
Good to know, I'll do that.
>
> >...
> >}
> > ...
> >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?
Yes, I'd like to. But I am trying to avoid cleanup changes that are not
necessarily needed for the API conversion. So yes, I will do this, but
with a separate series, there is a lot more to clean up as well.
>
> > 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?
yep, thanks.
>
> > 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.
Yes, unfortunatelly. But I'd like to change that in the cleanup series
after the conversion.
>
> > 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);
Yeah, it might be worth doing. Thanks.
>
> > }
> > 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.
Indeed, but again not as a part of api conversion.
>
> > }
> > 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.
This is eventually only used to parse mount options in super block, so
we do not want to call security_sb_eat_lsm_opts(). Other than that it's
basically exactly what generic_parse_monolithic() does.
Thanks!
-Lukas
>
> David
>