On Mon, Jul 02, 2012 at 03:05:14PM +0800, Wanlong Gao wrote:
> remove the mount options macro, use tokens instead.
>
> CC: Ben Myers <bpm@xxxxxxx>
> CC: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> CC: Dave Chinner <david@xxxxxxxxxxxxx>
> CC: Zach Brown <zab@xxxxxxxxx>
> Signed-off-by: Wanlong Gao <gaowanlong@xxxxxxxxxxxxxx>
> ---
> fs/xfs/xfs_super.c | 510 ++++++++++++++++++++++++++++++++---------------------
> 1 file changed, 307 insertions(+), 203 deletions(-)
....
One thing about duplication:
> +#define MNTOPT_LOGBUFS "logbufs"
> +#define MNTOPT_LOGBSIZE "logbsize"
> +#define MNTOPT_LOGDEV "logdev"
....
> static const match_table_t tokens = {
> - {Opt_barrier, "barrier"},
> - {Opt_nobarrier, "nobarrier"},
> + {Opt_logbufs, "logbufs=%d"}, /* number of XFS log buffers */
> + {Opt_logbsize, "logbsize=%s"}, /* size of XFS log buffers */
> + {Opt_logdev, "logdev=%s"}, /* log device */
You're effectively defining each string twice, which is almost
certainly going to lead to one but nothe other being updated in
future.
Can something like:
{Opt_logbufs, MNTOPT_LOGBUFS "=%d"}, /* number of XFS log buffers */
{Opt_logbsize,MNTOPT_LOGBSIZE "=%s"}, /* size of XFS log buffers */
{Opt_logdev, MNTOPT_LOGDEV "=%s"}, /* log device */
be done to avoid that duplication?
> + token = match_token(p, tokens, args);
> + switch (token) {
> + case Opt_logbufs:
> + intarg = 0;
> + ret = EINVAL;
> + match_int(args, &intarg);
> + if (!intarg)
> + goto free_orig;
Why is a value of zero an error? match_int() returns an error if it
fails....
> + mp->m_logbufs = intarg;
> + break;
I don't really like the "set error, call function, jump to error"
pattern. I'd prefer just to set the error value when an error
returns as it makes the code much easier and more logical to read.
i.e:
token = match_token(p, tokens, args);
switch (token) {
case Opt_logbufs:
ret = match_int(args, &intarg);
if (ret)
goto free_orig;
mp->m_logbufs = intarg;
break;
> + case Opt_logbsize:
> + ret = ENOMEM;
> + string = match_strdup(args);
> + if (!string)
> + goto free_orig;
> + ret = EINVAL;
> + intarg = suffix_strtoul(string, &eov, 0);
> + if (!intarg)
> + goto free_string;
So this is just an open coded version of match_int() using the
suffix_strtoul() rather than simple_strtoul() directly. Please write
a "suffix_match_int()" wrapper for it, and that avoids all the messy
error handling in this code....
> + break;
> + case Opt_logdev:
> + ret = ENOMEM;
> + string = match_strdup(args);
> + if (!string)
> + goto free_orig;
> +
> + mp->m_logname = kstrndup(string, MAXNAMELEN, GFP_KERNEL);
> if (!mp->m_logname)
> + goto free_string;
and is consistent with this and other similar code.
> + case Opt_biosize:
> + intarg = 0;
> + ret = EINVAL;
> + intarg = match_int(args, &intarg);
That's not valid. match_int() returns either 0 or -EINVAL/-ENOMEM,
and that will overwrite whatever value match_int() writes into
intarg when it decodes it.
> + if (!intarg)
> + goto free_orig;
hence on a correct mount option with value, it will abort with no
error.
> + iosizelog = ffs(intarg) - 1;
And on a bad value, it will write something bad into iosizelog and
continue.
> + case Opt_allocsize:
> + ret = ENOMEM;
> + string = match_strdup(args);
> + if (!string)
> + goto free_orig;
> + ret = EINVAL;
> + intarg = suffix_strtoul(string, &eov, 0);
> + if (!intarg)
> + goto free_string;
Why is a value of zero an error?
....
> + case Opt_delaylog:
> xfs_warn(mp,
> + "delaylog is the default now, option is deprecated.");
> + break;
> + case Opt_nodelaylog:
> xfs_warn(mp,
> + "nodelaylog support has been removed, option is deprecated.");
Hard coding mount options here again.....
> + break;
> + case Opt_discard:
> mp->m_flags |= XFS_MOUNT_DISCARD;
> + break;
> + case Opt_nodiscard:
> mp->m_flags &= ~XFS_MOUNT_DISCARD;
> + break;
Move these above all the deprecated options.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
