diff mbox series

[v2] xfsprogs: xfs_quota command error message improvement

Message ID 20200622131319.7717-1-billodo@redhat.com (mailing list archive)
State Superseded, archived
Headers show
Series [v2] xfsprogs: xfs_quota command error message improvement | expand

Commit Message

Bill O'Donnell June 22, 2020, 1:13 p.m. UTC
Make the error messages for rudimentary xfs_quota commands
(off, enable, disable) more user friendly, instead of the
terse sys error outputs.

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---

v2: enable internationalization and capitalize new message strings.

 quota/state.c | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

Comments

Darrick J. Wong June 22, 2020, 3:33 p.m. UTC | #1
On Mon, Jun 22, 2020 at 08:13:19AM -0500, Bill O'Donnell wrote:
> Make the error messages for rudimentary xfs_quota commands
> (off, enable, disable) more user friendly, instead of the
> terse sys error outputs.
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>

Yay, another sharp edge smoothed down,

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> v2: enable internationalization and capitalize new message strings.
> 
>  quota/state.c | 34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/quota/state.c b/quota/state.c
> index 8f9718f1..7c9fe517 100644
> --- a/quota/state.c
> +++ b/quota/state.c
> @@ -306,8 +306,15 @@ enable_enforcement(
>  		return;
>  	}
>  	dir = mount->fs_name;
> -	if (xfsquotactl(XFS_QUOTAON, dir, type, 0, (void *)&qflags) < 0)
> -		perror("XFS_QUOTAON");
> +	if (xfsquotactl(XFS_QUOTAON, dir, type, 0, (void *)&qflags) < 0) {
> +		if (errno == EEXIST)
> +			fprintf(stderr, _("Quota enforcement already enabled.\n"));
> +		else if (errno == EINVAL)
> +			fprintf(stderr,
> +				_("Can't enable when quotas are off.\n"));
> +		else
> +			perror("XFS_QUOTAON");
> +	}
>  	else if (flags & VERBOSE_FLAG)
>  		state_quotafile_mount(stdout, type, mount, flags);
>  }
> @@ -328,8 +335,16 @@ disable_enforcement(
>  		return;
>  	}
>  	dir = mount->fs_name;
> -	if (xfsquotactl(XFS_QUOTAOFF, dir, type, 0, (void *)&qflags) < 0)
> -		perror("XFS_QUOTAOFF");
> +	if (xfsquotactl(XFS_QUOTAOFF, dir, type, 0, (void *)&qflags) < 0) {
> +		if (errno == EEXIST)
> +			fprintf(stderr,
> +				_("Quota enforcement already disabled.\n"));
> +		else if (errno == EINVAL)
> +			fprintf(stderr,
> +				_("Can't disable when quotas are off.\n"));
> +		else
> +			perror("XFS_QUOTAOFF");
> +	}
>  	else if (flags & VERBOSE_FLAG)
>  		state_quotafile_mount(stdout, type, mount, flags);
>  }
> @@ -350,8 +365,15 @@ quotaoff(
>  		return;
>  	}
>  	dir = mount->fs_name;
> -	if (xfsquotactl(XFS_QUOTAOFF, dir, type, 0, (void *)&qflags) < 0)
> -		perror("XFS_QUOTAOFF");
> +	if (xfsquotactl(XFS_QUOTAOFF, dir, type, 0, (void *)&qflags) < 0) {
> +		if (errno == EEXIST)
> +			fprintf(stderr, _("Quota already off.\n"));
> +		else if (errno == EINVAL)
> +			fprintf(stderr,
> +				_("Can't disable when quotas are off.\n"));
> +		else
> +			perror("XFS_QUOTAOFF");
> +	}
>  	else if (flags & VERBOSE_FLAG)
>  		state_quotafile_mount(stdout, type, mount, flags);
>  }
> -- 
> 2.26.2
>
Eric Sandeen June 23, 2020, 10:30 p.m. UTC | #2
On 6/22/20 8:13 AM, Bill O'Donnell wrote:
> @@ -350,8 +365,15 @@ quotaoff(
>  		return;
>  	}
>  	dir = mount->fs_name;
> -	if (xfsquotactl(XFS_QUOTAOFF, dir, type, 0, (void *)&qflags) < 0)
> -		perror("XFS_QUOTAOFF");
> +	if (xfsquotactl(XFS_QUOTAOFF, dir, type, 0, (void *)&qflags) < 0) {
> +		if (errno == EEXIST)
> +			fprintf(stderr, _("Quota already off.\n"));
> +		else if (errno == EINVAL)
> +			fprintf(stderr,
> +				_("Can't disable when quotas are off.\n"));

Is this the right message here?  We get here from off_f(), which disables
enforcement and accounting, so I'm not sure "can't disable" makes sense
if "disable" means "disable enforcement" as it did in disable_enforcement()...?

(IOWs, have can you provoke EINVAL?  How?  Sorry, this just kind of jumps out
at me because "can't disable" seems a little out of place in quotaoff() so
I want to double check.)

Thanks,
-Eric
Bill O'Donnell June 23, 2020, 11:07 p.m. UTC | #3
On Tue, Jun 23, 2020 at 05:30:38PM -0500, Eric Sandeen wrote:
> On 6/22/20 8:13 AM, Bill O'Donnell wrote:
> > @@ -350,8 +365,15 @@ quotaoff(
> >  		return;
> >  	}
> >  	dir = mount->fs_name;
> > -	if (xfsquotactl(XFS_QUOTAOFF, dir, type, 0, (void *)&qflags) < 0)
> > -		perror("XFS_QUOTAOFF");
> > +	if (xfsquotactl(XFS_QUOTAOFF, dir, type, 0, (void *)&qflags) < 0) {
> > +		if (errno == EEXIST)
> > +			fprintf(stderr, _("Quota already off.\n"));
> > +		else if (errno == EINVAL)
> > +			fprintf(stderr,
> > +				_("Can't disable when quotas are off.\n"));
> 
> Is this the right message here?  We get here from off_f(), which disables
> enforcement and accounting, so I'm not sure "can't disable" makes sense
> if "disable" means "disable enforcement" as it did in disable_enforcement()...?
> 
> (IOWs, have can you provoke EINVAL?  How?  Sorry, this just kind of jumps out
> at me because "can't disable" seems a little out of place in quotaoff() so
> I want to double check.)

You're right. It was a copy/paste from the disable case. You also have a good point
in that I don't think EINVAL is ever provoked for the OFF case. I'll recheck it.

Thanks-
Bill

> 
> Thanks,
> -Eric
>
diff mbox series

Patch

diff --git a/quota/state.c b/quota/state.c
index 8f9718f1..7c9fe517 100644
--- a/quota/state.c
+++ b/quota/state.c
@@ -306,8 +306,15 @@  enable_enforcement(
 		return;
 	}
 	dir = mount->fs_name;
-	if (xfsquotactl(XFS_QUOTAON, dir, type, 0, (void *)&qflags) < 0)
-		perror("XFS_QUOTAON");
+	if (xfsquotactl(XFS_QUOTAON, dir, type, 0, (void *)&qflags) < 0) {
+		if (errno == EEXIST)
+			fprintf(stderr, _("Quota enforcement already enabled.\n"));
+		else if (errno == EINVAL)
+			fprintf(stderr,
+				_("Can't enable when quotas are off.\n"));
+		else
+			perror("XFS_QUOTAON");
+	}
 	else if (flags & VERBOSE_FLAG)
 		state_quotafile_mount(stdout, type, mount, flags);
 }
@@ -328,8 +335,16 @@  disable_enforcement(
 		return;
 	}
 	dir = mount->fs_name;
-	if (xfsquotactl(XFS_QUOTAOFF, dir, type, 0, (void *)&qflags) < 0)
-		perror("XFS_QUOTAOFF");
+	if (xfsquotactl(XFS_QUOTAOFF, dir, type, 0, (void *)&qflags) < 0) {
+		if (errno == EEXIST)
+			fprintf(stderr,
+				_("Quota enforcement already disabled.\n"));
+		else if (errno == EINVAL)
+			fprintf(stderr,
+				_("Can't disable when quotas are off.\n"));
+		else
+			perror("XFS_QUOTAOFF");
+	}
 	else if (flags & VERBOSE_FLAG)
 		state_quotafile_mount(stdout, type, mount, flags);
 }
@@ -350,8 +365,15 @@  quotaoff(
 		return;
 	}
 	dir = mount->fs_name;
-	if (xfsquotactl(XFS_QUOTAOFF, dir, type, 0, (void *)&qflags) < 0)
-		perror("XFS_QUOTAOFF");
+	if (xfsquotactl(XFS_QUOTAOFF, dir, type, 0, (void *)&qflags) < 0) {
+		if (errno == EEXIST)
+			fprintf(stderr, _("Quota already off.\n"));
+		else if (errno == EINVAL)
+			fprintf(stderr,
+				_("Can't disable when quotas are off.\n"));
+		else
+			perror("XFS_QUOTAOFF");
+	}
 	else if (flags & VERBOSE_FLAG)
 		state_quotafile_mount(stdout, type, mount, flags);
 }