Re: [RFC PATCH v2 1/3] tracing: Show real address for trace event arguments

[Steven Rostedt]

On Thu, 15 Oct 2020 18:00:08 +0900
Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:

> +#define STATIC_FMT_BUF_SIZE	128
> +static char static_fmt_buf[STATIC_FMT_BUF_SIZE];
> +
> +const char *trace_event_format(struct trace_iterator *iter, const char *fmt)
> +{
> +	const char *p, *new_fmt;
> +	char *q;
> +
> +	if (WARN_ON_ONCE(!fmt))
> +		return fmt;
> +retry:
> +	p = fmt;
> +	new_fmt = q = iter->fmt;
> +	while (*p) {
> +		if (unlikely(q - new_fmt + 3 > iter->fmt_size)) {
> +			/* expand format buffer if needed */
> +			if (iter->fmt == static_fmt_buf)
> +				return fmt;
> +
> +			iter->fmt_size = iter->fmt_size ? iter->fmt_size * 2
> +							: STATIC_FMT_BUF_SIZE;

Doubling may be too much, as fmt strings are not going to grow
exponentially.

> +			kfree(iter->fmt);
> +			iter->fmt = kmalloc(iter->fmt_size, GFP_KERNEL);

Need to test the return value of kmalloc. But I would use realloc instead,
and continue:

			char *tmp_fmt;
			unsigned int tmp_size;

			tmp_size = iter->fmt_size + STATIC_FMT_BUF_SIZE;

			tmp_fmt = krealloc(iter->fmt, tmp_size, GFP_KERNEL);
			if (!tmp_fmt)
				return fmt;

			q += tmp_fmt - new_fmt;
			new_fmt = iter->fmt = tmp_fmt;
			iter->fmt_size = tmp_size;

> +			goto retry;

Then you don't even need the retry, and just continue processing.

Also, if it fails to allocate, you still have the iter->fmt and fmt_size
intact and correct.

-- Steve

> +		}
> +		*q++ = *p++;
> +		/* Replace %p with %px */
> +		if (p[-1] == '%') {
> +			if (p[0] == '%') {
> +				*q++ = *p++;
> +			} else if (p[0] == 'p' && !isalnum(p[1])) {
> +				*q++ = *p++;
> +				*q++ = 'x';
> +			}
> +		}
> +	}
> +	*q = '\0';
> +
> +	return new_fmt;
> +}
> +