[v6,08/12] xfs: refactor suffix_kstrtoint()

Commit Message

Ian Kent Oct. 16, 2019, 12:41 a.m. UTC

The mount-api doesn't have a "human unit" parse type yet so
the options that have values like "10k" etc. still need to
be converted by the fs.

But the value comes to the fs as a string (not a substring_t
type) so there's a need to change the conversion function to
take a character string instead.

When xfs is switched to use the new mount-api match_kstrtoint()
will no longer be used and will be removed.

Signed-off-by: Ian Kent <[e-mail]>
Reviewed-by: Brian Foster <[e-mail]>
---
 fs/xfs/xfs_super.c |   22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

Comments

Christoph Hellwig Oct. 16, 2019, 8:34 a.m. UTC | #1

On Wed, Oct 16, 2019 at 08:41:27AM +0800, Ian Kent wrote:
> The mount-api doesn't have a "human unit" parse type yet so
> the options that have values like "10k" etc. still need to
> be converted by the fs.
> 
> But the value comes to the fs as a string (not a substring_t
> type) so there's a need to change the conversion function to
> take a character string instead.
> 
> When xfs is switched to use the new mount-api match_kstrtoint()
> will no longer be used and will be removed.

Please use up the full 72 chars available for the commit log.

> +STATIC int
> +match_kstrtoint(const substring_t *s, unsigned int base, int *res)

No need for static on new/heavily modified functions, just use static.

Note that both this and suffix_kstrtoint don't really follow the
normal XFS prototype formatting style either.

> +	const char	*value;
> +	int ret;

Similarly here - either you follow the XFS style of tab alignining
the variable names for all variables, or for none, but a mix is very
odd.

Darrick J. Wong Oct. 16, 2019, 3:37 p.m. UTC | #2

On Wed, Oct 16, 2019 at 01:34:20AM -0700, Christoph Hellwig wrote:
> On Wed, Oct 16, 2019 at 08:41:27AM +0800, Ian Kent wrote:
> > The mount-api doesn't have a "human unit" parse type yet so
> > the options that have values like "10k" etc. still need to
> > be converted by the fs.
> > 
> > But the value comes to the fs as a string (not a substring_t
> > type) so there's a need to change the conversion function to
> > take a character string instead.
> > 
> > When xfs is switched to use the new mount-api match_kstrtoint()
> > will no longer be used and will be removed.
> 
> Please use up the full 72 chars available for the commit log.
> 
> > +STATIC int
> > +match_kstrtoint(const substring_t *s, unsigned int base, int *res)
> 
> No need for static on new/heavily modified functions, just use static.
> 
> Note that both this and suffix_kstrtoint don't really follow the
> normal XFS prototype formatting style either.
> 
> > +	const char	*value;
> > +	int ret;
> 
> Similarly here - either you follow the XFS style of tab alignining
> the variable names for all variables, or for none, but a mix is very
> odd.

Please follow the xfs style of tab aligning variables inside xfs.

--D

Patch

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 1f706cbf9624..e54348be2a17 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -110,13 +110,13 @@  static const match_table_t tokens = {
 
 
 STATIC int
-suffix_kstrtoint(const substring_t *s, unsigned int base, int *res)
+suffix_kstrtoint(const char *s, unsigned int base, int *res)
 {
 	int	last, shift_left_factor = 0, _res;
 	char	*value;
 	int	ret = 0;
 
-	value = match_strdup(s);
+	value = kstrdup(s, GFP_KERNEL);
 	if (!value)
 		return -ENOMEM;
 
@@ -141,6 +141,20 @@  suffix_kstrtoint(const substring_t *s, unsigned int base, int *res)
 	return ret;
 }
 
+STATIC int
+match_kstrtoint(const substring_t *s, unsigned int base, int *res)
+{
+	const char	*value;
+	int ret;
+
+	value = match_strdup(s);
+	if (!value)
+		return -ENOMEM;
+	ret = suffix_kstrtoint(value, base, res);
+	kfree(value);
+	return ret;
+}
+
 /*
  * This function fills in xfs_mount_t fields based on mount args.
  * Note: the superblock has _not_ yet been read in.
@@ -203,7 +217,7 @@  xfs_parseargs(
 				return -EINVAL;
 			break;
 		case Opt_logbsize:
-			if (suffix_kstrtoint(args, 10, &mp->m_logbsize))
+			if (match_kstrtoint(args, 10, &mp->m_logbsize))
 				return -EINVAL;
 			break;
 		case Opt_logdev:
@@ -219,7 +233,7 @@  xfs_parseargs(
 				return -ENOMEM;
 			break;
 		case Opt_allocsize:
-			if (suffix_kstrtoint(args, 10, &iosize))
+			if (match_kstrtoint(args, 10, &iosize))
 				return -EINVAL;
 			iosizelog = ffs(iosize) - 1;
 			break;