Re: [pnfs] [PATCH 2/5] nfsd: Fix independence of a few nfsd related headers

[Trond Myklebust <Trond.Myklebust@netapp.com>]

On Thu, 2009-11-12 at 14:48 +0200, Boaz Harrosh wrote:
> On 11/12/2009 12:35 PM, Trond Myklebust wrote:
> > On Thu, 2009-11-12 at 12:28 +0200, Boaz Harrosh wrote:
> >> On 10/21/2009 10:14 AM, Boaz Harrosh wrote:
> >>> An header should be compilation independent, .i.e pull in
> >>> any header who's declarations are directly used by this header.
> >>> And not let users re-include all it's dependencies all over
> >>> again.
> >>>
> >>> [At the end of the day what's the use of a header if it does
> >>>  not have more then one user?]
> >>>
> >>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> >>
> >> Trond do I have an ACK on this patch.
> >> If not, then what should be changed to get it accepted?
> >>
> >>> ---
> >>>  include/linux/nfs_xdr.h   |    1 +
> >>
> >> This header is used exclusively by fs/nfs/... files and could just be moved
> >> there. The include must be fixed as below though.
> >>
> >>>  include/linux/nfsacl.h    |    1 +
> >>
> >> This file is used mixed between fs/nfs && fs/nfsd
> >>
> >>>  include/linux/posix_acl.h |    1 +
> >>
> >> Used by nfsd and filesystems
> >>
> >>>  3 files changed, 3 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> >>> index 2848a26..c316ca8 100644
> >>> --- a/include/linux/nfs_xdr.h
> >>> +++ b/include/linux/nfs_xdr.h
> >>> @@ -2,6 +2,7 @@
> >>>  #define _LINUX_NFS_XDR_H
> >>>  
> >>>  #include <linux/nfsacl.h>
> >>> +#include <linux/nfs3.h>
> >>>  
> >>>  /*
> >>>   * To change the maximum rsize and wsize supported by the NFS client, adjust
> >>> diff --git a/include/linux/nfsacl.h b/include/linux/nfsacl.h
> >>> index 43011b6..f321b57 100644
> >>> --- a/include/linux/nfsacl.h
> >>> +++ b/include/linux/nfsacl.h
> >>> @@ -29,6 +29,7 @@
> >>>  #ifdef __KERNEL__
> >>>  
> >>>  #include <linux/posix_acl.h>
> >>> +#include <linux/sunrpc/xdr.h>
> >>>  
> >>>  /* Maximum number of ACL entries over NFS */
> >>>  #define NFS_ACL_MAX_ENTRIES	1024
> >>> diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h
> >>> index 065a365..0dcf674 100644
> >>> --- a/include/linux/posix_acl.h
> >>> +++ b/include/linux/posix_acl.h
> >>> @@ -9,6 +9,7 @@
> >>>  #define __LINUX_POSIX_ACL_H
> >>>  
> >>>  #include <linux/slab.h>
> >>> +#include <linux/fs.h>
> > 
> > NACK to this. Pretty much _all_ filesystems already include linux/fs.h
> > somewhere in their include chains. There should be no need to include it
> > in posix_acl.h too.
> > 
> 
> from posix_acl.h:
> extern int posix_acl_permission(struct inode *, const struct posix_acl *, int);
> 
> stuct inode is defined in fs.h. hence the direct dependency.

Add a forward declaration of the form

struct inode;

posix_acl.h, and you're done. No need to read the entire contents of
fs.h plus all the crap it drags in with it, in order to satisfy a single
pointer dependence.

> Again a double inclusion is not a bad thing, it costs absolutely *nothing*.
>
> A miss-inclusion on the other hand is a bad thing. it causes a miss-compilation.

Really?

> It does not matter that filesystems include fs.h or not. What matters is that they
> now have to do this headers ordering magic. One places code compiles fine, another
> place it does not. When it does not, people *never* analyze the missing dependency
> what they do is copy-paste an include list from another file that works.
> 
> Proof of the matter the last patches in this patchset.
> 
> >>>  
> >>>  #define ACL_UNDEFINED_ID	(-1)
> >>>  
> >>
> > 
> > So, what is the motivation for all this? We have no dependency problems
> > here today. What is changing in the pNFS tree that makes this so
> > necessary?
> > 
> 
> We do have a dependency problem today!! look at the patchset. It is a grate
> cleanup and improvement of code today. And a much smoother ride for the future.

> What changed is that all this code was touched today. I have not done the cleanup for
> any files not touched by pnfsd. Though I could and should, because they will greatly
> improve just like these I did touch. Should I go head and do the reset of the files?
> 
> And please note that this particular file is an NFSD and vfs related file only, it
> has nothing to do with nfs.

...and hence you need to send it to the VFS maintainer. I'm not touching
that patch, or the patchset that "requires" it.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com