diff mbox series

xfs_quota: refactor code to generate id from name

Message ID 8b4b7edb-94b2-3bb1-9ede-73674db82330@redhat.com (mailing list archive)
State Superseded
Headers show
Series xfs_quota: refactor code to generate id from name | expand

Commit Message

Eric Sandeen May 9, 2020, 5:18 p.m. UTC
There's boilerplate for setting limits and warnings, where we have
a case statement for each of the 3 quota types, and from there call
3 different functions to configure each of the 3 types, each of which
calls its own version of id to string function... 

Refactor this so that the main function can call a generic id to string
conversion routine, and then call a common action.  This save a lot of
LOC.

I was looking at allowing xfs to bump out individual grace periods like
setquota can do, and this refactoring allows us to add new actions like
that without copyingall the boilerplate again.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

 edit.c |  196 ++++++++++++++++-----------------------------------------
 1 file changed, 51 insertions(+), 145 deletions(-)

Comments

Christoph Hellwig May 11, 2020, 3:32 p.m. UTC | #1
On Sat, May 09, 2020 at 12:18:42PM -0500, Eric Sandeen wrote:
> There's boilerplate for setting limits and warnings, where we have
> a case statement for each of the 3 quota types, and from there call
> 3 different functions to configure each of the 3 types, each of which
> calls its own version of id to string function... 
> 
> Refactor this so that the main function can call a generic id to string
> conversion routine, and then call a common action.  This save a lot of
> LOC.
> 
> I was looking at allowing xfs to bump out individual grace periods like
> setquota can do, and this refactoring allows us to add new actions like
> that without copyingall the boilerplate again.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
>  edit.c |  196 ++++++++++++++++-----------------------------------------
>  1 file changed, 51 insertions(+), 145 deletions(-)
> 
> diff --git a/quota/edit.c b/quota/edit.c
> index f9938b8a..70c0969f 100644
> --- a/quota/edit.c
> +++ b/quota/edit.c
> @@ -101,6 +101,42 @@ warn_help(void)
>  "\n"));
>  }
>  
> +static uint32_t
> +id_from_string(
> +	char	*name,
> +	int	type)
> +{
> +	uint32_t	id = -1;
> +
> +	switch (type) {
> +	case XFS_USER_QUOTA:
> +		id = uid_from_string(name);
> +		if (id == -1)
> +			fprintf(stderr, _("%s: invalid user name: %s\n"),
> +				progname, name);
> +		break;
> +	case XFS_GROUP_QUOTA:
> +		id = gid_from_string(name);
> +		if (id == -1)
> +			fprintf(stderr, _("%s: invalid group name: %s\n"),
> +				progname, name);
> +		break;
> +	case XFS_PROJ_QUOTA:
> +		id = prid_from_string(name);
> +		if (id == -1)
> +			fprintf(stderr, _("%s: invalid project name: %s\n"),
> +				progname, name);
> +		break;
> +	default:
> +		ASSERT(0);
> +		break;
> +	}
> +
> +	if (id == -1)
> +		exitcode = 1;
> +	return id;

What about de-duplicating the error printk as well?

static uint32_t
id_from_string(
	char		*name,
	int		type)
{
	uint32_t	id = -1;
	const char	*type = "invalid";

	switch (type) {
	case XFS_USER_QUOTA:
		type = "user";
		id = uid_from_string(name);
		break;
	case XFS_GROUP_QUOTA:
		type = "group";
		id = gid_from_string(name);
		break;
	case XFS_PROJ_QUOTA:
		type = "project";
		id = prid_from_string(name);
		break;
	default:
		ASSERT(0);
		break;
	}

	if (id == -1) {
		fprintf(stderr, _("%s: invalid %s name: %s\n"),
			type, progname, name);
		exitcode = 1;
	}
	return id;
}
Eric Sandeen May 11, 2020, 4 p.m. UTC | #2
On 5/11/20 10:32 AM, Christoph Hellwig wrote:
> On Sat, May 09, 2020 at 12:18:42PM -0500, Eric Sandeen wrote:

...

>> @@ -101,6 +101,42 @@ warn_help(void)
>>  "\n"));
>>  }
>>  
>> +static uint32_t
>> +id_from_string(
>> +	char	*name,
>> +	int	type)
>> +{
>> +	uint32_t	id = -1;
>> +
>> +	switch (type) {
>> +	case XFS_USER_QUOTA:
>> +		id = uid_from_string(name);
>> +		if (id == -1)
>> +			fprintf(stderr, _("%s: invalid user name: %s\n"),
>> +				progname, name);
>> +		break;
>> +	case XFS_GROUP_QUOTA:
>> +		id = gid_from_string(name);
>> +		if (id == -1)
>> +			fprintf(stderr, _("%s: invalid group name: %s\n"),
>> +				progname, name);
>> +		break;
>> +	case XFS_PROJ_QUOTA:
>> +		id = prid_from_string(name);
>> +		if (id == -1)
>> +			fprintf(stderr, _("%s: invalid project name: %s\n"),
>> +				progname, name);
>> +		break;
>> +	default:
>> +		ASSERT(0);
>> +		break;
>> +	}
>> +
>> +	if (id == -1)
>> +		exitcode = 1;
>> +	return id;
> 
> What about de-duplicating the error printk as well?

Oh yeah, good idea, I'll do that and send V2.

for some reason I always forget about handling things like this in
this way.  :)

