[PATCHv2 02/12]posix_acl: Add the check items

[PATCHv2 02/12]posix_acl: Add the check items

[Liuwenyi]

move the ACL validation check in to fs/posix_acl.c.
Including nullpointer check and PTR_ERR check.


---
Signed-off-by: Liuwenyi <qingshenlwy@gmail.com>
Cc: Chris Mason <chris.mason@oracle.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Joel Becker <joel.becker@oracle.com>
Cc: Chris Ball <cjb@laptop.org>
Cc: James Morris <jmorris@namei.org>
Cc: linux-btrfs@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
index 2e9e699..0f14f95 100644
--- a/fs/btrfs/acl.c
+++ b/fs/btrfs/acl.c
@@ -102,12 +102,9 @@ static int btrfs_set_acl(struct btrfs_trans_handle
*trans,
char *value = NULL;
mode_t mode;

- if (acl) {
- ret = posix_acl_valid(acl);
- if (ret < 0)
- return ret;
- ret = 0;
- }
+ ret = posix_acl_valid(acl);
+ if (ret < 0)
+ return ret;

switch (type) {
case ACL_TYPE_ACCESS:

-- 
Best Regards,
Liuwenyi

Re: [PATCHv2 02/12]posix_acl: Add the check items

[Al Viro]

On Mon, Dec 21, 2009 at 07:52:58PM +0800, Liuwenyi wrote:
> @@ -102,12 +102,9 @@ static int btrfs_set_acl(struct btrfs_trans_handle
> *trans,
> char *value = NULL;
> mode_t mode;
> 
> - if (acl) {
> - ret = posix_acl_valid(acl);
> - if (ret < 0)
> - return ret;
> - ret = 0;
> - }
> + ret = posix_acl_valid(acl);
> + if (ret < 0)
> + return ret;

... and now you can't remove acl from btrfs inode.  At all.

NAK the entire series.  You are changing existing user-visible behaviour
for all filesystems for no reason and at least in this case it's clearly
wrong change.

Re: [PATCHv2 02/12]posix_acl: Add the check items

[liu weni]

Hi All:

    I'm sorry for my stupid. I am crazy for fixing this bug.
And make so much mistake.
Maybe I need cool down and think more.

--
Best Regards,
Liuwenyi
2009-12-22



2009/12/22 Al Viro <viro@zeniv.linux.org.uk>:
> On Mon, Dec 21, 2009 at 07:52:58PM +0800, Liuwenyi wrote:
>> @@ -102,12 +102,9 @@ static int btrfs_set_acl(struct btrfs_trans_handle
>> *trans,
>> char *value = NULL;
>> mode_t mode;
>>
>> - if (acl) {
>> - ret = posix_acl_valid(acl);
>> - if (ret < 0)
>> - return ret;
>> - ret = 0;
>> - }
>> + ret = posix_acl_valid(acl);
>> + if (ret < 0)
>> + return ret;
>
> ... and now you can't remove acl from btrfs inode.  At all.
>
> NAK the entire series.  You are changing existing user-visible behaviour
> for all filesystems for no reason and at least in this case it's clearly
> wrong change.
>

Re: [PATCHv2 02/12]posix_acl: Add the check items

[Al Viro]

On Tue, Dec 22, 2009 at 10:31:38PM +0800, liu weni wrote:
> Hi All:
> 
>     I'm sorry for my stupid. I am crazy for fixing this bug.
> And make so much mistake.

Welcome to the club.  It's OK, _everyone_ around here had done that.
Many times.

> Maybe I need cool down and think more.
 
Always a good advice, but in this case it's a bit more specific - if you
see that you are modifying behaviour for some arguments, figure out what
would lead to such arguments and see if that behaviour is intentional ;-)
And if after that you are still not sure whether it's a bug or not, ask the
people involved what the hell had they intended to happen in such-and-such
case (often enough that'll bring variants of "nice catch" - bugs *do* happen).