On Tue, Nov 9, 2010 at 11:40 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Tue, 9 Nov 2010 08:35:02 -0600
> shirishpargaonkar@xxxxxxxxx wrote:
>
>> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
>>
>>
>> Modify get/set_cifs_acl* calls to reutrn error code and percolate the
>> error code up to the caller.
>>
>>
>> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
>> ---
>> fs/cifs/cifsacl.c | 46 ++++++++++++++++++++++++++++++----------------
>> 1 files changed, 30 insertions(+), 16 deletions(-)
>>
>> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
>> index c9b4792..003588a 100644
>> --- a/fs/cifs/cifsacl.c
>> +++ b/fs/cifs/cifsacl.c
>> @@ -568,8 +568,11 @@ static struct cifs_ntsd *get_cifs_acl_by_fid(struct cifs_sb_info *cifs_sb,
>>
>> cifs_put_tlink(tlink);
>>
>> - cFYI(1, "GetCIFSACL rc = %d ACL len %d", rc, *pacllen);
>> - return pntsd;
>> + cFYI(1, "%s: rc = %d ACL len %d", __func__, rc, *pacllen);
>> + if (rc)
>> + return ERR_PTR(rc);
>> + else
> ^^^^
> This else isn't needed
>
>> + return pntsd;
>> }
>>
>> static struct cifs_ntsd *get_cifs_acl_by_path(struct cifs_sb_info *cifs_sb,
>> @@ -591,19 +594,19 @@ static struct cifs_ntsd *get_cifs_acl_by_path(struct cifs_sb_info *cifs_sb,
>> rc = CIFSSMBOpen(xid, tcon, path, FILE_OPEN, READ_CONTROL, 0,
>> &fid, &oplock, NULL, cifs_sb->local_nls,
>> cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
>> - if (rc) {
>> - cERROR(1, "Unable to open file to get ACL");
>> - goto out;
>> + if (!rc) {
>> + rc = CIFSSMBGetCIFSACL(xid, tcon, fid, &pntsd, pacllen);
>> + CIFSSMBClose(xid, tcon, fid);
>> }
>>
>> - rc = CIFSSMBGetCIFSACL(xid, tcon, fid, &pntsd, pacllen);
>> - cFYI(1, "GetCIFSACL rc = %d ACL len %d", rc, *pacllen);
>> -
>> - CIFSSMBClose(xid, tcon, fid);
>> - out:
>> cifs_put_tlink(tlink);
>> FreeXid(xid);
>> - return pntsd;
>> +
>> + cFYI(1, "%s: rc = %d ACL len %d", __func__, rc, *pacllen);
>> + if (rc)
>> + return ERR_PTR(rc);
>> + else
> ^^^^
> Also not needed...
>
>> + return pntsd;
>> }
>>
>> /* Retrieve an ACL from the server */
>> @@ -711,12 +714,18 @@ cifs_acl_to_fattr(struct cifs_sb_info *cifs_sb, struct cifs_fattr *fattr,
>> pntsd = get_cifs_acl(cifs_sb, inode, path, &acllen);
>>
>> /* if we can retrieve the ACL, now parse Access Control Entries, ACEs */
>> - if (pntsd)
>> + if (!pntsd)
>> + cERROR(1, "%s: NULL sec desc", __func__);
> ^^^^^^^^
> Will this ever be a null pointer? I don't think it will.
> It's either going to be an ERR_PTR or a real cifs_ntsd.
> I think that check isn't needed.
get_cifs_acl_by_pid/path can return NULL when the return thus
if (IS_ERR(tlink))
return NULL;
>
>> + else if (IS_ERR(pntsd)) {
>> + rc = PTR_ERR(pntsd);
>> + cERROR(1, "%s: error %d getting sec desc", __func__, rc);
>> + } else {
>> rc = parse_sec_desc(pntsd, acllen, fattr);
>> - if (rc)
>> - cFYI(1, "parse sec desc failed rc = %d", rc);
>> + kfree(pntsd);
>> + if (rc)
>> + cERROR(1, "parse sec desc failed rc = %d", rc);
>> + }
>>
>> - kfree(pntsd);
>> return;
>> }
>>
> ^^^^^
> Hmm, now that you're fixing this error, why not have it bubble up to
> cifs_get_inode_info too? It seems like cifs_acl_to_fattr could now
> return an int, and the caller should handle those errors.
>
>> @@ -736,7 +745,12 @@ int mode_to_acl(struct inode *inode, const char *path, __u64 nmode)
>> /* Add three ACEs for owner, group, everyone getting rid of
>> other ACEs as chmod disables ACEs and set the security descriptor */
>>
>> - if (pntsd) {
>> + if (!pntsd)
>> + cERROR(1, "%s: NULL sec desc", __func__);
> ^^^^^^^^
> Again, this pointer will never be NULL. This can be removed.
>
>> + else if (IS_ERR(pntsd)) {
>> + rc = PTR_ERR(pntsd);
>> + cERROR(1, "%s: error %d getting sec desc", __func__, rc);
>> + } else {
>> /* allocate memory for the smb header,
>> set security descriptor request security descriptor
>> parameters, and secuirty descriptor itself */
>
>
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
