Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 29 Apr 2020 13:15:01 +0200
From:   Lukas Czerner <lczerner@redhat.com>
To:     David Howells <dhowells@redhat.com>
Cc:     viro@zeniv.linux.org.uk, linux-ext4@vger.kernel.org
Subject: Re: Notes on ext4 mount API parsing stuff
Message-ID: <20200429111501.lsshz3cd4lvur3gh@work>
References: <1020558.1588082682@warthog.procyon.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1020558.1588082682@warthog.procyon.org.uk>
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk

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
>