[PATCH] Return more useful error number when acls are too large

[PATCH] Return more useful error number when acls are too large

[Andreas Gruenbacher]

Hello,

could you please add this to mainline? Getting EINVAL when an acl
becomes too large is quite confusing.



Index: linux-2.6.6-rc2/fs/ext2/acl.c
===================================================================
--- linux-2.6.6-rc2.orig/fs/ext2/acl.c	2004-04-20 23:29:46.000000000 +0200
+++ linux-2.6.6-rc2/fs/ext2/acl.c	2004-04-26 11:45:59.724792120 +0200
@@ -256,7 +256,7 @@
 	}
  	if (acl) {
 		if (acl->a_count > EXT2_ACL_MAX_ENTRIES)
-			return -EINVAL;
+			return -ENOSPC;
 		value = ext2_acl_to_disk(acl, &size);
 		if (IS_ERR(value))
 			return (int)PTR_ERR(value);
Index: linux-2.6.6-rc2/fs/ext3/acl.c
===================================================================
--- linux-2.6.6-rc2.orig/fs/ext3/acl.c	2004-04-20 23:28:53.000000000 +0200
+++ linux-2.6.6-rc2/fs/ext3/acl.c	2004-04-26 11:46:05.143968280 +0200
@@ -260,7 +260,7 @@
 	}
  	if (acl) {
 		if (acl->a_count > EXT3_ACL_MAX_ENTRIES)
-			return -EINVAL;
+			return -ENOSPC;
 		value = ext3_acl_to_disk(acl, &size);
 		if (IS_ERR(value))
 			return (int)PTR_ERR(value);


Thanks,
-- 
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX AG

Re: [PATCH] Return more useful error number when acls are too large

[Nathan Scott]

hi Andreas,

On Mon, Apr 26, 2004 at 12:27:58PM +0200, Andreas Gruenbacher wrote:
> Hello,
> 
> could you please add this to mainline? Getting EINVAL when an acl
> becomes too large is quite confusing.
> 
>   	if (acl) {
>  		if (acl->a_count > EXT2_ACL_MAX_ENTRIES)
> -			return -EINVAL;
> +			return -ENOSPC;

That seems an odd error code to change it to, since its not
related to the device running out of free space right?  Maybe
use -E2BIG instead?

XFS has a similar check (different limit as you know), so is
also affected by this; could you update XFS at the same time
with whatever value gets chosen, if its not EINVAL?  Or just
let me know what gets chosen & I'll fix it up later.

thanks.

-- 
Nathan

Re: [PATCH] Return more useful error number when acls are too large

[Andreas Gruenbacher]

[-- Attachment #1: Type: text/plain, Size: 1970 bytes --]

On Tue, 2004-04-27 at 03:24, Nathan Scott wrote:
> hi Andreas,
> 
> On Mon, Apr 26, 2004 at 12:27:58PM +0200, Andreas Gruenbacher wrote:
> > Hello,
> > 
> > could you please add this to mainline? Getting EINVAL when an acl
> > becomes too large is quite confusing.
> > 
> >   	if (acl) {
> >  		if (acl->a_count > EXT2_ACL_MAX_ENTRIES)
> > -			return -EINVAL;
> > +			return -ENOSPC;
> 
> That seems an odd error code to change it to, since its not
> related to the device running out of free space right?  Maybe
> use -E2BIG instead?

I don't see a problem with giving ENOSPC this particular meaning here.
The error number used must be among those defined for NFSv3, so E2BIG
won't do.

> XFS has a similar check (different limit as you know), so is
> also affected by this; could you update XFS at the same time
> with whatever value gets chosen, if its not EINVAL?  Or just
> let me know what gets chosen & I'll fix it up later.

I think this patch is correct for xfs. Nathan, would you mind to
double-check? Thanks.


Index: linux-2.6.6-rc2/fs/xfs/xfs_acl.c
===================================================================
--- linux-2.6.6-rc2.orig/fs/xfs/xfs_acl.c
+++ linux-2.6.6-rc2/fs/xfs/xfs_acl.c
@@ -148,10 +148,7 @@ posix_acl_xattr_to_xfs(
 			return EINVAL;
 		}
 	}
-	if (xfs_acl_invalid(dest))
-		return EINVAL;
-
-	return 0;
+	return xfs_acl_invalid(dest);
 }
 
 /*
@@ -249,10 +246,9 @@ xfs_acl_vget(
 	if (!size) {
 		error = -posix_acl_xattr_size(XFS_ACL_MAX_ENTRIES);
 	} else {
-		if (xfs_acl_invalid(xfs_acl)) {
-			error = EINVAL;
+		error = xfs_acl_invalid(xfs_acl);
+		if (error)
 			goto out;
-		}
 		if (kind == _ACL_TYPE_ACCESS) {
 			vattr_t	va;
 
@@ -590,7 +586,7 @@ xfs_acl_invalid(
 		goto acl_invalid;
 
 	if (aclp->acl_cnt > XFS_ACL_MAX_ENTRIES)
-		goto acl_invalid;
+		return ENOSPC;
 
 	for (i = 0; i < aclp->acl_cnt; i++) {
 		entry = &aclp->acl_entry[i];


Thanks,
-- 
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX AG

[-- Attachment #2: acl-too-large-2 --]
[-- Type: text/x-patch, Size: 872 bytes --]

Index: linux-2.6.6-rc2/fs/xfs/xfs_acl.c
===================================================================
--- linux-2.6.6-rc2.orig/fs/xfs/xfs_acl.c
+++ linux-2.6.6-rc2/fs/xfs/xfs_acl.c
@@ -148,10 +148,7 @@ posix_acl_xattr_to_xfs(
 			return EINVAL;
 		}
 	}
-	if (xfs_acl_invalid(dest))
-		return EINVAL;
-
-	return 0;
+	return xfs_acl_invalid(dest);
 }
 
 /*
@@ -249,10 +246,9 @@ xfs_acl_vget(
 	if (!size) {
 		error = -posix_acl_xattr_size(XFS_ACL_MAX_ENTRIES);
 	} else {
-		if (xfs_acl_invalid(xfs_acl)) {
-			error = EINVAL;
+		error = xfs_acl_invalid(xfs_acl);
+		if (error)
 			goto out;
-		}
 		if (kind == _ACL_TYPE_ACCESS) {
 			vattr_t	va;
 
@@ -590,7 +586,7 @@ xfs_acl_invalid(
 		goto acl_invalid;
 
 	if (aclp->acl_cnt > XFS_ACL_MAX_ENTRIES)
-		goto acl_invalid;
+		return ENOSPC;
 
 	for (i = 0; i < aclp->acl_cnt; i++) {
 		entry = &aclp->acl_entry[i];

Re: [PATCH] Return more useful error number when acls are too large

[J. Bruce Fields]

On Mon, Apr 26, 2004 at 12:27:58PM +0200, Andreas Gruenbacher wrote:
> could you please add this to mainline? Getting EINVAL when an acl
> becomes too large is quite confusing.

On my system, at least, "man acl_set_file" does explicitly say that
EINVAL is returned in this case.  Whether that should be considered a
bug in the documentation or the code I don't know....

--Bruce Fields

Re: [PATCH] Return more useful error number when acls are too large

[Andreas Gruenbacher]

On Tue, 2004-04-27 at 20:32, J. Bruce Fields wrote:
> On Mon, Apr 26, 2004 at 12:27:58PM +0200, Andreas Gruenbacher wrote:
> > could you please add this to mainline? Getting EINVAL when an acl
> > becomes too large is quite confusing.
> 
> On my system, at least, "man acl_set_file" does explicitly say that
> EINVAL is returned in this case.  Whether that should be considered a
> bug in the documentation or the code I don't know....

Indeed ... looking at POSIX 1003.1e draft 17 I now see that this
function is indeed supposed to return EINVAL in this case. I was
assuming that this case was undefined, but that was wrong. So let's
leave the code as is -- sorry.


Cheers,
-- 
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX AG