Thanks,
-Eric
diff mbox series

Patch

diff --git a/quota/edit.c b/quota/edit.c
index f9938b8a..70c0969f 100644
--- a/quota/edit.c
+++ b/quota/edit.c
@@ -101,6 +101,42 @@  warn_help(void)
 "\n"));
 }
 
+static uint32_t
+id_from_string(
+	char	*name,
+	int	type)
+{
+	uint32_t	id = -1;
+
+	switch (type) {
+	case XFS_USER_QUOTA:
+		id = uid_from_string(name);
+		if (id == -1)
+			fprintf(stderr, _("%s: invalid user name: %s\n"),
+				progname, name);
+		break;
+	case XFS_GROUP_QUOTA:
+		id = gid_from_string(name);
+		if (id == -1)
+			fprintf(stderr, _("%s: invalid group name: %s\n"),
+				progname, name);
+		break;
+	case XFS_PROJ_QUOTA:
+		id = prid_from_string(name);
+		if (id == -1)
+			fprintf(stderr, _("%s: invalid project name: %s\n"),
+				progname, name);
+		break;
+	default:
+		ASSERT(0);
+		break;
+	}
+
+	if (id == -1)
+		exitcode = 1;
+	return id;
+}
+
 static void
 set_limits(
 	uint32_t	id,
@@ -135,75 +171,6 @@  set_limits(
 	}
 }
 
