On Tue, Nov 9, 2010 at 4:25 PM, <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.
>
> Even though, function cifs_acl_to_fattr() now returns a return code,
> it is not yet handled by the caller of cifs_acl_to_fattr().
> I would suggest to make that change, a part of another patch.
>
>
> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> ---
> fs/cifs/cifsacl.c | 44 ++++++++++++++++++++++++++------------------
> fs/cifs/cifsproto.h | 2 +-
> 2 files changed, 27 insertions(+), 19 deletions(-)
>
> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
> index c9b4792..cf53f7d 100644
> --- a/fs/cifs/cifsacl.c
> +++ b/fs/cifs/cifsacl.c
> @@ -560,7 +560,7 @@ static struct cifs_ntsd *get_cifs_acl_by_fid(struct cifs_sb_info *cifs_sb,
> struct tcon_link *tlink = cifs_sb_tlink(cifs_sb);
>
> if (IS_ERR(tlink))
> - return NULL;
> + ERR_CAST(tlink);
>
> xid = GetXid();
> rc = CIFSSMBGetCIFSACL(xid, tlink_tcon(tlink), fid, &pntsd, pacllen);
> @@ -568,7 +568,9 @@ 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);
> + cFYI(1, "%s: rc = %d ACL len %d", __func__, rc, *pacllen);
> + if (rc)
> + return ERR_PTR(rc);
> return pntsd;
> }
>
> @@ -583,7 +585,7 @@ static struct cifs_ntsd *get_cifs_acl_by_path(struct cifs_sb_info *cifs_sb,
> struct tcon_link *tlink = cifs_sb_tlink(cifs_sb);
>
> if (IS_ERR(tlink))
> - return NULL;
> + ERR_CAST(tlink);
>
> tcon = tlink_tcon(tlink);
> xid = GetXid();
> @@ -591,18 +593,17 @@ 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);
> +
> + cFYI(1, "%s: rc = %d ACL len %d", __func__, rc, *pacllen);
> + if (rc)
> + return ERR_PTR(rc);
> return pntsd;
> }
>
> @@ -695,7 +696,7 @@ static int set_cifs_acl(struct cifs_ntsd *pnntsd, __u32 acllen,
> }
>
> /* Translate the CIFS ACL (simlar to NTFS ACL) for a file into mode bits */
> -void
> +int
> cifs_acl_to_fattr(struct cifs_sb_info *cifs_sb, struct cifs_fattr *fattr,
> struct inode *inode, const char *path, const __u16 *pfid)
> {
> @@ -711,13 +712,17 @@ 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 (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;
> + return rc;
> }
>
> /* Convert mode bits to an ACL so we can update the ACL on the server */
> @@ -736,7 +741,10 @@ 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 (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 */
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index 7ed69b6..6ed59af 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -130,7 +130,7 @@ extern int cifs_get_file_info_unix(struct file *filp);
> extern int cifs_get_inode_info_unix(struct inode **pinode,
> const unsigned char *search_path,
> struct super_block *sb, int xid);
> -extern void cifs_acl_to_fattr(struct cifs_sb_info *cifs_sb,
> +extern int cifs_acl_to_fattr(struct cifs_sb_info *cifs_sb,
> struct cifs_fattr *fattr, struct inode *inode,
> const char *path, const __u16 *pfid);
> extern int mode_to_acl(struct inode *inode, const char *path, __u64);
> --
> 1.6.0.2
>
>
There is no change of behaviour with this patch the way connectathon
tests run. mode changes on the server are reflected correctly on the
client as well.
There is a bug in cifs module which manifests in inability to unload itself
from memory when general tests (-g -f) of connectathon are run, either
against a Samba server or a Windows server, with or without mount
option cifsacl (-o cifsacl) and with or without this patch.
# rmmod cifs
ERROR: Module cifs is in use
But at least error is percolated up to the caller now (cifs_acl_to_fattr())
and handling that error should be handled in another patch, IMHO.
So these two issues need to be addressed but this patch does not
introduce any adverse behaviour and is ready to merge if there are
no further comments/suggestions for change.
--
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
