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@xxxxxxxxxxx> >> >> 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. 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. 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. > Trond > Boaz -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
