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;
Organization: Red Hat UK Ltd. Registered Address: Red Hat UK Ltd, Amberley
        Place, 107-111 Peascod Street, Windsor, Berkshire, SI4 1TE, United
        Kingdom.
        Registered in England and Wales under Company Registration No. 3798903
From:   David Howells <dhowells@redhat.com>
To:     lczerner@redhat.com
cc:     dhowells@redhat.com, viro@zeniv.linux.org.uk,
        linux-ext4@vger.kernel.org
Subject: Notes on ext4 mount API parsing stuff
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1020557.1588082682.1@warthog.procyon.org.uk>
Content-Transfer-Encoding: quoted-printable
Date:   Tue, 28 Apr 2020 15:04:42 +0100
Message-ID: <1020558.1588082682@warthog.procyon.org.uk>
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk

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 =3D 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] =3D param->string;
	param->string =3D NULL;

>...
>}
> ...
>static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *p=
aram)
>{
>	struct ext4_fs_context *ctx =3D fc->fs_private;
>	const struct mount_opts *m;
>	struct fs_parse_result result;
>	kuid_t uid;
>	kgid_t gid;
>	int token;
>
>	token =3D fs_parse(fc, ext4_param_specs, param, &result);
>	if (token < 0)
>		return token;
>
>#ifdef CONFIG_QUOTA
>	if (token =3D=3D Opt_usrjquota) {
>		if (!*param->string)
>			return unnote_qf_name(fc, USRQUOTA);
>		else
>			return note_qf_name(fc, USRQUOTA, param);
>	} else if (token =3D=3D 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 =3D result.uint_32;
>	}

Missing break directive?

>	for (m =3D ext4_mount_opts; m->token !=3D Opt_err; m++)
>		if (token =3D=3D 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 |=3D m->flags;
>
>	if (m->token =3D=3D 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 =3D=3D Opt_commit) {
>		if (result.uint_32 =3D=3D 0)
>			ctx->s_commit_interval =3D 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 =3D HZ * result.uint_32;
>		ctx->spec |=3D EXT4_SPEC_s_commit_interval;
>	} else if (token =3D=3D 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