Skip to contentSkip to conversation

SUNRPC: Mark buffer used for debug printks with __maybe_unused

PATCH In review Pavel Emelyanov 4 participants 12 messages Feb 20, 2008

Subsystems (4)
Networking Network drivers NFS client NFS server
lore
Share & tools
lore

Pavel Emelyanov

author
#0 lore raw

PATCH 1/1 SUNRPC: Mark buffer used for debug printks with __maybe_unused

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 <[email protected]>

---

net/sunrpc/svcsock.c +3 -3
index 1d3e5fc..303f105 100644
@@ -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;
#1 lore raw
Quoted 4 lines · 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.

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

#2 lore raw
Quoted 3 lines · Joe Perches

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

#3 lore raw
Quoted 10 lines · Joe Perches
Quoted 4 lines · Joe Perches

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.

author
#4 lore raw
Quoted 10 lines · Patrick McHardy
Quoted 9 lines · Joe Perches
Quoted 4 lines · Joe Perches

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.

Quoted 1 line

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 :(

Quoted 3 lines

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.

Quoted 1 line
#5 lore raw
Quoted 8 lines · Pavel Emelyanov
Quoted 7 lines · Patrick McHardy
Quoted 6 lines · Joe Perches
Quoted 4 lines · Joe Perches

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

Quoted 5 lines
Quoted 3 lines
Quoted 3 lines

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 [email protected] 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

#6 lore raw
In reply to Patrick McHardy
Quoted 4 lines · Patrick McHardy

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

#7 lore raw
Quoted 16 lines · Joe Perches
Quoted 4 lines · Patrick McHardy

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

Quoted 7 lines

#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

#8 lore raw
Quoted 8 lines · Trond Myklebust
Quoted 7 lines

#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

#9 lore raw
Quoted 27 lines · Joe Perches
Quoted 8 lines · Trond Myklebust
Quoted 7 lines

#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.

Quoted 5 lines

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.

author
#10 lore raw
In reply to Joe Perches
Quoted 15 lines · Joe Perches
Quoted 4 lines · 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.

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];
Quoted 3 lines

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?

Quoted 2 lines
#11 lore raw
Quoted 1 line

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