[PATCH] SUNRPC: Mark buffer used for debug printks with __maybe_unused

[PATCH] SUNRPC: Mark buffer used for debug printks with __maybe_unused

[Pavel Emelyanov]

There are tree places, which declare the char buf[...] on the stack
to push it later into dprintk(). Since the dprintk sometimes (if the 
CONFIG_SYSCTL=n) becomes an empty do { } while (0) stub, these buffers
cause gcc to produce appropriate warnings.

Mark them as __maybe_unused.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>

---

diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 1d3e5fc..303f105 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -175,7 +175,7 @@ static int svc_sendto(struct svc_rqst *rqstp, struct xdr_buf *xdr)
 	size_t		base = xdr->page_base;
 	unsigned int	pglen = xdr->page_len;
 	unsigned int	flags = MSG_MORE;
-	char		buf[RPC_MAX_ADDRBUFLEN];
+	char		buf[RPC_MAX_ADDRBUFLEN] __maybe_unused;
 
 	slen = xdr->len;
 
@@ -716,7 +716,7 @@ static struct svc_xprt *svc_tcp_accept(struct svc_xprt *xprt)
 	struct socket	*newsock;
 	struct svc_sock	*newsvsk;
 	int		err, slen;
-	char		buf[RPC_MAX_ADDRBUFLEN];
+	char		buf[RPC_MAX_ADDRBUFLEN] __maybe_unused;
 
 	dprintk("svc: tcp_accept %p sock %p\n", svsk, sock);
 	if (!sock)
