| Message ID | 1375714059-29567-2-git-send-email-Julia.Lawall@lip6.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2013-08-05 at 16:47 +0200, Julia Lawall wrote: > From: Julia Lawall <Julia.Lawall@lip6.fr> > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> > > --- > > This patch adjusts the code so that the alignment matches the current > semantics. I have no idea if it is the intended semantics, though. Should > the call to nfs_setsecurity also be under the else? > > fs/nfs/inode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c > index af6e806..d8ad685 100644 > --- a/fs/nfs/inode.c > +++ b/fs/nfs/inode.c > @@ -463,7 +463,7 @@ nfs_fhget(struct super_block *sb, struct nfs_fh > *fh, struct nfs_fattr *fattr, st > unlock_new_inode(inode); > } else > nfs_refresh_inode(inode, fattr); > - nfs_setsecurity(inode, fattr, label); > + nfs_setsecurity(inode, fattr, label); > dprintk("NFS: nfs_fhget(%s/%Ld fh_crc=0x%08x ct=%d)\n", > inode->i_sb->s_id, > (long long)NFS_FILEID(inode), Hi Julia, Thanks for pointing this out! Given that the 'then' clause of the if statement already calls nfs_setsecurity before unlocking the inode, I suspect that the above _should_ really be part of the 'else' clause. That said, I can't see that calling nfs_setsecurity twice on the inode can cause any unintended side-effects, so I suggest that we rather queue the patch up for inclusion in 3.12. Steve and Dave, any comments? -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com
Hello, On 05/08/13 10:59, Myklebust, Trond wrote: > On Mon, 2013-08-05 at 16:47 +0200, Julia Lawall wrote: >> From: Julia Lawall <Julia.Lawall@lip6.fr> >> >> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> >> >> --- >> >> This patch adjusts the code so that the alignment matches the current >> semantics. I have no idea if it is the intended semantics, though. Should >> the call to nfs_setsecurity also be under the else? >> > >> fs/nfs/inode.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c >> index af6e806..d8ad685 100644 >> --- a/fs/nfs/inode.c >> +++ b/fs/nfs/inode.c >> @@ -463,7 +463,7 @@ nfs_fhget(struct super_block *sb, struct nfs_fh >> *fh, struct nfs_fattr *fattr, st >> unlock_new_inode(inode); >> } else >> nfs_refresh_inode(inode, fattr); >> - nfs_setsecurity(inode, fattr, label); >> + nfs_setsecurity(inode, fattr, label); This call to nfs_setsecurity() is not needed. The security only needs to be set when the i-node is created... steved. >> dprintk("NFS: nfs_fhget(%s/%Ld fh_crc=0x%08x ct=%d)\n", >> inode->i_sb->s_id, >> (long long)NFS_FILEID(inode), > > Hi Julia, > > Thanks for pointing this out! Given that the 'then' clause of the if > statement already calls nfs_setsecurity before unlocking the inode, I > suspect that the above _should_ really be part of the 'else' clause. > > That said, I can't see that calling nfs_setsecurity twice on the inode > can cause any unintended side-effects, so I suggest that we rather queue > the patch up for inclusion in 3.12. > Steve and Dave, any comments? > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/6/2013 2:04 PM, Steve Dickson wrote: > Hello, > > On 05/08/13 10:59, Myklebust, Trond wrote: >> On Mon, 2013-08-05 at 16:47 +0200, Julia Lawall wrote: >>> From: Julia Lawall <Julia.Lawall@lip6.fr> >>> >>> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> >>> >>> --- >>> >>> This patch adjusts the code so that the alignment matches the current >>> semantics. I have no idea if it is the intended semantics, though. Should >>> the call to nfs_setsecurity also be under the else? >>> >> >>> fs/nfs/inode.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c >>> index af6e806..d8ad685 100644 >>> --- a/fs/nfs/inode.c >>> +++ b/fs/nfs/inode.c >>> @@ -463,7 +463,7 @@ nfs_fhget(struct super_block *sb, struct nfs_fh >>> *fh, struct nfs_fattr *fattr, st >>> unlock_new_inode(inode); >>> } else >>> nfs_refresh_inode(inode, fattr); >>> - nfs_setsecurity(inode, fattr, label); >>> + nfs_setsecurity(inode, fattr, label); > This call to nfs_setsecurity() is not needed. The security only needs > to be set when the i-node is created... > > steved. > >>> dprintk("NFS: nfs_fhget(%s/%Ld fh_crc=0x%08x ct=%d)\n", >>> inode->i_sb->s_id, >>> (long long)NFS_FILEID(inode), >> >> Hi Julia, >> >> Thanks for pointing this out! Given that the 'then' clause of the if >> statement already calls nfs_setsecurity before unlocking the inode, I >> suspect that the above _should_ really be part of the 'else' clause. >> >> That said, I can't see that calling nfs_setsecurity twice on the inode >> can cause any unintended side-effects, so I suggest that we rather queue >> the patch up for inclusion in 3.12. >> Steve and Dave, any comments? >> > I can't see why it would be needed either. I agree with Steve. We can get rid of it. Dave -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c index af6e806..d8ad685 100644 --- a/fs/nfs/inode.c +++ b/fs/nfs/inode.c @@ -463,7 +463,7 @@ nfs_fhget(struct super_block *sb, struct nfs_fh *fh, struct nfs_fattr *fattr, st unlock_new_inode(inode); } else nfs_refresh_inode(inode, fattr); - nfs_setsecurity(inode, fattr, label); + nfs_setsecurity(inode, fattr, label); dprintk("NFS: nfs_fhget(%s/%Ld fh_crc=0x%08x ct=%d)\n", inode->i_sb->s_id, (long long)NFS_FILEID(inode),