Page MenuHomeFreeBSD

NFS remove vfs_statfs and vnode_mount macro
ClosedPublic

Authored by shivank on Jun 14 2020, 4:14 AM.

Details

Summary

delete vfs_statfs macro and inline it at the call sites(only 2).
This macro causes trouble with vfs_statfs field in vfsops structure. Also, the macro definition & purpose contradicts the VFS_STATFS(9)

These macro definitions are no longer needed as NFS OSX port is long dead and they cause conflict in other places.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

shivank created this revision.Jun 14 2020, 4:14 AM
shivank requested review of this revision.Jun 14 2020, 4:14 AM
asomers accepted this revision.Jun 14 2020, 2:57 PM

This change LGTM. But I'll give @rmacklem some time to weigh in before I merge it.

This revision is now accepted and ready to land.Jun 14 2020, 2:57 PM
rmacklem accepted this revision.Jun 15 2020, 1:01 AM

Looks fine to me as well. I had never noticed it ended up in multiple
.h files.
I would suggest getting rid of vnode_mount() as well.
And if you feel like doing a lot of editing, "vnode_t" is
really just "struct vnode *" and getting rid of it would
probably make the code more readable (and maintainable?).

I've clicked reviewed, in case you want to leave vnode_mount() for now.

sys/fs/nfs/nfs_commonsubs.c
1406 ↗(On Diff #73090)

You could take this a step further and replace
vnode_mount(vp) with vp->v_mount. For example, "vfs_statfs(vnode_mount(vp))->"
can be replaced with "vp->v_mount->mnt_stat.".

Both vfs_statfs() and vnode_mount() were Mac OS/X's way of doing things via
accessor functions and the macros in nfskpiport.h were meant to make the
Mac OS/X code work in FreeBSD.

Since the Mac OS/X port is ancient history, I have planned on eventually
getting rid of all of the macros in nfskpiport.h by doing the replacement
manually. Someday I'll do this for "vnode_t", but it is painful, because after
you do a global substitution, there are a lot of wrapped lines that need to be
fixed, etc. (I did do some of them recently, but haven't gotten around to the rest.

sys/fs/nfs/nfsdport.h
108 ↗(On Diff #73090)

As noted above, you can get rid of vnode_mount() as well, although
it probably shows up in a lot of places.

shivank updated this revision to Diff 73150.Jun 15 2020, 9:29 PM
shivank retitled this revision from NFS remove vfs_statfs macro to NFS remove vfs_statfs and vnode_mount macro.
shivank edited the summary of this revision. (Show Details)
This revision now requires review to proceed.Jun 15 2020, 9:29 PM
shivank marked 2 inline comments as done.Jun 15 2020, 9:35 PM

Please re-review it,
I updated the diff after removing the vnode_mount macro as suggested by Rick.

sys/fs/nfs/nfsdport.h
108 ↗(On Diff #73090)

Thanks for the suggestion. I removed vnode_mount(vp) macro and replaced it with the inline code

Except for the two inline comments, everything looks fine.

sys/fs/nfs/nfs_commonsubs.c
1405 ↗(On Diff #73150)

It might read better as

vp->v_mount->mnt_stat.f_fsid.val[0] ||

instead of taking the address of and then
using it as a pointer.
(Same below.)

sys/fs/nfsclient/nfs_clstate.c
3064 ↗(On Diff #73150)

Minor style nit.
BSD likes a 4 space indentation when a statement continues
on a subsequent line.

shivank updated this revision to Diff 73160.Jun 16 2020, 5:43 AM
shivank marked 3 inline comments as done.
rmacklem accepted this revision.Jun 16 2020, 2:54 PM
This revision is now accepted and ready to land.Jun 16 2020, 2:54 PM
asomers accepted this revision.Jun 17 2020, 4:16 AM
This revision was automatically updated to reflect the committed changes.