Message-ID: <1363453326.25967.105.camel@gandalf.local.home>
Subject: Re: [RFC PATCH] seq_file: Use seq_puts when seq_printf has only a
 format with no args
From: Steven Rostedt <rostedt@goodmis.org>
To: Joe Perches <joe@perches.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
        Andrew Morton <akpm@linux-foundation.org>,
        LKML <linux-kernel@vger.kernel.org>
Date: Sat, 16 Mar 2013 13:02:06 -0400
In-Reply-To: <1363450540.2023.30.camel@joe-AO722>
References: <1363441844.2023.17.camel@joe-AO722>
	 <1363449458.25967.76.camel@gandalf.local.home>
	 <1363450540.2023.30.camel@joe-AO722>
Content-Type: text/plain; charset="ISO-8859-15"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3828
Lines: 136

On Sat, 2013-03-16 at 09:15 -0700, Joe Perches wrote:

> > > +int (seq_printf)(struct seq_file *m, const char *f, ...)
> > 
> > That's rather ugly. Why not just #undef seq_printf before defining it?
> 
> The whole thing is ugly, nasty and hackish.
> I kinda like it.
> 
> But I don't like unnecessary undefs.
> The preprocessor doesn't expand (funcname).

Either way, as long as it's commented.

> 
> > Anyway, not making va_args a whacky name is dangerous. This is why I add
> > those crazy underscores. If someone does:
> > 
> > 	var = 1;
> > 	va_args[] = "abc";
> > 	seq_printf(m, "%d %s", var, va_args);
> 
> The same could be true of fmt and it's
> used in lots of macros no?

No, not at all. Try it. The difference is that 'fmt' is the macro arg
and gets replaced by the pre-processor, where as va_args is what gets
placed into the C code by the C pre-processor. Any variables that's not
a macro argument MUST BE UNIQUE!

> 
> > What will be printed is:
> > 
> > 	1 var, va_args
> > 
> > That will be very confusing to people.
> 
> And so be fixed very quickly.

By changing the caller, it's a bug in the implementation of the macro,
not the user of the macro.

> 
> > > +	if (sizeof(va_args) > 1)				\
> > > +		seq_printf(seq, fmt, ##__VA_ARGS__);		\
> > > +	else							\
> > > +		seq_puts(seq, fmt);				\
> > > +} while (0)
> > 
> > BTW, you need to return a value.
> 
> Oh, yeah, thanks.
> 
> >  #define seq_printf(seq, fmt, ...)				\
> > -do {								\
> > +({								\
> >  	char va_args[] = __stringify(__VA_ARGS__);		\
> > +	int _____ret;						\
> >  	if (sizeof(va_args) > 1)				\
> > -		seq_printf(seq, fmt, ##__VA_ARGS__);		\
> > +		_____ret = seq_printf(seq, fmt, ##__VA_ARGS__); \
> >  	else							\
> > -		seq_puts(seq, fmt);				\
> > -} while (0)
> > +		_____ret = seq_puts(seq, fmt);			\
> > +	_____ret;						\
> > +})
> 
> It's certainly better as a statement expression,
> but I think the underscores are really ugly and
> not necessary as ret is locally scoped.

Let me show you the issue:

#define seq_printf(seq, fmt, ...)			\
({							\
	char va_args[] = __stringify(__VA_ARGS__);		\
	int ret;					\
	if (sizeof(va_args) > 1)			\
		ret = seq_printf(seq, fmt, ##__VA_ARGS__);	\
	else
		ret = seq_puts(seq, fmt);		\
	ret;						\
})

Now lets look at the usage of in the code:

	ret = 1;
	va_args = 5;
	fmt = "hello world\n";
	seq_print(m, "my fmt=%s args=%d ret=%d\n", fmt, va_args, ret);

Notice that here we have va_args as a integer.

Here's what cpp does to it (adding newlines to make it readable):

	ret = 1;
	va_args = 5;
	fmt = "hello world";
	({
		char va_args[] = "fmt, va_args, ret";
		int ret;
		if (sizeof(va_args) > 1)
			ret = seq_printf(seq, "my fmt=%s args=%d ret=%d\n", fmt, va_args, ret);
		else
			ret = seq_puts(seq, "my fmt=%s args=%d ret=%d\n");
		ret;
	})

A couple of things here. One, you'll get a gcc warning about ret being
used uninitialized. That should confuse the hell out of the developer.

Then you'll also get a warning about %d expecting a number but the
argument is a string. Another damn confusing thing for developers to
see.

Let me stress it one more time... Any variable names added by a macro,
that's not replaced by the parameters of the macro MUST BE UNIQUE!!!!
Otherwise you will get very difficult hard to debug bugs.

I'm also thinking, as macros may include macros, we may need to define a
macro variable name space policy. Something like:

	char __seq_printf__va_args[] = __stringify(__VA_ARGS__);
	int __seq_printf__ret;

It may make the macro ugly, but it helps keep conflicts in name spacing.

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/