Nor me nor my box is going to act as NFS server, so ifdef all
exporting code.
Signed-off-by: Alexey Dobriyan <[email protected]>
---
fs/ext2/namei.c | 2 ++
fs/ext2/super.c | 4 ++++
2 files changed, 6 insertions(+)
--- a/fs/ext2/namei.c
+++ b/fs/ext2/namei.c
@@ -70,6 +70,7 @@ static struct dentry *ext2_lookup(struct
return d_splice_alias(inode, dentry);
}
+#if defined(CONFIG_EXPORTFS) || defined(CONFIG_EXPORTFS_MODULE)
struct dentry *ext2_get_parent(struct dentry *child)
{
unsigned long ino;
@@ -94,6 +95,7 @@ struct dentry *ext2_get_parent(struct de
}
return parent;
}
+#endif
/*
* By the time this is called, we already have created
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -250,6 +250,7 @@ #ifdef CONFIG_QUOTA
#endif
};
+#if defined(CONFIG_EXPORTFS) || defined(CONFIG_EXPORTFS_MODULE)
static struct dentry *ext2_get_dentry(struct super_block *sb, void *vobjp)
{
__u32 *objp = vobjp;
@@ -297,6 +298,7 @@ static struct export_operations ext2_exp
.get_parent = ext2_get_parent,
.get_dentry = ext2_get_dentry,
};
+#endif
static unsigned long get_sb_block(void **data)
{
@@ -916,7 +918,9 @@ static int ext2_fill_super(struct super_
* set up enough so that it can read an inode
*/
sb->s_op = &ext2_sops;
+#if defined(CONFIG_EXPORTFS) || defined(CONFIG_EXPORTFS_MODULE)
sb->s_export_op = &ext2_export_ops;
+#endif
sb->s_xattr = ext2_xattr_handlers;
root = iget(sb, EXT2_ROOT_INO);
sb->s_root = d_alloc_root(root);
On Sat, 6 Jan 2007 22:58:31 +0300
Alexey Dobriyan <[email protected]> wrote:
> --- a/fs/ext2/namei.c
> +++ b/fs/ext2/namei.c
> @@ -70,6 +70,7 @@ static struct dentry *ext2_lookup(struct
> return d_splice_alias(inode, dentry);
> }
>
> +#if defined(CONFIG_EXPORTFS) || defined(CONFIG_EXPORTFS_MODULE)
> struct dentry *ext2_get_parent(struct dentry *child)
hm. Officially, one module isn't supposed to know about the presence of
another one at compile-time. Someone might want to come along and later
configure and compile the nfsd module and then try to load it into a kernel
which wasn't compiled with nfsd enabled.
But given that both modules are in mainline I suspect that nobody would
really be doing that in practice, and in the case of nfsd it might not even
work.
On Sat, Jan 06, 2007 at 12:16:11PM -0800, Andrew Morton wrote:
> On Sat, 6 Jan 2007 22:58:31 +0300
> Alexey Dobriyan <[email protected]> wrote:
> > --- a/fs/ext2/namei.c
> > +++ b/fs/ext2/namei.c
> > @@ -70,6 +70,7 @@ static struct dentry *ext2_lookup(struct
> > return d_splice_alias(inode, dentry);
> > }
> >
> > +#if defined(CONFIG_EXPORTFS) || defined(CONFIG_EXPORTFS_MODULE)
> > struct dentry *ext2_get_parent(struct dentry *child)
>
> hm. Officially, one module isn't supposed to know about the presence of
> another one at compile-time.
Whole new requirement.
> Someone might want to come along and later
> configure and compile the nfsd module and then try to load it into a kernel
> which wasn't compiled with nfsd enabled.
methinks, this is not a case.
agrep _MODULE -n | grep defined -w
shows plenty of cases, where code is conditionally compiled out with
CONFIG_IPV6 being the major "offender".
> But given that both modules are in mainline I suspect that nobody would
> really be doing that in practice, and in the case of nfsd it might not even
> work.
Yes, compat_sys_nfsservctl() is conditionally compiled in.
I interpret CONFIG_FOO=n as
"I am not going to use FOO, not now, not in future".
On Sat, Jan 06, 2007 at 10:58:31PM +0300, Alexey Dobriyan wrote:
> Nor me nor my box is going to act as NFS server, so ifdef all
> exporting code.
> @@ -916,7 +918,9 @@ static int ext2_fill_super(struct super_
> * set up enough so that it can read an inode
> */
> sb->s_op = &ext2_sops;
> +#if defined(CONFIG_EXPORTFS) || defined(CONFIG_EXPORTFS_MODULE)
> sb->s_export_op = &ext2_export_ops;
> +#endif
To avoid putting ifdefs within a function, how about adding:
#if defined(CONFIG_EXPORTFS) || defined(CONFIG_EXPORTFS_MODULE)
#define set_export_ops(sb, ops) sb->s_export_op = ops
#else
#define set_export_ops(sb, ops) 0
#endif
That way you can get rid of the function pointer from the struct
superblock too.
But Dave Woodhouse is going to kill you for adding another
CONFIG_*_MODULE dependency.
On Sat, 6 Jan 2007 22:58:31 +0300
Alexey Dobriyan <[email protected]> wrote:
> Nor me nor my box is going to act as NFS server, so ifdef all
> exporting code.
>
> Signed-off-by: Alexey Dobriyan <[email protected]>
> ---
>
> fs/ext2/namei.c | 2 ++
> fs/ext2/super.c | 4 ++++
> 2 files changed, 6 insertions(+)
>
> --- a/fs/ext2/namei.c
> +++ b/fs/ext2/namei.c
> @@ -70,6 +70,7 @@ static struct dentry *ext2_lookup(struct
> return d_splice_alias(inode, dentry);
> }
>
> +#if defined(CONFIG_EXPORTFS) || defined(CONFIG_EXPORTFS_MODULE)
> struct dentry *ext2_get_parent(struct dentry *child)
> {
> unsigned long ino;
> @@ -94,6 +95,7 @@ struct dentry *ext2_get_parent(struct de
> }
> return parent;
> }
> +#endif
>
> /*
> * By the time this is called, we already have created
> --- a/fs/ext2/super.c
> +++ b/fs/ext2/super.c
> @@ -250,6 +250,7 @@ #ifdef CONFIG_QUOTA
> #endif
> };
>
> +#if defined(CONFIG_EXPORTFS) || defined(CONFIG_EXPORTFS_MODULE)
> static struct dentry *ext2_get_dentry(struct super_block *sb, void *vobjp)
> {
> __u32 *objp = vobjp;
> @@ -297,6 +298,7 @@ static struct export_operations ext2_exp
> .get_parent = ext2_get_parent,
> .get_dentry = ext2_get_dentry,
> };
> +#endif
>
> static unsigned long get_sb_block(void **data)
> {
> @@ -916,7 +918,9 @@ static int ext2_fill_super(struct super_
> * set up enough so that it can read an inode
> */
> sb->s_op = &ext2_sops;
> +#if defined(CONFIG_EXPORTFS) || defined(CONFIG_EXPORTFS_MODULE)
> sb->s_export_op = &ext2_export_ops;
> +#endif
> sb->s_xattr = ext2_xattr_handlers;
> root = iget(sb, EXT2_ROOT_INO);
> sb->s_root = d_alloc_root(root);
>
The linker will remove unreferenced static functions from vmlinux. So a
better approach would be to move ext2_get_parent into super.c, make it
static and do Matthew's trick, yielding zero ifdefs.
That has the additional advantage that the nfsd-supporting code will always
be compiled.
On Sat, Jan 06, 2007 at 02:25:40PM -0700, Matthew Wilcox wrote:
> On Sat, Jan 06, 2007 at 10:58:31PM +0300, Alexey Dobriyan wrote:
> > Nor me nor my box is going to act as NFS server, so ifdef all
> > exporting code.
>
> > @@ -916,7 +918,9 @@ static int ext2_fill_super(struct super_
> > * set up enough so that it can read an inode
> > */
> > sb->s_op = &ext2_sops;
> > +#if defined(CONFIG_EXPORTFS) || defined(CONFIG_EXPORTFS_MODULE)
> > sb->s_export_op = &ext2_export_ops;
> > +#endif
>
> To avoid putting ifdefs within a function, how about adding:
>
> #if defined(CONFIG_EXPORTFS) || defined(CONFIG_EXPORTFS_MODULE)
> #define set_export_ops(sb, ops) sb->s_export_op = ops
> #else
> #define set_export_ops(sb, ops) 0
> #endif
>
> That way you can get rid of the function pointer from the struct
> superblock too.
Exactly! I've just started with filesystems I use.
it should be wrapped in do {} while 0, of course.
> But Dave Woodhouse is going to kill you for adding another
> CONFIG_*_MODULE dependency.
I missed a flamewar on this topic?
On Sun, Jan 07, 2007 at 12:44:56AM +0300, Alexey Dobriyan wrote:
> > #if defined(CONFIG_EXPORTFS) || defined(CONFIG_EXPORTFS_MODULE)
> > #define set_export_ops(sb, ops) sb->s_export_op = ops
> > #else
> > #define set_export_ops(sb, ops) 0
> > #endif
> >
> > That way you can get rid of the function pointer from the struct
> > superblock too.
>
> Exactly! I've just started with filesystems I use.
>
> it should be wrapped in do {} while 0, of course.
What the hell for? Repeat after me:
* do {} while(0) is always inferior to ((void)0)
* do { expr; } while(0) is always inferior to ((void)(expr))