On Mon, Nov 8, 2010 at 11:13 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Mon, 8 Nov 2010 09:47:18 -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 | 84 ++++++++++++++++++++++++++++++----------------------
>> 1 files changed, 48 insertions(+), 36 deletions(-)
>>
>> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
>> index c9b4792..8c260b9 100644
>> --- a/fs/cifs/cifsacl.c
>> +++ b/fs/cifs/cifsacl.c
>> @@ -552,38 +552,40 @@ static int build_sec_desc(struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd,
>> return rc;
>> }
>>
>> -static struct cifs_ntsd *get_cifs_acl_by_fid(struct cifs_sb_info *cifs_sb,
>> - __u16 fid, u32 *pacllen)
>> +static int
>> +get_cifs_acl_by_fid(struct cifs_sb_info *cifs_sb,
>> + __u16 fid, u32 *pacllen, struct cifs_ntsd **pntsd)
> ^^^^^^^^^^^
> Dealing with pointers to pointers like this is cumbersome and
> inefficient. Why not just have it return a struct pointer like before
> or a ERR_PTR converted error?
I think we now have a consistent interface not only within
various get/set_cifs_acl* functions but like most of the rest of of
the functions
i.e. they return error code and not a ptr to a structure when successful
or an err ptr for failure?
>
>> {
>> - struct cifs_ntsd *pntsd = NULL;
>> - int xid, rc;
>> + int xid;
>> + int rc = 0;
>> struct tcon_link *tlink = cifs_sb_tlink(cifs_sb);
>>
>> if (IS_ERR(tlink))
>> - return NULL;
>> + PTR_ERR(tlink);
> ^^^^^^^^^^^^^^^
> bug?
>
>>
>> xid = GetXid();
>> - rc = CIFSSMBGetCIFSACL(xid, tlink_tcon(tlink), fid, &pntsd, pacllen);
>> + rc = CIFSSMBGetCIFSACL(xid, tlink_tcon(tlink), fid, pntsd, pacllen);
>> FreeXid(xid);
>>
>> cifs_put_tlink(tlink);
>>
>> cFYI(1, "GetCIFSACL rc = %d ACL len %d", rc, *pacllen);
>> - return pntsd;
>> + return rc;
>> }
>>
>> -static struct cifs_ntsd *get_cifs_acl_by_path(struct cifs_sb_info *cifs_sb,
>> - const char *path, u32 *pacllen)
>> +static int
>> +get_cifs_acl_by_path(struct cifs_sb_info *cifs_sb, const char *path,
>> + u32 *pacllen, struct cifs_ntsd **pntsd)
>> {
>> - struct cifs_ntsd *pntsd = NULL;
>> int oplock = 0;
>> - int xid, rc;
>> + int xid;
>> + int rc = 0;
>> __u16 fid;
>> struct cifsTconInfo *tcon;
>> struct tcon_link *tlink = cifs_sb_tlink(cifs_sb);
>>
>> if (IS_ERR(tlink))
>> - return NULL;
>> + return PTR_ERR(tlink);
>>
>> tcon = tlink_tcon(tlink);
>> xid = GetXid();
>> @@ -596,32 +598,34 @@ static struct cifs_ntsd *get_cifs_acl_by_path(struct cifs_sb_info *cifs_sb,
>> goto out;
>> }
>>
>> - rc = CIFSSMBGetCIFSACL(xid, tcon, fid, &pntsd, pacllen);
>> + 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;
>> + return rc;
>> }
>>
>> /* Retrieve an ACL from the server */
>> -static struct cifs_ntsd *get_cifs_acl(struct cifs_sb_info *cifs_sb,
>> - struct inode *inode, const char *path,
>> - u32 *pacllen)
>> +static int
>> +get_cifs_acl(struct cifs_sb_info *cifs_sb, struct inode *inode,
>> + const char *path, u32 *pacllen, struct cifs_ntsd **pntsd)
>> {
>> - struct cifs_ntsd *pntsd = NULL;
>> + int rc = 0;
>> struct cifsFileInfo *open_file = NULL;
>>
>> if (inode)
>> open_file = find_readable_file(CIFS_I(inode), true);
> ^^^^^^^^^^
> This predates your patch, but under what conditions would we
> call this function without an inode? If you're cleaning up the
> prototype for this function, then I think there are some
> unneeded arguments here.
>
> --
> 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