@@ -1206,7 +1206,7 @@ static struct svc_xprt *svc_create_socket(struct svc_serv *serv,
 	struct socket	*sock;
 	int		error;
 	int		type;
-	char		buf[RPC_MAX_ADDRBUFLEN];
+	char		buf[RPC_MAX_ADDRBUFLEN] __maybe_unused;
 	struct sockaddr_storage addr;
 	struct sockaddr *newsin = (struct sockaddr *)&addr;
 	int		newlen;

Re: [PATCH] SUNRPC: Mark buffer used for debug printks with __maybe_unused

[Joe Perches]

On Wed, 2008-02-20 at 17:02 +0300, Pavel Emelyanov wrote:
> There are tree places, which declare the char buf[...] on the stack
> to push it later into dprintk(). Since the dprintk sometimes (if the 
> CONFIG_SYSCTL=n) becomes an empty do { } while (0) stub, these buffers
> cause gcc to produce appropriate warnings.

What about the uses in fs?

fs/lockd/svc.c:         char buf[RPC_MAX_ADDRBUFLEN];
fs/lockd/svc4proc.c:            char buf[RPC_MAX_ADDRBUFLEN];
fs/lockd/svcproc.c:             char buf[RPC_MAX_ADDRBUFLEN];
fs/nfs/callback.c:      char buf[RPC_MAX_ADDRBUFLEN];
fs/nfsd/nfsfh.c:                char buf[RPC_MAX_ADDRBUFLEN];
fs/nfsd/nfsproc.c:              char buf[RPC_MAX_ADDRBUFLEN];

Perhaps there should be a DECLARE_RPC_BUF(buf) macro?

#define DECLARE_RPC_BUF(var) char var[MAC_BUF_SIZE] __maybe_unused

Re: [PATCH] SUNRPC: Mark buffer used for debug printks with __maybe_unused

[Joe Perches]

On Wed, 2008-02-20 at 07:29 -0800, Joe Perches wrote:
> fs/nfsd/nfsproc.c:              char buf[RPC_MAX_ADDRBUFLEN];
> Perhaps there should be a DECLARE_RPC_BUF(buf) macro?
> #define DECLARE_RPC_BUF(var) char var[MAC_BUF_SIZE] __maybe_unused

Make that:

#define DECLARE_RPC_BUF(var) char var[RPC_MAX_ADDRBUFLEN] __maybe_unused

Re: [PATCH] SUNRPC: Mark buffer used for debug printks with __maybe_unused

[Patrick McHardy]

Joe Perches wrote:
> On Wed, 2008-02-20 at 07:29 -0800, Joe Perches wrote:
>   
>> fs/nfsd/nfsproc.c:              char buf[RPC_MAX_ADDRBUFLEN];
>> Perhaps there should be a DECLARE_RPC_BUF(buf) macro?
>> #define DECLARE_RPC_BUF(var) char var[MAC_BUF_SIZE] __maybe_unused
>>     
>
> Make that:
>
> #define DECLARE_RPC_BUF(var) char var[RPC_MAX_ADDRBUFLEN] __maybe_unuse

Alternatively change the dprintk macro to behave similar like
pr_debug() and mark things like svc_print_addr() __pure, which
has the advantage that is still performs format checking even
if debugging is disabled.

Re: [PATCH] SUNRPC: Mark buffer used for debug printks with __maybe_unused

[Pavel Emelyanov]

Joe Perches wrote:
> On Wed, 2008-02-20 at 17:02 +0300, Pavel Emelyanov wrote:
>> There are tree places, which declare the char buf[...] on the stack
>> to push it later into dprintk(). Since the dprintk sometimes (if the 
>> CONFIG_SYSCTL=n) becomes an empty do { } while (0) stub, these buffers
>> cause gcc to produce appropriate warnings.
> 
> What about the uses in fs?
> 
> fs/lockd/svc.c:         char buf[RPC_MAX_ADDRBUFLEN];
> fs/lockd/svc4proc.c:            char buf[RPC_MAX_ADDRBUFLEN];
> fs/lockd/svcproc.c:             char buf[RPC_MAX_ADDRBUFLEN];
> fs/nfs/callback.c:      char buf[RPC_MAX_ADDRBUFLEN];
> fs/nfsd/nfsfh.c:                char buf[RPC_MAX_ADDRBUFLEN];
> fs/nfsd/nfsproc.c:              char buf[RPC_MAX_ADDRBUFLEN];
> 
> Perhaps there should be a DECLARE_RPC_BUF(buf) macro?
> 
> #define DECLARE_RPC_BUF(var) char var[MAC_BUF_SIZE] __maybe_unused

Sigh... Why is that better than a strait declaration with attribute?

> 
> 

Re: [PATCH] SUNRPC: Mark buffer used for debug printks with __maybe_unused

[Joe Perches]

> Sigh... Why is that better than a strait declaration with attribute?

If at some point there's a gcc'ism to remove a maybe_unused
variable from the stack declaration, you only have change
the macro.

cheers, Joe

Re: [PATCH] SUNRPC: Mark buffer used for debug printks with __maybe_unused

[Pavel Emelyanov]

Patrick McHardy wrote:
> Joe Perches wrote:
>> On Wed, 2008-02-20 at 07:29 -0800, Joe Perches wrote:
>>   
>>> fs/nfsd/nfsproc.c:              char buf[RPC_MAX_ADDRBUFLEN];
>>> Perhaps there should be a DECLARE_RPC_BUF(buf) macro?
>>> #define DECLARE_RPC_BUF(var) char var[MAC_BUF_SIZE] __maybe_unused
>>>     
>> Make that:
>>
>> #define DECLARE_RPC_BUF(var) char var[RPC_MAX_ADDRBUFLEN] __maybe_unuse

OK, I'll send the patch in a moment.

> Alternatively change the dprintk macro to behave similar like

This is too heavy. The problem is that some arguments passed to this
function exist only under appropriate ifdefs, so having a static
inline there will produce a warning :(

> pr_debug() and mark things like svc_print_addr() __pure, which
> has the advantage that is still performs format checking even
> if debugging is disabled.

Taking my above statement into account, this becomes useless, since
svc_print_addr() is used inside those "empty" macros and are complied
out automatically.

> 

Re: [PATCH] SUNRPC: Mark buffer used for debug printks with __maybe_unused

[Joe Perches]

On Wed, 2008-02-20 at 16:35 +0100, Patrick McHardy wrote:
> Alternatively change the dprintk macro to behave similar like
> pr_debug() and mark things like svc_print_addr() __pure, which
> has the advantage that is still performs format checking even
> if debugging is disabled.

I think it's better to change the dprintk style macros
to what Philip Craig suggested.

http://marc.info/?l=linux-wireless&m=120338413108120&w=2

This makes clear to the compiler that the called function is not
going to be used so it can be optimized away, keeps any argument
verification in place, and doesn't require __pure attributes on
arbitrary functions that may be called during the dprintk

#ifdef DEBUG
#define some_print_wrapper(fmt, arg...) \
        do { if (0) printk(KERN_DEBUG fmt, ##arg); } while (0)
#else
#define some_print_wrapper(fmt, arg...) \
	printk(KERN_DEBUG fmt, ##arg)
#endif

Re: [PATCH] SUNRPC: Mark buffer used for debug printks with __maybe_unused

[Trond Myklebust]

On Wed, 2008-02-20 at 19:27 +0300, Pavel Emelyanov wrote:
> Patrick McHardy wrote:
> > Joe Perches wrote:
> >> On Wed, 2008-02-20 at 07:29 -0800, Joe Perches wrote:
> >>   
> >>> fs/nfsd/nfsproc.c:              char buf[RPC_MAX_ADDRBUFLEN];
> >>> Perhaps there should be a DECLARE_RPC_BUF(buf) macro?
> >>> #define DECLARE_RPC_BUF(var) char var[MAC_BUF_SIZE] __maybe_unused
> >>>     
> >> Make that:
> >>
> >> #define DECLARE_RPC_BUF(var) char var[RPC_MAX_ADDRBUFLEN] __maybe_unuse
> 
> OK, I'll send the patch in a moment.

1) Please always Cc linux-nfs@vger.kernel.org when changing the sunrpc
code

2) Please don't use the name RPC_BUF. These are debugging strings, not
buffers. Something like DECLARE_RPC_DEBUG_STR() would be more
appropriate.

3) If you're going to use a macro, why not just use the existing
RPC_IFDEBUG()?

Trond

Re: [PATCH] SUNRPC: Mark buffer used for debug printks with __maybe_unused

[Trond Myklebust]

On Wed, 2008-02-20 at 08:36 -0800, Joe Perches wrote:
> On Wed, 2008-02-20 at 16:35 +0100, Patrick McHardy wrote:
> > Alternatively change the dprintk macro to behave similar like
> > pr_debug() and mark things like svc_print_addr() __pure, which
> > has the advantage that is still performs format checking even
> > if debugging is disabled.
> 
> I think it's better to change the dprintk style macros
> to what Philip Craig suggested.
> 
> http://marc.info/?l=linux-wireless&m=120338413108120&w=2
> 
> This makes clear to the compiler that the called function is not
> going to be used so it can be optimized away, keeps any argument
> verification in place, and doesn't require __pure attributes on
> arbitrary functions that may be called during the dprintk
> 
> #ifdef DEBUG
> #define some_print_wrapper(fmt, arg...) \
>         do { if (0) printk(KERN_DEBUG fmt, ##arg); } while (0)
> #else
> #define some_print_wrapper(fmt, arg...) \
> 	printk(KERN_DEBUG fmt, ##arg)
> #endif

Have you actually read include/linux/sunrpc/debug.h?

Trond

Re: [PATCH] SUNRPC: Mark buffer used for debug printks with __maybe_unused

[Joe Perches]

On Wed, 2008-02-20 at 12:02 -0500, Trond Myklebust wrote:
> > #ifdef DEBUG
> > #define some_print_wrapper(fmt, arg...) \
> >         do { if (0) printk(KERN_DEBUG fmt, ##arg); } while (0)
> > #else
> > #define some_print_wrapper(fmt, arg...) \
> > 	printk(KERN_DEBUG fmt, ##arg)
> > #endif
> Have you actually read include/linux/sunrpc/debug.h?

Yes, I have.

What's there:

#define dfprintk(fac, args...) do ; while (0)

vs what's suggested:

#define dfprintk(fac, args...) \
	do  { if (0) printk(##args); } while (0);

No argument verification is done to args

There has been code that fails to compile with -DDEBUG when
the code use two different #ifdef DEBUG #else macros.
I think some of the USB code was reworked because of that.

The extra verification is just a guard against bad arguments
when compiled normally.  It's similar to what's done in kernel.h
pr_debug without the __attribute__((format(printf,x,y))) so
that calls made as arguments to functions aren't called
unnecessarily.

cheers, Joe

Re: [PATCH] SUNRPC: Mark buffer used for debug printks with __maybe_unused

[Trond Myklebust]

On Wed, 2008-02-20 at 09:23 -0800, Joe Perches wrote:
> On Wed, 2008-02-20 at 12:02 -0500, Trond Myklebust wrote:
> > > #ifdef DEBUG
> > > #define some_print_wrapper(fmt, arg...) \
> > >         do { if (0) printk(KERN_DEBUG fmt, ##arg); } while (0)
> > > #else
> > > #define some_print_wrapper(fmt, arg...) \
> > > 	printk(KERN_DEBUG fmt, ##arg)
> > > #endif
> > Have you actually read include/linux/sunrpc/debug.h?
> 
> Yes, I have.
> 
> What's there:
> 
> #define dfprintk(fac, args...) do ; while (0)
> 
> vs what's suggested:
> 
> #define dfprintk(fac, args...) \
> 	do  { if (0) printk(##args); } while (0);
> 
> No argument verification is done to args
> 
> There has been code that fails to compile with -DDEBUG when
> the code use two different #ifdef DEBUG #else macros.
> I think some of the USB code was reworked because of that.
> 
> The extra verification is just a guard against bad arguments
> when compiled normally.  It's similar to what's done in kernel.h
> pr_debug without the __attribute__((format(printf,x,y))) so
> that calls made as arguments to functions aren't called
> unnecessarily.

RPC_DEBUG is on by default if SYSCTL is compiled in, so it is not as if
we're having any trouble typechecking the arguments. In fact, most of
the major distros also tend to enable RPC_DEBUG, since it does come in
handy when their customers report rpc/nfs problems.

As you can see, there is already a macro RPC_IFDEBUG to deal with simple
code that depends on whether or not RPC_DEBUG is set. Using that will
make it obvious to a reader exactly when the declaration will be
optimised away, and why, without compromising gcc's ability to warn us
if it were to be unused due to some future code change.