[PATCH 0/3] cifs: neatening and embedded module size reduction

[PATCH 0/3] cifs: neatening and embedded module size reduction

[Joe Perches]

Joe Perches (3):
  cifs: Remove unused cEVENT macro
  cifs: Make CIFS_DEBUG possible to undefine
  cifs: Enable Kconfig control of CIFS_DEBUG

 fs/cifs/Kconfig      |   10 ++++++-
 fs/cifs/cifs_debug.h |   74 +++++++++++++++++++++++++++++--------------------
 2 files changed, 53 insertions(+), 31 deletions(-)

-- 
1.7.8.112.g3fd21

[PATCH 1/3] cifs: Remove unused cEVENT macro

[Joe Perches]

It uses an undefined KERN_EVENT and is itself unused.

Signed-off-by: Joe Perches <joe@perches.com>
---
 fs/cifs/cifs_debug.h |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/fs/cifs/cifs_debug.h b/fs/cifs/cifs_debug.h
index c0c68bb..b0fc344 100644
--- a/fs/cifs/cifs_debug.h
+++ b/fs/cifs/cifs_debug.h
@@ -63,12 +63,6 @@ do {						\
 /* debug event message: */
 extern int cifsERROR;
 
-#define cEVENT(fmt, arg...)						\
-do {									\
-	if (cifsERROR)							\
-		printk(KERN_EVENT "%s: " fmt "\n", __FILE__, ##arg);	\
-} while (0)
-
 /* error event message: e.g., i/o error */
 #define cifserror(fmt, arg...)					\
 do {								\
@@ -88,7 +82,6 @@ do {						\
  */
 #else		/* _CIFS_DEBUG */
 #define cERROR(set, fmt, arg...)
-#define cEVENT(fmt, arg...)
 #define cFYI(set, fmt, arg...)
 #define cifserror(fmt, arg...)
 #endif		/* _CIFS_DEBUG */
-- 
1.7.8.112.g3fd21

Re: [PATCH 1/3] cifs: Remove unused cEVENT macro

[Jeff Layton]

On Thu, 29 Nov 2012 11:37:18 -0800
Joe Perches <joe@perches.com> wrote:

> It uses an undefined KERN_EVENT and is itself unused.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  fs/cifs/cifs_debug.h |    7 -------
>  1 files changed, 0 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/cifs/cifs_debug.h b/fs/cifs/cifs_debug.h
> index c0c68bb..b0fc344 100644
> --- a/fs/cifs/cifs_debug.h
> +++ b/fs/cifs/cifs_debug.h
> @@ -63,12 +63,6 @@ do {						\
>  /* debug event message: */
>  extern int cifsERROR;
>  
> -#define cEVENT(fmt, arg...)						\
> -do {									\
> -	if (cifsERROR)							\
> -		printk(KERN_EVENT "%s: " fmt "\n", __FILE__, ##arg);	\
> -} while (0)
> -
>  /* error event message: e.g., i/o error */
>  #define cifserror(fmt, arg...)					\
>  do {								\
> @@ -88,7 +82,6 @@ do {						\
>   */
>  #else		/* _CIFS_DEBUG */
>  #define cERROR(set, fmt, arg...)
> -#define cEVENT(fmt, arg...)
>  #define cFYI(set, fmt, arg...)
>  #define cifserror(fmt, arg...)
>  #endif		/* _CIFS_DEBUG */

Yay! More cruft removal!

Reviewed-by: Jeff Layton <jlayton@redhat.com>

Re: [PATCH 1/3] cifs: Remove unused cEVENT macro

[Steve French]

I don't mind removing cEVENT (it is currently unused, and presumably
the older file systems that used EVENT logging, have moved to dtrace
or other tracing mechanisms).

But ... the need still remains for more tracing for cifs (I run into
this a few times a month at least when debugging) - does anyone have
any ideas for how to add dynamic trace points to cifs (has anyone even
done it for the vfs?) - is there any file system that does a good job
defining dynamic trace points?

On Fri, Nov 30, 2012 at 5:47 AM, Jeff Layton <jlayton@redhat.com> wrote:
> On Thu, 29 Nov 2012 11:37:18 -0800
> Joe Perches <joe@perches.com> wrote:
>
>> It uses an undefined KERN_EVENT and is itself unused.
>>
>> Signed-off-by: Joe Perches <joe@perches.com>
>> ---
>>  fs/cifs/cifs_debug.h |    7 -------
>>  1 files changed, 0 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/cifs/cifs_debug.h b/fs/cifs/cifs_debug.h
>> index c0c68bb..b0fc344 100644
>> --- a/fs/cifs/cifs_debug.h
>> +++ b/fs/cifs/cifs_debug.h
>> @@ -63,12 +63,6 @@ do {                                               \
>>  /* debug event message: */
>>  extern int cifsERROR;
>>
>> -#define cEVENT(fmt, arg...)                                          \
>> -do {                                                                 \
>> -     if (cifsERROR)                                                  \
>> -             printk(KERN_EVENT "%s: " fmt "\n", __FILE__, ##arg);    \
>> -} while (0)
>> -
>>  /* error event message: e.g., i/o error */
>>  #define cifserror(fmt, arg...)                                       \
>>  do {                                                         \
>> @@ -88,7 +82,6 @@ do {                                                \
>>   */
>>  #else                /* _CIFS_DEBUG */
>>  #define cERROR(set, fmt, arg...)
>> -#define cEVENT(fmt, arg...)
>>  #define cFYI(set, fmt, arg...)
>>  #define cifserror(fmt, arg...)
>>  #endif               /* _CIFS_DEBUG */
>
> Yay! More cruft removal!
>
> Reviewed-by: Jeff Layton <jlayton@redhat.com>



-- 
Thanks,

Steve

Re: [PATCH 1/3] cifs: Remove unused cEVENT macro

[Joe Perches]

On Fri, 2012-11-30 at 09:39 -0600, Steve French wrote:
> But ... the need still remains for more tracing for cifs (I run into
> this a few times a month at least when debugging) - does anyone have
> any ideas for how to add dynamic trace points to cifs (has anyone even
> done it for the vfs?) - is there any file system that does a good job
> defining dynamic trace points?

I believe ext4 is the only filesystem with significant use of
tracepoints.

Ted? Any tips you care to give as to best implementation methods?

Re: [PATCH 1/3] cifs: Remove unused cEVENT macro

[Joe Perches]

(resending due to bounces, sorry for any inconvenience)

On Fri, 2012-11-30 at 09:39 -0600, Steve French wrote:
> But ... the need still remains for more tracing for cifs (I run into
> this a few times a month at least when debugging) - does anyone have
> any ideas for how to add dynamic trace points to cifs (has anyone even
> done it for the vfs?) - is there any file system that does a good job
> defining dynamic trace points?

I believe ext4 is the only filesystem with significant use of
tracepoints.

Ted? Any tips you care to give as to best implementation methods?

[PATCH 2/3] cifs: Make CIFS_DEBUG possible to undefine

[Joe Perches]

Make the compilation work again when CIFS_DEBUG is not #define'd.

Add format and argument verification for the various macros when
CIFS_DEBUG is not #define'd.

Signed-off-by: Joe Perches <joe@perches.com>
---
 fs/cifs/cifs_debug.h |   64 ++++++++++++++++++++++++++++++++------------------
 1 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/fs/cifs/cifs_debug.h b/fs/cifs/cifs_debug.h
index b0fc344..4d12fe4 100644
--- a/fs/cifs/cifs_debug.h
+++ b/fs/cifs/cifs_debug.h
@@ -37,6 +37,9 @@ void dump_smb(void *, int);
 #define CIFS_RC		0x02
 #define CIFS_TIMER	0x04
 
+extern int cifsFYI;
+extern int cifsERROR;
+
 /*
  *	debug ON
  *	--------
@@ -44,36 +47,33 @@ void dump_smb(void *, int);
 #ifdef CIFS_DEBUG
 
 /* information message: e.g., configuration, major event */
-extern int cifsFYI;
-#define cifsfyi(fmt, arg...)						\
+#define cifsfyi(fmt, ...)						\
 do {									\
 	if (cifsFYI & CIFS_INFO)					\
-		printk(KERN_DEBUG "%s: " fmt "\n", __FILE__, ##arg);	\
+		printk(KERN_DEBUG "%s: " fmt "\n",			\
+		       __FILE__, ##__VA_ARGS__);			\
 } while (0)
 
-#define cFYI(set, fmt, arg...)			\
-do {						\
-	if (set)				\
-		cifsfyi(fmt, ##arg);		\
+#define cFYI(set, fmt, ...)						\
+do {									\
+	if (set)							\
+		cifsfyi(fmt, ##__VA_ARGS__);				\
 } while (0)
 
-#define cifswarn(fmt, arg...)			\
-	printk(KERN_WARNING fmt "\n", ##arg)
-
-/* debug event message: */
-extern int cifsERROR;
+#define cifswarn(fmt, ...)						\
+	printk(KERN_WARNING fmt "\n", ##__VA_ARGS__)
 
 /* error event message: e.g., i/o error */
-#define cifserror(fmt, arg...)					\
-do {								\
-	if (cifsERROR)						\
-		printk(KERN_ERR "CIFS VFS: " fmt "\n", ##arg);	\
+#define cifserror(fmt, ...)						\
+do {									\
+	if (cifsERROR)							\
+		printk(KERN_ERR "CIFS VFS: " fmt "\n", ##__VA_ARGS__);	\
 } while (0)
 
-#define cERROR(set, fmt, arg...)		\
-do {						\
-	if (set)				\
-		cifserror(fmt, ##arg);		\
+#define cERROR(set, fmt, ...)						\
+do {									\
+	if (set)							\
+		cifserror(fmt, ##__VA_ARGS__);				\
 } while (0)
 
 /*
@@ -81,9 +81,27 @@ do {						\
  *	---------
  */
 #else		/* _CIFS_DEBUG */
-#define cERROR(set, fmt, arg...)
-#define cFYI(set, fmt, arg...)
-#define cifserror(fmt, arg...)
+#define cifsfyi(fmt, ...)						\
+do {									\
+	if (0)								\
+		printk(KERN_DEBUG "%s: " fmt "\n",			\
+		       __FILE__, ##__VA_ARGS__);			\
+} while (0)
+#define cFYI(set, fmt, ...)						\
+do {									\
+	if (0 && set)							\
+		cifsfyi(fmt, ##__VA_ARGS__);				\
+} while (0)
+#define cifserror(fmt, ...)						\
+do {									\
+	if (0)								\
+		printk(KERN_ERR "CIFS VFS: " fmt "\n", ##__VA_ARGS__);	\
+} while (0)
+#define cERROR(set, fmt, ...)						\
+do {									\
+	if (0 && set)							\
+		cifserror(fmt, ##__VA_ARGS__);				\
+} while (0)
 #endif		/* _CIFS_DEBUG */
 
 #endif				/* _H_CIFS_DEBUG */
-- 
1.7.8.112.g3fd21

Re: [PATCH 2/3] cifs: Make CIFS_DEBUG possible to undefine

[Jeff Layton]

On Thu, 29 Nov 2012 11:37:19 -0800
Joe Perches <joe@perches.com> wrote:

> Make the compilation work again when CIFS_DEBUG is not #define'd.
> 
> Add format and argument verification for the various macros when
> CIFS_DEBUG is not #define'd.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  fs/cifs/cifs_debug.h |   64 ++++++++++++++++++++++++++++++++------------------
>  1 files changed, 41 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/cifs/cifs_debug.h b/fs/cifs/cifs_debug.h
> index b0fc344..4d12fe4 100644
> --- a/fs/cifs/cifs_debug.h
> +++ b/fs/cifs/cifs_debug.h
> @@ -37,6 +37,9 @@ void dump_smb(void *, int);
>  #define CIFS_RC		0x02
>  #define CIFS_TIMER	0x04
>  
> +extern int cifsFYI;
> +extern int cifsERROR;
> +
>  /*
>   *	debug ON
>   *	--------
> @@ -44,36 +47,33 @@ void dump_smb(void *, int);
>  #ifdef CIFS_DEBUG
>  
>  /* information message: e.g., configuration, major event */
> -extern int cifsFYI;
> -#define cifsfyi(fmt, arg...)						\
> +#define cifsfyi(fmt, ...)						\
>  do {									\
>  	if (cifsFYI & CIFS_INFO)					\
> -		printk(KERN_DEBUG "%s: " fmt "\n", __FILE__, ##arg);	\
> +		printk(KERN_DEBUG "%s: " fmt "\n",			\
> +		       __FILE__, ##__VA_ARGS__);			\
>  } while (0)
>  
> -#define cFYI(set, fmt, arg...)			\
> -do {						\
> -	if (set)				\
> -		cifsfyi(fmt, ##arg);		\
> +#define cFYI(set, fmt, ...)						\
> +do {									\
> +	if (set)							\
> +		cifsfyi(fmt, ##__VA_ARGS__);				\
>  } while (0)
>  
> -#define cifswarn(fmt, arg...)			\
> -	printk(KERN_WARNING fmt "\n", ##arg)
> -
> -/* debug event message: */
> -extern int cifsERROR;
> +#define cifswarn(fmt, ...)						\
> +	printk(KERN_WARNING fmt "\n", ##__VA_ARGS__)
>  
>  /* error event message: e.g., i/o error */
> -#define cifserror(fmt, arg...)					\
> -do {								\
> -	if (cifsERROR)						\
> -		printk(KERN_ERR "CIFS VFS: " fmt "\n", ##arg);	\
> +#define cifserror(fmt, ...)						\
> +do {									\
> +	if (cifsERROR)							\
> +		printk(KERN_ERR "CIFS VFS: " fmt "\n", ##__VA_ARGS__);	\
>  } while (0)
>  
> -#define cERROR(set, fmt, arg...)		\
> -do {						\
> -	if (set)				\
> -		cifserror(fmt, ##arg);		\
> +#define cERROR(set, fmt, ...)						\
> +do {									\
> +	if (set)							\
> +		cifserror(fmt, ##__VA_ARGS__);				\
>  } while (0)
>  
>  /*
> @@ -81,9 +81,27 @@ do {						\
>   *	---------
>   */
>  #else		/* _CIFS_DEBUG */
> -#define cERROR(set, fmt, arg...)
> -#define cFYI(set, fmt, arg...)
> -#define cifserror(fmt, arg...)
> +#define cifsfyi(fmt, ...)						\
> +do {									\
> +	if (0)								\
> +		printk(KERN_DEBUG "%s: " fmt "\n",			\
> +		       __FILE__, ##__VA_ARGS__);			\
> +} while (0)
> +#define cFYI(set, fmt, ...)						\
> +do {									\
> +	if (0 && set)							\
> +		cifsfyi(fmt, ##__VA_ARGS__);				\
> +} while (0)
> +#define cifserror(fmt, ...)						\
> +do {									\
> +	if (0)								\
> +		printk(KERN_ERR "CIFS VFS: " fmt "\n", ##__VA_ARGS__);	\
> +} while (0)
> +#define cERROR(set, fmt, ...)						\
> +do {									\
> +	if (0 && set)							\
> +		cifserror(fmt, ##__VA_ARGS__);				\
> +} while (0)

Would it be better to simply make those the standard

"do { ; } while(0)" noop macros?

I'm not sure I see the point in keeping the printk statements in there...

>  #endif		/* _CIFS_DEBUG */
>  
>  #endif				/* _H_CIFS_DEBUG */

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 2/3] cifs: Make CIFS_DEBUG possible to undefine

[Joe Perches]

On Fri, 2012-11-30 at 06:49 -0500, Jeff Layton wrote:
> On Thu, 29 Nov 2012 11:37:19 -0800
> Joe Perches <joe@perches.com> wrote:
> 
> > Make the compilation work again when CIFS_DEBUG is not #define'd.
> > 
> > Add format and argument verification for the various macros when
> > CIFS_DEBUG is not #define'd.
[]
> Would it be better to simply make those the standard
> 
> "do { ; } while(0)" noop macros?
> 
> I'm not sure I see the point in keeping the printk statements in there...

Actually, that's not standard.
Look at printk.h

It prevents developers from adding code that compiles
without warnings in one mode but compiles with warnings
in another.

The idea is to make sure that format and arguments always
match regardless of whether or not you are compiling
debug or non-debug.

Re: [PATCH 2/3] cifs: Make CIFS_DEBUG possible to undefine

[Jeff Layton]

On Fri, 30 Nov 2012 06:52:09 -0800
Joe Perches <joe@perches.com> wrote:

> On Fri, 2012-11-30 at 06:49 -0500, Jeff Layton wrote:
> > On Thu, 29 Nov 2012 11:37:19 -0800
> > Joe Perches <joe@perches.com> wrote:
> > 
> > > Make the compilation work again when CIFS_DEBUG is not #define'd.
> > > 
> > > Add format and argument verification for the various macros when
> > > CIFS_DEBUG is not #define'd.
> []
> > Would it be better to simply make those the standard
> > 
> > "do { ; } while(0)" noop macros?
> > 
> > I'm not sure I see the point in keeping the printk statements in there...
> 
> Actually, that's not standard.
> Look at printk.h
> 
> It prevents developers from adding code that compiles
> without warnings in one mode but compiles with warnings
> in another.
> 
> The idea is to make sure that format and arguments always
> match regardless of whether or not you are compiling
> debug or non-debug.
> 
> 

Fair enough then...

Reviewed-by: Jeff Layton <jlayton@redhat.com>

[PATCH 3/3] cifs: Enable Kconfig control of CIFS_DEBUG

[Joe Perches]

Rather than havign a permanently enabled #define, allow
control over it.

This can reduce the size of the module by ~100KB which
could be useful for embedded systems.

Signed-off-by: Joe Perches <joe@perches.com>
---
 fs/cifs/Kconfig      |   10 +++++++++-
 fs/cifs/cifs_debug.h |    5 ++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/Kconfig b/fs/cifs/Kconfig
index 2075ddf..21ff76c 100644
--- a/fs/cifs/Kconfig
+++ b/fs/cifs/Kconfig
@@ -122,9 +122,17 @@ config CIFS_ACL
 	    Allows fetching CIFS/NTFS ACL from the server.  The DACL blob
 	    is handed over to the application/caller.
 
+config CIFS_DEBUG
+	bool "Enable CIFS debugging routines"
+	default y
+	depends on CIFS
+	help
+	   Enabling this option adds helpful debugging messages to
+	   the cifs code which increases the size of the cifs module.
+	   If unsure, say Y.
 config CIFS_DEBUG2
 	bool "Enable additional CIFS debugging routines"
-	depends on CIFS
+	depends on CIFS_DEBUG
 	help
 	   Enabling this option adds a few more debugging routines
 	   to the cifs code which slightly increases the size of
diff --git a/fs/cifs/cifs_debug.h b/fs/cifs/cifs_debug.h
index 4d12fe4..6867a7f 100644
--- a/fs/cifs/cifs_debug.h
+++ b/fs/cifs/cifs_debug.h
@@ -18,7 +18,10 @@
  *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
  *
 */
-#define CIFS_DEBUG		/* BB temporary */
+
+#ifdef CONFIG_CIFS_DEBUG
+#define CIFS_DEBUG
+#endif
 
 #ifndef _H_CIFS_DEBUG
 #define _H_CIFS_DEBUG
-- 
1.7.8.112.g3fd21

Re: [PATCH 3/3] cifs: Enable Kconfig control of CIFS_DEBUG

[Jeff Layton]

On Thu, 29 Nov 2012 11:37:20 -0800
Joe Perches <joe@perches.com> wrote:

> Rather than havign a permanently enabled #define, allow
> control over it.
> 
> This can reduce the size of the module by ~100KB which
> could be useful for embedded systems.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  fs/cifs/Kconfig      |   10 +++++++++-
>  fs/cifs/cifs_debug.h |    5 ++++-
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/cifs/Kconfig b/fs/cifs/Kconfig
> index 2075ddf..21ff76c 100644
> --- a/fs/cifs/Kconfig
> +++ b/fs/cifs/Kconfig
> @@ -122,9 +122,17 @@ config CIFS_ACL
>  	    Allows fetching CIFS/NTFS ACL from the server.  The DACL blob
>  	    is handed over to the application/caller.
>  
> +config CIFS_DEBUG
> +	bool "Enable CIFS debugging routines"
> +	default y
> +	depends on CIFS
> +	help
> +	   Enabling this option adds helpful debugging messages to
> +	   the cifs code which increases the size of the cifs module.
> +	   If unsure, say Y.
>  config CIFS_DEBUG2
>  	bool "Enable additional CIFS debugging routines"
> -	depends on CIFS
> +	depends on CIFS_DEBUG
>  	help
>  	   Enabling this option adds a few more debugging routines
>  	   to the cifs code which slightly increases the size of
> diff --git a/fs/cifs/cifs_debug.h b/fs/cifs/cifs_debug.h
> index 4d12fe4..6867a7f 100644
> --- a/fs/cifs/cifs_debug.h
> +++ b/fs/cifs/cifs_debug.h
> @@ -18,7 +18,10 @@
>   *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>   *
>  */
> -#define CIFS_DEBUG		/* BB temporary */
> +
> +#ifdef CONFIG_CIFS_DEBUG
> +#define CIFS_DEBUG
> +#endif
>  
>  #ifndef _H_CIFS_DEBUG
>  #define _H_CIFS_DEBUG

Looks fine, but I'd probably prefer to just get rid of CIFS_DEBUG
altogether here and replace it with CONFIG_CIFS_DEBUG.

While you're at it, /proc/fs/cifs/cifsFYI should probably not be
present when CONFIG_CIFS_DEBUG isn't set. Perhaps we should also
disable traceSMB and its related functions too in that case?

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 3/3] cifs: Enable Kconfig control of CIFS_DEBUG

[Joe Perches]

On Fri, 2012-11-30 at 06:56 -0500, Jeff Layton wrote:
> On Thu, 29 Nov 2012 11:37:20 -0800
> Joe Perches <joe@perches.com> wrote:
> 
> > Rather than havign a permanently enabled #define, allow
> > control over it.
> > 
> > This can reduce the size of the module by ~100KB which
> > could be useful for embedded systems.
[]
> > diff --git a/fs/cifs/cifs_debug.h b/fs/cifs/cifs_debug.h
> > index 4d12fe4..6867a7f 100644
> > --- a/fs/cifs/cifs_debug.h
> > +++ b/fs/cifs/cifs_debug.h
> > @@ -18,7 +18,10 @@
> >   *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> >   *
> >  */
> > -#define CIFS_DEBUG		/* BB temporary */
> > +
> > +#ifdef CONFIG_CIFS_DEBUG
> > +#define CIFS_DEBUG
> > +#endif
> >  
> >  #ifndef _H_CIFS_DEBUG
> >  #define _H_CIFS_DEBUG
> 
> Looks fine, but I'd probably prefer to just get rid of CIFS_DEBUG
> altogether here and replace it with CONFIG_CIFS_DEBUG.

I wasn't sure if there was other code anywhere else that depends
on CIFS_DEBUG.  There isn't any in the kernel source tree.  If not,
then you are right, it should be just CONFIG_CIFS_DEBUG.

> While you're at it, /proc/fs/cifs/cifsFYI should probably not be
> present when CONFIG_CIFS_DEBUG isn't set. Perhaps we should also
> disable traceSMB and its related functions too in that case?

That could be considered a kernel api change.
If people agree to delete it, I'd be OK with it.

Re: [PATCH 3/3] cifs: Enable Kconfig control of CIFS_DEBUG

[Jeff Layton]

On Fri, 30 Nov 2012 06:57:40 -0800
Joe Perches <joe@perches.com> wrote:

> On Fri, 2012-11-30 at 06:56 -0500, Jeff Layton wrote:
> > On Thu, 29 Nov 2012 11:37:20 -0800
> > Joe Perches <joe@perches.com> wrote:
> > 
> > > Rather than havign a permanently enabled #define, allow
> > > control over it.
> > > 
> > > This can reduce the size of the module by ~100KB which
> > > could be useful for embedded systems.
> []
> > > diff --git a/fs/cifs/cifs_debug.h b/fs/cifs/cifs_debug.h
> > > index 4d12fe4..6867a7f 100644
> > > --- a/fs/cifs/cifs_debug.h
> > > +++ b/fs/cifs/cifs_debug.h
> > > @@ -18,7 +18,10 @@
> > >   *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> > >   *
> > >  */
> > > -#define CIFS_DEBUG		/* BB temporary */
> > > +
> > > +#ifdef CONFIG_CIFS_DEBUG
> > > +#define CIFS_DEBUG
> > > +#endif
> > >  
> > >  #ifndef _H_CIFS_DEBUG
> > >  #define _H_CIFS_DEBUG
> > 
> > Looks fine, but I'd probably prefer to just get rid of CIFS_DEBUG
> > altogether here and replace it with CONFIG_CIFS_DEBUG.
> 
> I wasn't sure if there was other code anywhere else that depends
> on CIFS_DEBUG.  There isn't any in the kernel source tree.  If not,
> then you are right, it should be just CONFIG_CIFS_DEBUG.
> 

There shouldn't be. None of this stuff is exposed to userspace, AFAIK.
I think it's ok to just make that CONFIG_CIFS_DEBUG.

> > While you're at it, /proc/fs/cifs/cifsFYI should probably not be
> > present when CONFIG_CIFS_DEBUG isn't set. Perhaps we should also
> > disable traceSMB and its related functions too in that case?
> 
> That could be considered a kernel api change.
> If people agree to delete it, I'd be OK with it.
> 

If you compile with CONFIG_CIFS_DEBUG turned off, then the cifsFYI file
will be a knob that's not hooked up to anything. I think it's best not
to present it at all in that case.

traceSMB is a little harder to make the case for. Theoretically, it'll
still work after your changes, but I'm not sure it makes sense to leave
that knob around since it's technically a flag for debugging. You're
probably fine not to worry about that one. I'll look at that at some
point in the future.

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

[PATCH V2 3/3] cifs: Add CONFIG_CIFS_DEBUG and rename use of CIFS_DEBUG

[Joe Perches]

On Fri, 2012-11-30 at 10:07 -0500, Jeff Layton wrote:
> On Fri, 30 Nov 2012 06:57:40 -0800
> Joe Perches <joe@perches.com> wrote:
> > On Fri, 2012-11-30 at 06:56 -0500, Jeff Layton wrote:
> > > On Thu, 29 Nov 2012 11:37:20 -0800
> > > Joe Perches <joe@perches.com> wrote:
> > > > Rather than havign a permanently enabled #define, allow
> > > > control over it.
> > > > 
> > > > This can reduce the size of the module by ~100KB which
> > > > could be useful for embedded systems.
> > []
> > > > diff --git a/fs/cifs/cifs_debug.h b/fs/cifs/cifs_debug.h
> > > > index 4d12fe4..6867a7f 100644
> > > > --- a/fs/cifs/cifs_debug.h
> > > > +++ b/fs/cifs/cifs_debug.h
> > > > @@ -18,7 +18,10 @@
> > > >   *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> > > >   *
> > > >  */
> > > > -#define CIFS_DEBUG		/* BB temporary */
> > > > +
> > > > +#ifdef CONFIG_CIFS_DEBUG
> > > > +#define CIFS_DEBUG
> > > > +#endif
> > > >  
> > > >  #ifndef _H_CIFS_DEBUG
> > > >  #define _H_CIFS_DEBUG
> > > 
> > > Looks fine, but I'd probably prefer to just get rid of CIFS_DEBUG
> > > altogether here and replace it with CONFIG_CIFS_DEBUG.
> > 
> > I wasn't sure if there was other code anywhere else that depends
> > on CIFS_DEBUG.  There isn't any in the kernel source tree.  If not,
> > then you are right, it should be just CONFIG_CIFS_DEBUG.
> There shouldn't be. None of this stuff is exposed to userspace, AFAIK.
> I think it's ok to just make that CONFIG_CIFS_DEBUG.
> 
> > > While you're at it, /proc/fs/cifs/cifsFYI should probably not be
> > > present when CONFIG_CIFS_DEBUG isn't set. Perhaps we should also
> > > disable traceSMB and its related functions too in that case?
> > 
> > That could be considered a kernel api change.
> > If people agree to delete it, I'd be OK with it.
> > 
> 
> If you compile with CONFIG_CIFS_DEBUG turned off, then the cifsFYI file
> will be a knob that's not hooked up to anything. I think it's best not
> to present it at all in that case.

If there's a userspace script that does
echo 1 > /proc/fs/cifs/cifsFYI
it'd now get a file not found error.

> traceSMB is a little harder to make the case for. Theoretically, it'll
> still work after your changes, but I'm not sure it makes sense to leave
> that knob around since it's technically a flag for debugging. You're
> probably fine not to worry about that one. I'll look at that at some
> point in the future.

This could be changed in a separate patch if appropriate.
Here's the modified patch.

From: Joe Perches <joe@perches.com>

This can reduce the size of the module by ~120KB which
could be useful for embedded systems.

$ size fs/cifs/built-in.o*
   text	   data	    bss	    dec	    hex	filename
 388567	  34459	 100440	 523466	  7fcca	fs/cifs/built-in.o.new
 495970	  34599	 117904	 648473	  9e519	fs/cifs/built-in.o.old

Signed-off-by: Joe Perches <joe@perches.com>
---
 fs/cifs/Kconfig      |   10 +++++++++-
 fs/cifs/cifs_debug.h |    3 +--
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/Kconfig b/fs/cifs/Kconfig
index 2075ddf..21ff76c 100644
--- a/fs/cifs/Kconfig
+++ b/fs/cifs/Kconfig
@@ -122,9 +122,17 @@ config CIFS_ACL
 	    Allows fetching CIFS/NTFS ACL from the server.  The DACL blob
 	    is handed over to the application/caller.
 
+config CIFS_DEBUG
+	bool "Enable CIFS debugging routines"
+	default y
+	depends on CIFS
+	help
+	   Enabling this option adds helpful debugging messages to
+	   the cifs code which increases the size of the cifs module.
+	   If unsure, say Y.
 config CIFS_DEBUG2
 	bool "Enable additional CIFS debugging routines"
-	depends on CIFS
+	depends on CIFS_DEBUG
 	help
 	   Enabling this option adds a few more debugging routines
 	   to the cifs code which slightly increases the size of
diff --git a/fs/cifs/cifs_debug.h b/fs/cifs/cifs_debug.h
index 4d12fe4..86e92ef 100644
--- a/fs/cifs/cifs_debug.h
+++ b/fs/cifs/cifs_debug.h
@@ -18,7 +18,6 @@
  *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
  *
 */
-#define CIFS_DEBUG		/* BB temporary */
 
 #ifndef _H_CIFS_DEBUG
 #define _H_CIFS_DEBUG
@@ -44,7 +43,7 @@ extern int cifsERROR;
  *	debug ON
  *	--------
  */
-#ifdef CIFS_DEBUG
+#ifdef CONFIG_CIFS_DEBUG
 
 /* information message: e.g., configuration, major event */
 #define cifsfyi(fmt, ...)						\
-- 
1.7.8.112.g3fd21

Re: [PATCH V2 3/3] cifs: Add CONFIG_CIFS_DEBUG and rename use of CIFS_DEBUG

[Jeff Layton]

On Fri, 30 Nov 2012 07:31:56 -0800
Joe Perches <joe@perches.com> wrote:

> On Fri, 2012-11-30 at 10:07 -0500, Jeff Layton wrote:
> > On Fri, 30 Nov 2012 06:57:40 -0800
> > Joe Perches <joe@perches.com> wrote:
> > > On Fri, 2012-11-30 at 06:56 -0500, Jeff Layton wrote:
> > > > On Thu, 29 Nov 2012 11:37:20 -0800
> > > > Joe Perches <joe@perches.com> wrote:
> > > > > Rather than havign a permanently enabled #define, allow
> > > > > control over it.
> > > > > 
> > > > > This can reduce the size of the module by ~100KB which
> > > > > could be useful for embedded systems.
> > > []
> > > > > diff --git a/fs/cifs/cifs_debug.h b/fs/cifs/cifs_debug.h
> > > > > index 4d12fe4..6867a7f 100644
> > > > > --- a/fs/cifs/cifs_debug.h
> > > > > +++ b/fs/cifs/cifs_debug.h
> > > > > @@ -18,7 +18,10 @@
> > > > >   *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> > > > >   *
> > > > >  */
> > > > > -#define CIFS_DEBUG		/* BB temporary */
> > > > > +
> > > > > +#ifdef CONFIG_CIFS_DEBUG
> > > > > +#define CIFS_DEBUG
> > > > > +#endif
> > > > >  
> > > > >  #ifndef _H_CIFS_DEBUG
> > > > >  #define _H_CIFS_DEBUG
> > > > 
> > > > Looks fine, but I'd probably prefer to just get rid of CIFS_DEBUG
> > > > altogether here and replace it with CONFIG_CIFS_DEBUG.
> > > 
> > > I wasn't sure if there was other code anywhere else that depends
> > > on CIFS_DEBUG.  There isn't any in the kernel source tree.  If not,
> > > then you are right, it should be just CONFIG_CIFS_DEBUG.
> > There shouldn't be. None of this stuff is exposed to userspace, AFAIK.
> > I think it's ok to just make that CONFIG_CIFS_DEBUG.
> > 
> > > > While you're at it, /proc/fs/cifs/cifsFYI should probably not be
> > > > present when CONFIG_CIFS_DEBUG isn't set. Perhaps we should also
> > > > disable traceSMB and its related functions too in that case?
> > > 
> > > That could be considered a kernel api change.
> > > If people agree to delete it, I'd be OK with it.
> > > 
> > 
> > If you compile with CONFIG_CIFS_DEBUG turned off, then the cifsFYI file
> > will be a knob that's not hooked up to anything. I think it's best not
> > to present it at all in that case.
> 
> If there's a userspace script that does
> echo 1 > /proc/fs/cifs/cifsFYI
> it'd now get a file not found error.
> 

It probably should fail since it wouldn't get the results it was
looking for anyway...

> > traceSMB is a little harder to make the case for. Theoretically, it'll
> > still work after your changes, but I'm not sure it makes sense to leave
> > that knob around since it's technically a flag for debugging. You're
> > probably fine not to worry about that one. I'll look at that at some
> > point in the future.
> 
> This could be changed in a separate patch if appropriate.
> Here's the modified patch.
> 
> From: Joe Perches <joe@perches.com>
> 
> This can reduce the size of the module by ~120KB which
> could be useful for embedded systems.
> 
> $ size fs/cifs/built-in.o*
>    text	   data	    bss	    dec	    hex	filename
>  388567	  34459	 100440	 523466	  7fcca	fs/cifs/built-in.o.new
>  495970	  34599	 117904	 648473	  9e519	fs/cifs/built-in.o.old
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  fs/cifs/Kconfig      |   10 +++++++++-
>  fs/cifs/cifs_debug.h |    3 +--
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/cifs/Kconfig b/fs/cifs/Kconfig
> index 2075ddf..21ff76c 100644
> --- a/fs/cifs/Kconfig
> +++ b/fs/cifs/Kconfig
> @@ -122,9 +122,17 @@ config CIFS_ACL
>  	    Allows fetching CIFS/NTFS ACL from the server.  The DACL blob
>  	    is handed over to the application/caller.
>  
> +config CIFS_DEBUG
> +	bool "Enable CIFS debugging routines"
> +	default y
> +	depends on CIFS
> +	help
> +	   Enabling this option adds helpful debugging messages to
> +	   the cifs code which increases the size of the cifs module.
> +	   If unsure, say Y.
>  config CIFS_DEBUG2
>  	bool "Enable additional CIFS debugging routines"
> -	depends on CIFS
> +	depends on CIFS_DEBUG
>  	help
>  	   Enabling this option adds a few more debugging routines
>  	   to the cifs code which slightly increases the size of
> diff --git a/fs/cifs/cifs_debug.h b/fs/cifs/cifs_debug.h
> index 4d12fe4..86e92ef 100644
> --- a/fs/cifs/cifs_debug.h
> +++ b/fs/cifs/cifs_debug.h
> @@ -18,7 +18,6 @@
>   *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>   *
>  */
> -#define CIFS_DEBUG		/* BB temporary */
>  
>  #ifndef _H_CIFS_DEBUG
>  #define _H_CIFS_DEBUG
> @@ -44,7 +43,7 @@ extern int cifsERROR;
>   *	debug ON
>   *	--------
>   */
> -#ifdef CIFS_DEBUG
> +#ifdef CONFIG_CIFS_DEBUG
>  
>  /* information message: e.g., configuration, major event */
>  #define cifsfyi(fmt, ...)						\

Yep, fair enough, we can move the cifsFYI file under CONFIG_CIFS_DEBUG
in a separate patch.

Reviewed-by: Jeff Layton <jlayton@redhat.com>

Re: [PATCH 3/3] cifs: Enable Kconfig control of CIFS_DEBUG

[Steve French]

On Fri, Nov 30, 2012 at 9:07 AM, Jeff Layton <jlayton@redhat.com> wrote:
> On Fri, 30 Nov 2012 06:57:40 -0800
> Joe Perches <joe@perches.com> wrote:
>
>> On Fri, 2012-11-30 at 06:56 -0500, Jeff Layton wrote:
>> > On Thu, 29 Nov 2012 11:37:20 -0800
>> > Joe Perches <joe@perches.com> wrote:
>> >
>> > > Rather than havign a permanently enabled #define, allow
>> > > control over it.
>> > >
>> > > This can reduce the size of the module by ~100KB which
>> > > could be useful for embedded systems.
>> []
>> > > diff --git a/fs/cifs/cifs_debug.h b/fs/cifs/cifs_debug.h
>> > > index 4d12fe4..6867a7f 100644
>> > > --- a/fs/cifs/cifs_debug.h
>> > > +++ b/fs/cifs/cifs_debug.h
>> > > @@ -18,7 +18,10 @@
>> > >   *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>> > >   *
>> > >  */
>> > > -#define CIFS_DEBUG               /* BB temporary */
>> > > +
>> > > +#ifdef CONFIG_CIFS_DEBUG
>> > > +#define CIFS_DEBUG
>> > > +#endif
>> > >
>> > >  #ifndef _H_CIFS_DEBUG
>> > >  #define _H_CIFS_DEBUG
>> >
>> > Looks fine, but I'd probably prefer to just get rid of CIFS_DEBUG
>> > altogether here and replace it with CONFIG_CIFS_DEBUG.
>>
>> I wasn't sure if there was other code anywhere else that depends
>> on CIFS_DEBUG.  There isn't any in the kernel source tree.  If not,
>> then you are right, it should be just CONFIG_CIFS_DEBUG.
>>
>
> There shouldn't be. None of this stuff is exposed to userspace, AFAIK.
> I think it's ok to just make that CONFIG_CIFS_DEBUG.
>
>> > While you're at it, /proc/fs/cifs/cifsFYI should probably not be
>> > present when CONFIG_CIFS_DEBUG isn't set. Perhaps we should also
>> > disable traceSMB and its related functions too in that case?
>>
>> That could be considered a kernel api change.
>> If people agree to delete it, I'd be OK with it.
>>
>
> If you compile with CONFIG_CIFS_DEBUG turned off, then the cifsFYI file
> will be a knob that's not hooked up to anything. I think it's best not
> to present it at all in that case.
>
> traceSMB is a little harder to make the case for. Theoretically, it'll
> still work after your changes, but I'm not sure it makes sense to leave
> that knob around since it's technically a flag for debugging. You're
> probably fine not to worry about that one. I'll look at that at some
> point in the future.

I think it is fine to leave the traceSMB around with debug config flag
off - it is not strongly related to cERROR and cFYI and is small.


-- 
Thanks,

Steve