Re: pr_cat() + CATSTR(name, size)?

[Joe Perches]

On Wed, 2012-07-11 at 12:33 +0200, Kay Sievers wrote:
> Hey Joe,

Hi Kay.

> what do you think this?
> 
> It would make composing continuation lines at the caller side entirely
> race-free, and it might fit into the usual pattern.

Maybe.  comments below...

> The more interesting thing, this would allow us to completely race-free
> use the dev_printk() calls with continuation content, which we should
> avoid otherwise for integrity reasons.
> 
> The patch below is just hacked it into the printk.c file to illustrate
> the idea. It prints:
>   [    0.000000] Kernel command line: root=/dev/sda2 ...
>   [    0.000000] 12 34 56 78
>   [    0.000000] PID hash table entries: 2048 (order: 2, 16384 bytes)
> 
>   5,96,0;Kernel command line: root=/dev/sda2 ...
>   4,97,0;12 34 56 78
>   6,98,0;PID hash table entries: 2048 (order: 2, 16384 bytes)
> 
> Thanks,
> Kay
> 
> 
> diff --git a/kernel/printk.c b/kernel/printk.c
> index dba1821..1fd00b0 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -48,6 +48,32 @@
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/printk.h>
>  
> +#define CATSTR(name, max)		\
> +	char name[max];			\
> +	size_t _##name_len = 0;		\
> +	size_t _##name_max = max;
> +
> +#define pr_cat(name, fmt, ...)		\
> +	_catstr(name, &_##name_len, _##name_max, fmt, ##__VA_ARGS__)
> +
> +ssize_t _catstr(char *s, size_t *len, size_t size, const char *fmt, ...)
> +{
> +	va_list args;
> +	size_t l = *len;
> +	size_t r;
> +
> +	va_start(args, fmt);
> +	r = vsnprintf(s + l, size - l, fmt, args);
> +	va_end(args);
> +
> +	if (r >= size - l)
> +		return -EINVAL;
> +
> +	*len += r;
> +	s[*len] = '\0';
> +	return r;
> +}
> +
>  /*
>   * Architectures can override it:
>   */
> @@ -668,6 +694,12 @@ void __init setup_log_buf(int early)
>  	char *new_log_buf;
>  	int free;
>  
> +	CATSTR(line, 80);
> +	pr_cat(line, "%i ", 12);
> +	pr_cat(line, "%i ", 34);
> +	pr_cat(line, "%i ", 56);
> +	pr_warn("%s%i\n", line, 78);
> +
>  	if (!new_log_buf_len)
>  		return;

Interesting idea, perhaps workable, but it has
a few defects I can see.

It works for most uses, but it doesn't work for
when there are multiple function sites like

void dump_info(struct foo *bar)
{
	if (bar->baz)
		pr_cont("baz...");
}

---

	pr_info("Some initiator: ")
	dump_info(&descriptor);

Another negative is that is uses a local stack
variable for the entire line which increases
stack pressure.

It also fails to immediately output after some
defect unlike your change to output directly to
console.

It would require all sites with continuation lines
be modified.  Because it requires in-situ code
modifications, I'd prefer a cookie based approach.

I think it's more flexible, allows the cookie to be
passed into extending functions and doesn't demand
(much) extra stack.

Something like:
https://lkml.org/lkml/2012/4/3/231
https://lkml.org/lkml/2011/11/14/349

cheers, Joe

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