From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Steve French" Subject: Re: [PATCH 2/4] cifs: use common code for turning off ATTR_READONLY in cifs_unlink Date: Tue, 16 Sep 2008 18:37:25 -0500 Message-ID: <524f69650809161637s7f635a66v73c8c90e35f165b1@mail.gmail.com> References: <1221588319-7887-1-git-send-email-jlayton@redhat.com> <1221588319-7887-3-git-send-email-jlayton@redhat.com> <524f69650809161635o1f3830f3k4ea01858584cdd1a@mail.gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit To: linux-fsdevel Return-path: Received: from nf-out-0910.google.com ([64.233.182.188]:8048 "EHLO nf-out-0910.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751671AbYIPXh1 (ORCPT ); Tue, 16 Sep 2008 19:37:27 -0400 Received: by nf-out-0910.google.com with SMTP id d3so1613992nfc.21 for ; Tue, 16 Sep 2008 16:37:25 -0700 (PDT) In-Reply-To: <524f69650809161635o1f3830f3k4ea01858584cdd1a@mail.gmail.com> Content-Disposition: inline Sender: linux-fsdevel-owner@vger.kernel.org List-ID: I will commit the patch to the cifs git development tree but wanted to consider things: 1) your patch changes from open with FILE_WRITE_ATTRIBUTES to open with GENERIC_WRITE by calling cifs_set_file_info 2) we should probably do the Open then RenameOpenFile whether or not the cifs_set_file_info fails 3) to remove the extern for a static in the c file, the function cifs_set_file_info should be moved forward in the file On Tue, Sep 16, 2008 at 6:35 PM, Steve French wrote: > > I will commit the patch to the cifs git development tree but wanted to consider things: > > 1) your patch changes from open with FILE_WRITE_ATTRIBUTES to open with GENERIC_WRITE by calling cifs_set_file_info > 2) we should probably do the Open then RenameOpenFile whether or not the cifs_set_file_info fails > 3) to remove the extern for a static in the c file, the function cifs_set_file_info should be moved forward in the file > > On Tue, Sep 16, 2008 at 1:05 PM, Jeff Layton wrote: >> >> We already have a cifs_set_file_info function that can flip DOS >> attribute bits. Have cifs_unlink call it to handle turning ATTR_HIDDEN >> on and ATTR_READONLY off when an unlink attempt returns -EACCES. >> >> This also removes a level of indentation from cifs_unlink. >> >> Signed-off-by: Jeff Layton >> --- >> fs/cifs/inode.c | 118 +++++++++++++++++++++--------------------------------- >> 1 files changed, 46 insertions(+), 72 deletions(-) >> >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c >> index 2f75714..783f4ad 100644 >> --- a/fs/cifs/inode.c >> +++ b/fs/cifs/inode.c >> @@ -29,6 +29,8 @@ >> #include "cifs_debug.h" >> #include "cifs_fs_sb.h" >> >> +static int cifs_set_file_info(struct inode *inode, struct iattr *attrs, >> + int xid, char *full_path, __u32 dosattr); >> >> static void cifs_set_ops(struct inode *inode, const bool is_dfs_referral) >> { >> @@ -675,7 +677,8 @@ int cifs_unlink(struct inode *dir, struct dentry *dentry) >> struct super_block *sb = dir->i_sb; >> struct cifs_sb_info *cifs_sb = CIFS_SB(sb); >> struct cifsTconInfo *tcon = cifs_sb->tcon; >> - FILE_BASIC_INFO *pinfo_buf; >> + struct iattr *attrs; >> + __u32 dosattr; >> >> cFYI(1, ("cifs_unlink, dir=0x%p, dentry=0x%p", dir, dentry)); >> >> @@ -728,84 +731,55 @@ psx_del_no_retry: >> } >> } else if (rc == -EACCES) { >> /* try only if r/o attribute set in local lookup data? */ >> - pinfo_buf = kzalloc(sizeof(FILE_BASIC_INFO), GFP_KERNEL); >> - if (pinfo_buf) { >> - /* ATTRS set to normal clears r/o bit */ >> - pinfo_buf->Attributes = cpu_to_le32(ATTR_NORMAL); >> - if (!(tcon->ses->flags & CIFS_SES_NT4)) >> - rc = CIFSSMBSetPathInfo(xid, tcon, full_path, >> - pinfo_buf, >> - cifs_sb->local_nls, >> - cifs_sb->mnt_cifs_flags & >> - CIFS_MOUNT_MAP_SPECIAL_CHR); >> - else >> - rc = -EOPNOTSUPP; >> - >> - if (rc == -EOPNOTSUPP) { >> - int oplock = 0; >> - __u16 netfid; >> - /* rc = CIFSSMBSetAttrLegacy(xid, tcon, >> - full_path, >> - (__u16)ATTR_NORMAL, >> - cifs_sb->local_nls); >> - For some strange reason it seems that NT4 eats the >> - old setattr call without actually setting the >> - attributes so on to the third attempted workaround >> - */ >> - >> - /* BB could scan to see if we already have it open >> - and pass in pid of opener to function */ >> - rc = CIFSSMBOpen(xid, tcon, full_path, >> - FILE_OPEN, SYNCHRONIZE | >> - FILE_WRITE_ATTRIBUTES, 0, >> - &netfid, &oplock, NULL, >> - cifs_sb->local_nls, >> - cifs_sb->mnt_cifs_flags & >> - CIFS_MOUNT_MAP_SPECIAL_CHR); >> - if (rc == 0) { >> - rc = CIFSSMBSetFileInfo(xid, tcon, >> - pinfo_buf, >> - netfid, >> - current->tgid); >> - CIFSSMBClose(xid, tcon, netfid); >> - } >> - } >> - kfree(pinfo_buf); >> + attrs = kzalloc(sizeof(*attrs), GFP_KERNEL); >> + if (attrs == NULL) { >> + rc = -ENOMEM; >> + goto out_reval; >> } >> + >> + /* try to reset dos attributes */ >> + cifsInode = CIFS_I(inode); >> + dosattr = cifsInode->cifsAttrs & ~ATTR_READONLY; >> + if (dosattr == 0) >> + dosattr |= ATTR_NORMAL; >> + dosattr |= ATTR_HIDDEN; >> + >> + rc = cifs_set_file_info(inode, attrs, xid, full_path, dosattr); >> + kfree(attrs); >> + if (rc != 0) >> + goto out_reval; >> + rc = CIFSSMBDelFile(xid, tcon, full_path, cifs_sb->local_nls, >> + cifs_sb->mnt_cifs_flags & >> + CIFS_MOUNT_MAP_SPECIAL_CHR); >> if (rc == 0) { >> - rc = CIFSSMBDelFile(xid, tcon, full_path, >> - cifs_sb->local_nls, >> - cifs_sb->mnt_cifs_flags & >> - CIFS_MOUNT_MAP_SPECIAL_CHR); >> - if (!rc) { >> + if (inode) >> + drop_nlink(inode); >> + } else if (rc == -ETXTBSY) { >> + int oplock = 0; >> + __u16 netfid; >> + >> + rc = CIFSSMBOpen(xid, tcon, full_path, >> + FILE_OPEN, DELETE, >> + CREATE_NOT_DIR | >> + CREATE_DELETE_ON_CLOSE, >> + &netfid, &oplock, NULL, >> + cifs_sb->local_nls, >> + cifs_sb->mnt_cifs_flags & >> + CIFS_MOUNT_MAP_SPECIAL_CHR); >> + if (rc == 0) { >> + CIFSSMBRenameOpenFile(xid, tcon, >> + netfid, NULL, >> + cifs_sb->local_nls, >> + cifs_sb->mnt_cifs_flags & >> + CIFS_MOUNT_MAP_SPECIAL_CHR); >> + CIFSSMBClose(xid, tcon, netfid); >> if (inode) >> drop_nlink(inode); >> - } else if (rc == -ETXTBSY) { >> - int oplock = 0; >> - __u16 netfid; >> - >> - rc = CIFSSMBOpen(xid, tcon, full_path, >> - FILE_OPEN, DELETE, >> - CREATE_NOT_DIR | >> - CREATE_DELETE_ON_CLOSE, >> - &netfid, &oplock, NULL, >> - cifs_sb->local_nls, >> - cifs_sb->mnt_cifs_flags & >> - CIFS_MOUNT_MAP_SPECIAL_CHR); >> - if (rc == 0) { >> - CIFSSMBRenameOpenFile(xid, tcon, >> - netfid, NULL, >> - cifs_sb->local_nls, >> - cifs_sb->mnt_cifs_flags & >> - CIFS_MOUNT_MAP_SPECIAL_CHR); >> - CIFSSMBClose(xid, tcon, netfid); >> - if (inode) >> - drop_nlink(inode); >> - } >> - /* BB if rc = -ETXTBUSY goto the rename logic BB */ >> } >> + /* BB if rc = -ETXTBUSY goto the rename logic BB */ >> } >> } >> +out_reval: >> if (inode) { >> cifsInode = CIFS_I(inode); >> cifsInode->time = 0; /* will force revalidate to get info >> -- >> 1.5.5.1 >> > > > > -- > Thanks, > > Steve -- Thanks, Steve