[nfs-discuss] Code review for 6888022 NFSv4: grpid mount flag is not effective for files

[Marcel Telka]

Hi Robert,

On Wed, Oct 21, 2009 at 11:27:11AM -0500, Robert Gordon wrote:
> 
> On Oct 21, 2009, at 10:55 AM, Marcel Telka wrote:
> 
> >Hm. There are many other MI flags with same values, for example
> >MI_HARD vs.
> >MI4_HARD, MI_PRINTED vs. MI4_PRINTED, etc.
> >
> >In addition, the nfsstat command silently assumes that those
> >values are equal.
> >Please read usr/src/cmd/fs.d/nfs/nfsstat/nfsstat.c around line 1220.
> >
> >If we wound want to add some kind of comment describing this
> >aspect it should
> >be more general, not only related to MI_GRPID vs. MI4_GRPID.
> 
> Oh yeah.. i was just using GRPID as an example..
> 
> >
> >I believe (and I hope :-) this is out of the scope of the proposed
> >fix.
> 
> Oh yes, for sure.. maybe it's a os-bite-size bug target :)

Ok. If you want to have it noted please report the bug yourself at
. I personally do not feel that we need such
comment so I'll not file a bug myself :-).


Have a nice day.

-- 
Marcel Telka
Solaris RPE

[nfs-discuss] Code review for 6888022 NFSv4: grpid mount flag is not effective for files

[Marcel Telka]

Hi Robert,

On Wed, Oct 21, 2009 at 10:41:50AM -0500, Robert Gordon wrote:
> It looks good!.

Thank you!

> 
> I did notice the following however:
> 
> usr/src/lib/libfsmgt/common/nfs_mntinfo.c and usr/src/cmd/fs.d/nfs/
> nfsstat/nfsstat.c don't use the MI4_* defines, which is okay since
> (for example) MI4_GRPID and MI_GRPID have the same value. At a
> minimum we should add a comment to nfs4_clnt.h that states the
> "dependency" .. (around line 1039..)

Hm. There are many other MI flags with same values, for example MI_HARD vs.
MI4_HARD, MI_PRINTED vs. MI4_PRINTED, etc.

In addition, the nfsstat command silently assumes that those values are equal.
Please read usr/src/cmd/fs.d/nfs/nfsstat/nfsstat.c around line 1220.

If we wound want to add some kind of comment describing this aspect it should
be more general, not only related to MI_GRPID vs. MI4_GRPID.

I believe (and I hope :-) this is out of the scope of the proposed fix.

Thanks.

-- 
Marcel Telka
Solaris RPE

[nfs-discuss] Code review for 6888022 NFSv4: grpid mount flag is not effective for files

[Marcel Telka]

Hi all,

Please code review the following fix:
http://cr.opensolaris.org/~aragorn/6888022-grpid/

Background
==

nfs4_create() function lacks the grpid mount flag handling in nfs4open_otw().
On the other side the nfs4_mkdir() function calls for directory creation
call_nfs4_create_req() function. The call_nfs4_create_req() handles properly
the grpid flag for directories.

The fix adds request to set the gid into the OPEN operation in case when grpid
mount flag (MI4_GRPID) is used. The idea of the fix is same as the code used in
call_nfs4_create_req() where the directory creation is handled.

Unit test
=

I unit tested the fix on both sparc and x86 machines with all possible 
combinations of
- NFS version (2, 3, 4)
- grpid mount flag used/not used
- setgid bit set/not set for a directory owning the newly created file/directory


Thank you.

-- 
Marcel Telka
Solaris RPE

[nfs-discuss] Code review for 6888022 NFSv4: grpid mount flag is not effective for files

[Robert Gordon]

On Oct 21, 2009, at 10:55 AM, Marcel Telka wrote:

> Hm. There are many other MI flags with same values, for example  
> MI_HARD vs.
> MI4_HARD, MI_PRINTED vs. MI4_PRINTED, etc.
>
> In addition, the nfsstat command silently assumes that those values  
> are equal.
> Please read usr/src/cmd/fs.d/nfs/nfsstat/nfsstat.c around line 1220.
>
> If we wound want to add some kind of comment describing this aspect  
> it should
> be more general, not only related to MI_GRPID vs. MI4_GRPID.

Oh yeah.. i was just using GRPID as an example..

>
> I believe (and I hope :-) this is out of the scope of the proposed  
> fix.

Oh yes, for sure.. maybe it's a os-bite-size bug target :)

Robert. 
  

[nfs-discuss] Code review for 6888022 NFSv4: grpid mount flag is not effective for files

[Robert Gordon]

It looks good!.

I did notice the following however:

usr/src/lib/libfsmgt/common/nfs_mntinfo.c and usr/src/cmd/fs.d/nfs/ 
nfsstat/nfsstat.c don't use the MI4_* defines, which is okay since  
(for example) MI4_GRPID and MI_GRPID have the same value. At a minimum  
we should add a comment to nfs4_clnt.h that states the "dependency" ..  
(around line 1039..)

Robert.


On Oct 21, 2009, at 10:06 AM, Marcel Telka wrote:

> Hi all,
>
> Please code review the following fix:
> http://cr.opensolaris.org/~aragorn/6888022-grpid/
>
> Background
> ==
>
> nfs4_create() function lacks the grpid mount flag handling in  
> nfs4open_otw().
> On the other side the nfs4_mkdir() function calls for directory  
> creation
> call_nfs4_create_req() function. The call_nfs4_create_req() handles  
> properly
> the grpid flag for directories.
>
> The fix adds request to set the gid into the OPEN operation in case  
> when grpid
> mount flag (MI4_GRPID) is used. The idea of the fix is same as the  
> code used in
> call_nfs4_create_req() where the directory creation is handled.
>
> Unit test
> =
>
> I unit tested the fix on both sparc and x86 machines with all  
> possible combinations of
> - NFS version (2, 3, 4)
> - grpid mount flag used/not used
> - setgid bit set/not set for a directory owning the newly created  
> file/directory
>
>
> Thank you.
>
> -- 
> Marcel Telka
> Solaris RPE
> ___
> nfs-discuss mailing list
> nfs-discuss at opensolaris.org