Bug 658734

I had a look through the code and found that when there isn't an entry we fail silently instead of jumping to the "fail" label. The following patch changes the behaviour to the anticipated behaviour:
------------------------------------------------------------
--- a/setfacl/do_set.c
+++ b/setfacl/do_set.c
@@ -339,7 +340,7 @@ do_set(
 				if (ent)
 					acl_delete_entry(*xacl, ent);
 				else
-					/* ignore */;
+					goto fail;
 				break;
 
 			case CMD_REMOVE_EXTENDED_ACL:
------------------------------------------------------------
However, I'm uncertain if this change will impact the expected behaviour elsewhere.

Regards,
Harshula

I should have mentioned that an earlier getxattr(), via RETRIEVE_ACL(), sets errno to EOPNOTSUPP when setfacl -x is run on a f/s with "noacl".

RETRIEVE_ACL()
- retrieve_acl()
  - acl_get_file()
    - getxattr()

The getfacl and setfacl utilities can be transparently used on file systems with and without POSIX ACL support.  When a file has no permissions set other than the file permission bits, this is equivalent to a three-entry acl.  For example, a file mode of rw-r----- is equivalent to this acl:

   user::rw-
   group::r--
   other::---

When asked to modify the acl of a file, the setfacl utility retrieves the acl, modifies it according to the commands given, and saves the result.

The -x operation does not fail when asked to remove a non-existent entry; this is by design.  (I have clarified this in the man page now.)

The consequence is that on a file system without POSIX ACL support, "setfacl -x u:test" is a no-op, and "setfacl -m u:test:rwx" will try to set a real POSIX ACL which the file system cannot store, which must obviously fail.

Andreas, thank you for the clarification and the man page fix.

upstream commit:

http://git.savannah.gnu.org/cgit/acl.git/commit/?id=5a5670a

Thanks Andreas for adding your comment.

Removing a non-existent ACL entry does, however, fail in some cases. It quite rightly reports an error for:

# setfacl -x u:test1 f
setfacl: Option -x: Invalid argument near character 3

The user "test1" does not exist. Similarly, I think the correct behaviour when the file system does not support non file permission bit ACLs is to report EOPNOTSUPP to the user. As soon as getxattr() completes (via retrieve_acl()) and sets errno to EOPNOTSUPP, we know both commands (setfacl -x u:test f and setfacl -m u:test:rw f) can not succeed. And that happens before we check if the ACL entry exists.

That's why I believe, if the f/s does not support ACLs:

1) getfacl should report the file permission bits
2) setfacl should succeed when operating ONLY on file permission bits
3) setfacl should report EOPNOTSUPP when NOT operating on file permission bits.

The patch in Comment 2 is definitely incorrect. To get the aforementioned behaviour would require a larger change.

Also, Andreas noted that it is not required that removing a non-existing ACL entry be a no-op by the draft standard.

Regards,
Harshula

The behavior you would like to see is not obviously better than the status quo, it is just different.  I am open to continuing this discussion, but not without any additional, sound arguments on your side.  Thanks.

Harshula, how are you confirming that the current behavior is wrong?  If you just need to check whether the file system supports ACLs, there is a better way to do so...

Andreas, perhaps the "setfacl --remove" behaviour you have a personal preference for should be executed when the user wants to force the command by giving -f|--force ? e.g. see how rm behaves:
----------------------------------------------------
$ /bin/rm filedoesnotexist ; echo $?
/bin/rm: cannot remove `filedoesnotexist': No such file or directory
1
$ /bin/rm -f filedoesnotexist ; echo $?
0
----------------------------------------------------
User's already have expectations based on the precedent set by commonly used commands. We should strive for consistency.

I've conveyed my reasons for why at a minimum EOPNOTSUPP should be propagated to the user. I'll let this stew with you, and leave it up to you guys to decide. At least we have this BZ when the next customer complains.

BTW, there's a small typo in the updated man page: s/enries/entries/ .

Regards,
Harshula

> BTW, there's a small typo in the updated man page: s/enries/entries/ .

Thanks, the typo was even there before the latest change; it is fixed now:

  http://git.savannah.gnu.org/cgit/acl.git/commit/?id=986642a

Created attachment 464977 [details]
merged patch that applies on top of acl-2.2.49-4.el6

An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHBA-2011-0780.html