[PATCH] xfs: return ENOSPC when trying to set more ACLs than XFS_ACL_MAX_ENTRIES

[PATCH] xfs: return ENOSPC when trying to set more ACLs than XFS_ACL_MAX_ENTRIES

[Jeff Liu]

From: Jie Liu <jeff.liu@oracle.com>

We currently return EINVAL when trying to set more ACL entries than
XFS_ACL_MAX_ENTRIES(), but it would be a bit more meaningful to return
ENOSPC in this situation, because the later is used to indicate there
is no more space to store new ACLs IMHO.

Signed-off-by: Jie Liu <jeff.liu@oracle.com>
---
 fs/xfs/xfs_acl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
index 370eb3e..4e54a4d 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -397,7 +397,7 @@ xfs_xattr_acl_set(struct dentry *dentry, const char *name,
 	if (error)
 		goto out_release;
 
-	error = -EINVAL;
+	error = -ENOSPC;
 	if (acl->a_count > XFS_ACL_MAX_ENTRIES(XFS_M(inode->i_sb)))
 		goto out_release;
 
-- 
1.8.3.2

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: return ENOSPC when trying to set more ACLs than XFS_ACL_MAX_ENTRIES

[Eric Sandeen]

On 11/24/13, 9:36 AM, Jeff Liu wrote:
> From: Jie Liu <jeff.liu@oracle.com>
> 
> We currently return EINVAL when trying to set more ACL entries than
> XFS_ACL_MAX_ENTRIES(), but it would be a bit more meaningful to return
> ENOSPC in this situation, because the later is used to indicate there
> is no more space to store new ACLs IMHO.

I'm not quite convinced that it's better; the user will get an
error string of "no space left on device" which is misleading too,
and I'd argue that it's no better than "invalid argument."

To me, I think it's not worth changing, but others may disagree.

(I guess looking at ext4, it uses ENOSPC for some similar constraints,
so maybe three is precedent for this)

The setxattr(2) man page says:

> If there is insufficient space remaining to store the extended
> attribute, errno is set to either ENOSPC

but it doesn't say space in _what_ ...

-Eric

> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
> ---
>  fs/xfs/xfs_acl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> index 370eb3e..4e54a4d 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
> @@ -397,7 +397,7 @@ xfs_xattr_acl_set(struct dentry *dentry, const char *name,
>  	if (error)
>  		goto out_release;
>  
> -	error = -EINVAL;
> +	error = -ENOSPC;
>  	if (acl->a_count > XFS_ACL_MAX_ENTRIES(XFS_M(inode->i_sb)))
>  		goto out_release;
>  
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: return ENOSPC when trying to set more ACLs than XFS_ACL_MAX_ENTRIES

[Jeff Liu]

On 11/25 2013 11:34 AM, Eric Sandeen wrote:
> On 11/24/13, 9:36 AM, Jeff Liu wrote:
>> From: Jie Liu <jeff.liu@oracle.com>
>>
>> We currently return EINVAL when trying to set more ACL entries than
>> XFS_ACL_MAX_ENTRIES(), but it would be a bit more meaningful to return
>> ENOSPC in this situation, because the later is used to indicate there
>> is no more space to store new ACLs IMHO.
> 
> I'm not quite convinced that it's better; the user will get an
> error string of "no space left on device" which is misleading too,
I admit that both looks misleading...
> and I'd argue that it's no better than "invalid argument."
> 
> To me, I think it's not worth changing, but others may disagree.
> 
> (I guess looking at ext4, it uses ENOSPC for some similar constraints,
> so maybe three is precedent for this)
Btrfs also uses ENOSPC, but JFS would return something like "Argument list too long"
in this case.

Thanks,
-Jeff
> 
> The setxattr(2) man page says:
> 
>> If there is insufficient space remaining to store the extended
>> attribute, errno is set to either ENOSPC
> 
> but it doesn't say space in _what_ ...
> 
> -Eric
> 
>> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
>> ---
>>  fs/xfs/xfs_acl.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
>> index 370eb3e..4e54a4d 100644
>> --- a/fs/xfs/xfs_acl.c
>> +++ b/fs/xfs/xfs_acl.c
>> @@ -397,7 +397,7 @@ xfs_xattr_acl_set(struct dentry *dentry, const char *name,
>>  	if (error)
>>  		goto out_release;
>>  
>> -	error = -EINVAL;
>> +	error = -ENOSPC;
>>  	if (acl->a_count > XFS_ACL_MAX_ENTRIES(XFS_M(inode->i_sb)))
>>  		goto out_release;
>>  
>>
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: return ENOSPC when trying to set more ACLs than XFS_ACL_MAX_ENTRIES

[Ben Myers]

Hey Jeff,

On Mon, Nov 25, 2013 at 12:57:15PM +0800, Jeff Liu wrote:
> 
> On 11/25 2013 11:34 AM, Eric Sandeen wrote:
> > On 11/24/13, 9:36 AM, Jeff Liu wrote:
> >> From: Jie Liu <jeff.liu@oracle.com>
> >>
> >> We currently return EINVAL when trying to set more ACL entries than
> >> XFS_ACL_MAX_ENTRIES(), but it would be a bit more meaningful to return
> >> ENOSPC in this situation, because the later is used to indicate there
> >> is no more space to store new ACLs IMHO.
> > 
> > I'm not quite convinced that it's better; the user will get an
> > error string of "no space left on device" which is misleading too,
> I admit that both looks misleading...
> > and I'd argue that it's no better than "invalid argument."
> > 
> > To me, I think it's not worth changing, but others may disagree.
> > 
> > (I guess looking at ext4, it uses ENOSPC for some similar constraints,
> > so maybe three is precedent for this)
> Btrfs also uses ENOSPC, but JFS would return something like "Argument list too long"
> in this case.

I tend to agree with Eric on this one, but if Dave or Christoph want to weigh
in that's cool.

Thanks,
	Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: return ENOSPC when trying to set more ACLs than XFS_ACL_MAX_ENTRIES

[Jeff Liu]

Hi Ben,

On 12/12 2013 03:36 AM, Ben Myers wrote:
> Hey Jeff,
> 
> On Mon, Nov 25, 2013 at 12:57:15PM +0800, Jeff Liu wrote:
>>
>> On 11/25 2013 11:34 AM, Eric Sandeen wrote:
>>> On 11/24/13, 9:36 AM, Jeff Liu wrote:
>>>> From: Jie Liu <jeff.liu@oracle.com>
>>>>
>>>> We currently return EINVAL when trying to set more ACL entries than
>>>> XFS_ACL_MAX_ENTRIES(), but it would be a bit more meaningful to return
>>>> ENOSPC in this situation, because the later is used to indicate there
>>>> is no more space to store new ACLs IMHO.
>>>
>>> I'm not quite convinced that it's better; the user will get an
>>> error string of "no space left on device" which is misleading too,
>> I admit that both looks misleading...
>>> and I'd argue that it's no better than "invalid argument."
>>>
>>> To me, I think it's not worth changing, but others may disagree.
>>>
>>> (I guess looking at ext4, it uses ENOSPC for some similar constraints,
>>> so maybe three is precedent for this)
>> Btrfs also uses ENOSPC, but JFS would return something like "Argument list too long"
>> in this case.
> 
> I tend to agree with Eric on this one, but if Dave or Christoph want to weigh
> in that's cool.
I agree to Eric as well since either errno cannot indicate this situation much
cleaner.  I also can not find an existing errno could be used for this situation,
maybe someday there would have a particular errno to be used in this case if users
complain about it.

Thanks,
-Jeff

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: return ENOSPC when trying to set more ACLs than XFS_ACL_MAX_ENTRIES

[Christoph Hellwig]

On Wed, Dec 11, 2013 at 01:36:22PM -0600, Ben Myers wrote:
> > > (I guess looking at ext4, it uses ENOSPC for some similar constraints,
> > > so maybe three is precedent for this)
> > Btrfs also uses ENOSPC, but JFS would return something like "Argument list too long"
> > in this case.

I think ENOSPC is a really bad idea in this case, but I also think we
should make sure Linux filesystems behave unfiformly.

Jeff, can you write a summary of the errors returned by all common
filesystems and post it to -fsdevel for comment?  We should have a
common error for this, and I'd be a happier man if it weren't ENOSPC,
but that's secondary.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: return ENOSPC when trying to set more ACLs than XFS_ACL_MAX_ENTRIES

[Jeff Liu]

On 12/13 2013 21:04 PM, Christoph Hellwig wrote:
> On Wed, Dec 11, 2013 at 01:36:22PM -0600, Ben Myers wrote:
>>>> (I guess looking at ext4, it uses ENOSPC for some similar constraints,
>>>> so maybe three is precedent for this)
>>> Btrfs also uses ENOSPC, but JFS would return something like "Argument list too long"
>>> in this case.
> 
> I think ENOSPC is a really bad idea in this case, but I also think we
> should make sure Linux filesystems behave unfiformly.
> 
> Jeff, can you write a summary of the errors returned by all common
> filesystems and post it to -fsdevel for comment?  We should have a
> common error for this, and I'd be a happier man if it weren't ENOSPC,
> but that's secondary.
Yep, let me gather up those info...

Thanks,
-Jeff

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs