Re: [linux-cifs-client] [PATCH 04/13] cifs: clean up set_cifs_acl interfaces

[Jeff Layton <jlayton@redhat.com>]

On Wed, 13 May 2009 21:53:13 -0500
Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:

> On Wed, May 13, 2009 at 3:04 PM, Jeff Layton <jlayton@redhat.com> wrote:
> > From: Christoph Hellwig <hch@infradead.org>
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  fs/cifs/cifsacl.c |   78 ++++++++++++++++++++++++++++-------------------------
> >  1 files changed, 41 insertions(+), 37 deletions(-)
> >
> > diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
> > index 7f8e6c4..1403b5d 100644
> > --- a/fs/cifs/cifsacl.c
> > +++ b/fs/cifs/cifsacl.c
> > @@ -612,57 +612,61 @@ static struct cifs_ntsd *get_cifs_acl(struct cifs_sb_info *cifs_sb,
> >        return pntsd;
> >  }
> >
> > -/* Set an ACL on the server */
> > -static int set_cifs_acl(struct cifs_ntsd *pnntsd, __u32 acllen,
> > -                               struct inode *inode, const char *path)
> > +static int set_cifs_acl_by_fid(struct cifs_sb_info *cifs_sb, __u16 fid,
> > +               struct cifs_ntsd *pnntsd, u32 acllen)
> >  {
> > -       struct cifsFileInfo *open_file;
> > -       bool unlock_file = false;
> > -       int xid;
> > -       int rc = -EIO;
> > -       __u16 fid;
> > -       struct super_block *sb;
> > -       struct cifs_sb_info *cifs_sb;
> > +       int xid, rc;
> >
> > -       cFYI(DBG2, ("set ACL for %s from mode 0x%x", path, inode->i_mode));
> > +       xid = GetXid();
> > +       rc = CIFSSMBSetCIFSACL(xid, cifs_sb->tcon, fid, pnntsd, acllen);
> > +       FreeXid(xid);
> >
> > -       if (!inode)
> > -               return rc;
> > +       cFYI(DBG2, ("SetCIFSACL rc = %d", rc));
> > +       return rc;
> > +}
> >
> > -       sb = inode->i_sb;
> > -       if (sb == NULL)
> > -               return rc;
> > +static int set_cifs_acl_by_path(struct cifs_sb_info *cifs_sb, const char *path,
> > +               struct cifs_ntsd *pnntsd, u32 acllen)
> > +{
> > +       int oplock = 0;
> > +       int xid, rc;
> > +       __u16 fid;
> >
> > -       cifs_sb = CIFS_SB(sb);
> >        xid = GetXid();
> >
> > -       open_file = find_readable_file(CIFS_I(inode));
> > -       if (open_file) {
> > -               unlock_file = true;
> > -               fid = open_file->netfid;
> > -       } else {
> > -               int oplock = 0;
> > -               /* open file */
> > -               rc = CIFSSMBOpen(xid, cifs_sb->tcon, path, FILE_OPEN,
> > -                               WRITE_DAC, 0, &fid, &oplock, NULL,
> > -                               cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
> > -                                       CIFS_MOUNT_MAP_SPECIAL_CHR);
> > -               if (rc != 0) {
> > -                       cERROR(1, ("Unable to open file to set ACL"));
> > -                       FreeXid(xid);
> > -                       return rc;
> > -               }
> > +       rc = CIFSSMBOpen(xid, cifs_sb->tcon, path, FILE_OPEN, WRITE_DAC, 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 set ACL"));
> > +               goto out;
> >        }
> >
> >        rc = CIFSSMBSetCIFSACL(xid, cifs_sb->tcon, fid, pnntsd, acllen);
> >        cFYI(DBG2, ("SetCIFSACL rc = %d", rc));
> > -       if (unlock_file)
> > -               atomic_dec(&open_file->wrtPending);
> > -       else
> > -               CIFSSMBClose(xid, cifs_sb->tcon, fid);
> >
> > +       CIFSSMBClose(xid, cifs_sb->tcon, fid);
> > + out:
> >        FreeXid(xid);
> > +       return rc;
> > +}
> >
> > +/* Set an ACL on the server */
> > +static int set_cifs_acl(struct cifs_ntsd *pnntsd, __u32 acllen,
> > +                               struct inode *inode, const char *path)
> > +{
> > +       struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
> > +       struct cifsFileInfo *open_file;
> > +       int rc;
> > +
> > +       cFYI(DBG2, ("set ACL for %s from mode 0x%x", path, inode->i_mode));
> > +
> > +       open_file = find_readable_file(CIFS_I(inode));
> 
> We do not know how the file was opened or whether one can set the
> attributes just
> because we have a file handle.
> So there is a possibility that set_cifs_acl_by_fid can fail but
> set_cifs_acl_by_path
> will succeed by virtue of opening file with attribute FILE_OPEN, WRITE_DAC?
> 

You're correct that that does seem wrong, but I don't think this patch
makes that any worse than it already is. The current set_cifs_acl code
does a find_readable_file, and uses that fid to try and set the ACL.

On a side note, this whole find_readable_file/find_writeable_file
interface is a real mess. What we really need I think is a new
find_open_file function that takes a set of open flags as an arg. When
it finds an open fh with flags that match the ones you've requested, it
can return that with the refcount bumped.

In fact, it would be even better if all of the "fallback to doing a new
open" stuff was hidden in an interface too so that callers didn't have
to worry about it.

> > +       if (!open_file)
> > +               return set_cifs_acl_by_path(cifs_sb, path, pnntsd, acllen);
> > +
> > +       rc = set_cifs_acl_by_fid(cifs_sb, open_file->netfid, pnntsd, acllen);
> > +       atomic_dec(&open_file->wrtPending);
> >        return rc;
> >  }
> >
> > --
> > 1.6.2.2
> >


-- 
Jeff Layton <jlayton@redhat.com>