-static void
-set_user_limits(
-	char		*name,
-	uint		type,
-	uint		mask,
-	uint64_t	*bsoft,
-	uint64_t	*bhard,
-	uint64_t	*isoft,
-	uint64_t	*ihard,
-	uint64_t	*rtbsoft,
-	uint64_t	*rtbhard)
-{
-	uid_t		uid = uid_from_string(name);
-
-	if (uid == -1) {
-		exitcode = 1;
-		fprintf(stderr, _("%s: invalid user name: %s\n"),
-				progname, name);
-	} else
-		set_limits(uid, type, mask, fs_path->fs_name,
-				bsoft, bhard, isoft, ihard, rtbsoft, rtbhard);
-}
-
-static void
-set_group_limits(
-	char		*name,
-	uint		type,
-	uint		mask,
-	uint64_t	*bsoft,
-	uint64_t	*bhard,
-	uint64_t	*isoft,
-	uint64_t	*ihard,
-	uint64_t	*rtbsoft,
-	uint64_t	*rtbhard)
-{
-	gid_t		gid = gid_from_string(name);
-
-	if (gid == -1) {
-		exitcode = 1;
-		fprintf(stderr, _("%s: invalid group name: %s\n"),
-				progname, name);
-	} else
-		set_limits(gid, type, mask, fs_path->fs_name,
-				bsoft, bhard, isoft, ihard, rtbsoft, rtbhard);
-}
-
-static void
-set_project_limits(
-	char		*name,
-	uint		type,
-	uint		mask,
-	uint64_t	*bsoft,
-	uint64_t	*bhard,
-	uint64_t	*isoft,
-	uint64_t	*ihard,
-	uint64_t	*rtbsoft,
-	uint64_t	*rtbhard)
-{
-	prid_t		prid = prid_from_string(name);
-
-	if (prid == -1) {
-		exitcode = 1;
-		fprintf(stderr, _("%s: invalid project name: %s\n"),
-				progname, name);
-	} else
-		set_limits(prid, type, mask, fs_path->fs_name,
-				bsoft, bhard, isoft, ihard, rtbsoft, rtbhard);
-}
-
 /* extract number of blocks from an ascii string */
 static int
 extractb(
@@ -258,6 +225,7 @@  limit_f(
 	char		**argv)
 {
 	char		*name;
+	uint32_t	id;
 	uint64_t	bsoft, bhard, isoft, ihard, rtbsoft, rtbhard;
 	int		c, type = 0, mask = 0, flags = 0;
 	uint		bsize, ssize, endoptions;
@@ -339,20 +307,13 @@  limit_f(
 		return command_usage(&limit_cmd);
 	}
 
-	switch (type) {
-	case XFS_USER_QUOTA:
-		set_user_limits(name, type, mask,
-			&bsoft, &bhard, &isoft, &ihard, &rtbsoft, &rtbhard);
-		break;
-	case XFS_GROUP_QUOTA:
-		set_group_limits(name, type, mask,
-			&bsoft, &bhard, &isoft, &ihard, &rtbsoft, &rtbhard);
-		break;
-	case XFS_PROJ_QUOTA:
-		set_project_limits(name, type, mask,
-			&bsoft, &bhard, &isoft, &ihard, &rtbsoft, &rtbhard);
-		break;
-	}
+
+	id = id_from_string(name, type);
+	if (id >= 0)
+		set_limits(id, type, mask, fs_path->fs_name,
+			   &bsoft, &bhard, &isoft, &ihard, &rtbsoft, &rtbhard);
+	else
+		exitcode = -1;
 	return 0;
 }
 
@@ -561,63 +522,13 @@  set_warnings(
 	}
 }
 
-static void
-set_user_warnings(
-	char		*name,
-	uint		type,
-	uint		mask,
-	uint		value)
-{
-	uid_t		uid = uid_from_string(name);
-
-	if (uid == -1) {
-		exitcode = 1;
-		fprintf(stderr, _("%s: invalid user name: %s\n"),
-				progname, name);
-	} else
-		set_warnings(uid, type, mask, fs_path->fs_name, value);
-}
-
-static void
-set_group_warnings(
-	char		*name,
-	uint		type,
-	uint		mask,
-	uint		value)
-{
-	gid_t		gid = gid_from_string(name);
-
-	if (gid == -1) {
-		exitcode = 1;
-		fprintf(stderr, _("%s: invalid group name: %s\n"),
-				progname, name);
-	} else
-		set_warnings(gid, type, mask, fs_path->fs_name, value);
-}
-
-static void
-set_project_warnings(
-	char		*name,
-	uint		type,
-	uint		mask,
-	uint		value)
-{
-	prid_t		prid = prid_from_string(name);
-
-	if (prid == -1) {
-		exitcode = 1;
-		fprintf(stderr, _("%s: invalid project name: %s\n"),
-				progname, name);
-	} else
-		set_warnings(prid, type, mask, fs_path->fs_name, value);
-}
-
 static int
 warn_f(
 	int		argc,
 	char		**argv)
 {
 	char		*name;
+	uint32_t	id;
 	uint		value;
 	int		c, flags = 0, type = 0, mask = 0;
 
@@ -675,17 +586,12 @@  warn_f(
 		return command_usage(&warn_cmd);
 	}
 
-	switch (type) {
-	case XFS_USER_QUOTA:
-		set_user_warnings(name, type, mask, value);
-		break;
-	case XFS_GROUP_QUOTA:
-		set_group_warnings(name, type, mask, value);
-		break;
-	case XFS_PROJ_QUOTA:
-		set_project_warnings(name, type, mask, value);
-		break;
-	}
+	id = id_from_string(name, type);
+	if (id >= 0)
+		set_warnings(id, type, mask, fs_path->fs_name, value);
+	else
+		exitcode = -1;
+
 	return 0;
 }