diff mbox series

Avoid using the heap for small strings with tracef/tracelog

Message ID 20210519165648.792939-1-nolange79@gmail.com
State New
Headers show
Series Avoid using the heap for small strings with tracef/tracelog | expand

Commit Message

Norbert Lange May 19, 2021, 4:56 p.m. UTC
Try to use vsnprintf with a 512 Byte buffer on the Stack,
if that fails allocate a larger one.

Signed-off-by: Norbert Lange <nolange79 at gmail.com>
---
 liblttng-ust/tracef.c   | 13 ++++++++++---
 liblttng-ust/tracelog.c | 12 +++++++++---
 2 files changed, 19 insertions(+), 6 deletions(-)

Comments

Mathieu Desnoyers May 19, 2021, 7 p.m. UTC | #1
----- On May 19, 2021, at 12:56 PM, lttng-dev lttng-dev at lists.lttng.org wrote:

> Try to use vsnprintf with a 512 Byte buffer on the Stack,
> if that fails allocate a larger one.

I agree with the approach.

Can we move the hardcoded "512" to a #define within src/common/tracer.h ?

Thanks,

Mathieu

> 
> Signed-off-by: Norbert Lange <nolange79 at gmail.com>
> ---
> liblttng-ust/tracef.c   | 13 ++++++++++---
> liblttng-ust/tracelog.c | 12 +++++++++---
> 2 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/liblttng-ust/tracef.c b/liblttng-ust/tracef.c
> index ea98e43e..18c29e25 100644
> --- a/liblttng-ust/tracef.c
> +++ b/liblttng-ust/tracef.c
> @@ -32,17 +32,24 @@
> void _lttng_ust_tracef(const char *fmt, ...)
> {
> 	va_list ap;
> -	char *msg;
> +	char local_buf[512];
> +	char *msg = local_buf;
> 	int len;
> 
> 	va_start(ap, fmt);
> -	len = vasprintf(&msg, fmt, ap);
> +	len = vsnprintf(local_buf, sizeof(local_buf), fmt, ap);
> +	if (len >= sizeof(local_buf)) {
> +		msg = (char *)malloc(len + 1);
> +		len = msg ? vsnprintf(msg, len + 1, fmt, ap) : -1;
> +	}
> +
> 	/* len does not include the final \0 */
> 	if (len < 0)
> 		goto end;
> 	__tracepoint_cb_lttng_ust_tracef___event(msg, len,
> 		LTTNG_UST_CALLER_IP());
> -	free(msg);
> end:
> +	if (msg != local_buf)
> +		free(msg);
> 	va_end(ap);
> }
> diff --git a/liblttng-ust/tracelog.c b/liblttng-ust/tracelog.c
> index 65fc87ed..a5d110fa 100644
> --- a/liblttng-ust/tracelog.c
> +++ b/liblttng-ust/tracelog.c
> @@ -35,19 +35,25 @@
> 			const char *fmt, ...) \
> 	{ \
> 		va_list ap; \
> -		char *msg; \
> +		char local_buf[512]; \
> +		char *msg = local_buf; \
> 		int len; \
> 		\
> 		va_start(ap, fmt); \
> -		len = vasprintf(&msg, fmt, ap); \
> +		len = vsnprintf(local_buf, sizeof(local_buf), fmt, ap); \
> +		if (len >= sizeof(local_buf)) { \
> +			msg = (char *)malloc(len + 1); \
> +			len = msg ? vsnprintf(msg, len + 1, fmt, ap) : -1; \
> +		} \
> 		/* len does not include the final \0 */ \
> 		if (len < 0) \
> 			goto end; \
> 		__tracepoint_cb_lttng_ust_tracelog___##level(file, \
> 			line, func, msg, len, \
> 			LTTNG_UST_CALLER_IP()); \
> -		free(msg); \
> 	end: \
> +		if (msg != local_buf) \
> +			free(msg); \
> 		va_end(ap); \
> 	}
> 
> --
> 2.30.2
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
Norbert Lange May 20, 2021, 12:20 p.m. UTC | #2
Am Mi., 19. Mai 2021 um 21:00 Uhr schrieb Mathieu Desnoyers
<mathieu.desnoyers at efficios.com>:
>
> ----- On May 19, 2021, at 12:56 PM, lttng-dev lttng-dev at lists.lttng.org wrote:
>
> > Try to use vsnprintf with a 512 Byte buffer on the Stack,
> > if that fails allocate a larger one.
>
> I agree with the approach.
>
> Can we move the hardcoded "512" to a #define within src/common/tracer.h ?
>
> Thanks,
>
> Mathieu
>
> >
> > Signed-off-by: Norbert Lange <nolange79 at gmail.com>
> > ---
> > liblttng-ust/tracef.c   | 13 ++++++++++---
> > liblttng-ust/tracelog.c | 12 +++++++++---
> > 2 files changed, 19 insertions(+), 6 deletions(-)
> >
> > diff --git a/liblttng-ust/tracef.c b/liblttng-ust/tracef.c
> > index ea98e43e..18c29e25 100644
> > --- a/liblttng-ust/tracef.c
> > +++ b/liblttng-ust/tracef.c
> > @@ -32,17 +32,24 @@
> > void _lttng_ust_tracef(const char *fmt, ...)
> > {
> >       va_list ap;
> > -     char *msg;
> > +     char local_buf[512];
> > +     char *msg = local_buf;
> >       int len;
> >
> >       va_start(ap, fmt);
> > -     len = vasprintf(&msg, fmt, ap);
> > +     len = vsnprintf(local_buf, sizeof(local_buf), fmt, ap);
> > +     if (len >= sizeof(local_buf)) {
> > +             msg = (char *)malloc(len + 1);
> > +             len = msg ? vsnprintf(msg, len + 1, fmt, ap) : -1;
> > +     }
> > +
> >       /* len does not include the final \0 */
> >       if (len < 0)
> >               goto end;
> >       __tracepoint_cb_lttng_ust_tracef___event(msg, len,
> >               LTTNG_UST_CALLER_IP());
> > -     free(msg);
> > end:
> > +     if (msg != local_buf)
> > +             free(msg);
> >       va_end(ap);
> > }
> > diff --git a/liblttng-ust/tracelog.c b/liblttng-ust/tracelog.c
> > index 65fc87ed..a5d110fa 100644
> > --- a/liblttng-ust/tracelog.c
> > +++ b/liblttng-ust/tracelog.c
> > @@ -35,19 +35,25 @@
> >                       const char *fmt, ...) \
> >       { \
> >               va_list ap; \
> > -             char *msg; \
> > +             char local_buf[512]; \
> > +             char *msg = local_buf; \
> >               int len; \
> >               \
> >               va_start(ap, fmt); \
> > -             len = vasprintf(&msg, fmt, ap); \
> > +             len = vsnprintf(local_buf, sizeof(local_buf), fmt, ap); \
> > +             if (len >= sizeof(local_buf)) { \
> > +                     msg = (char *)malloc(len + 1); \
> > +                     len = msg ? vsnprintf(msg, len + 1, fmt, ap) : -1; \
> > +             } \
> >               /* len does not include the final \0 */ \
> >               if (len < 0) \
> >                       goto end; \
> >               __tracepoint_cb_lttng_ust_tracelog___##level(file, \
> >                       line, func, msg, len, \
> >                       LTTNG_UST_CALLER_IP()); \
> > -             free(msg); \
> >       end: \
> > +             if (msg != local_buf) \
> > +                     free(msg); \
> >               va_end(ap); \
> >       }
> >
> > --
> > 2.30.2
> >
> > _______________________________________________
> > lttng-dev mailing list
> > lttng-dev at lists.lttng.org
> > https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com

This patch was intended for 2.12, no src/common/tracer.h there.
Also this patch is brocken, as a va_list cant be iterated twice.
Mathieu Desnoyers May 20, 2021, 1:33 p.m. UTC | #3
----- On May 20, 2021, at 8:20 AM, Norbert Lange nolange79 at gmail.com wrote:

> This patch was intended for 2.12, no src/common/tracer.h there.

You should post feature patches against the master branch. Or if you just
want to send a RFC, and you work on an older branch, please state that the
patch is RFC and which branch the patch is against, but that work won't
be merged unless it's rebased against master.

> Also this patch is brocken, as a va_list cant be iterated twice.

Right. But you could easily do:

va_start() [ first use ] va_end()
then
va_start() [ second use ] va_end(), right ?

Thanks,

Mathieu
Norbert Lange May 20, 2021, 1:44 p.m. UTC | #4
Am Do., 20. Mai 2021 um 15:33 Uhr schrieb Mathieu Desnoyers
<mathieu.desnoyers at efficios.com>:
>
> ----- On May 20, 2021, at 8:20 AM, Norbert Lange nolange79 at gmail.com wrote:
>
> > This patch was intended for 2.12, no src/common/tracer.h there.
>
> You should post feature patches against the master branch. Or if you just
> want to send a RFC, and you work on an older branch, please state that the
> patch is RFC and which branch the patch is against, but that work won't
> be merged unless it's rebased against master.

Yeah, I read the contributing guide since then, sorry =)

> > Also this patch is brocken, as a va_list cant be iterated twice.
>
> Right. But you could easily do:
>
> va_start() [ first use ] va_end()
> then
> va_start() [ second use ] va_end(), right ?

New patch vs master does this.

Norbert
diff mbox series

Patch

diff --git a/liblttng-ust/tracef.c b/liblttng-ust/tracef.c
index ea98e43e..18c29e25 100644
--- a/liblttng-ust/tracef.c
+++ b/liblttng-ust/tracef.c
@@ -32,17 +32,24 @@ 
 void _lttng_ust_tracef(const char *fmt, ...)
 {
 	va_list ap;
-	char *msg;
+	char local_buf[512];
+	char *msg = local_buf;
 	int len;
 
 	va_start(ap, fmt);
-	len = vasprintf(&msg, fmt, ap);
+	len = vsnprintf(local_buf, sizeof(local_buf), fmt, ap);
+	if (len >= sizeof(local_buf)) {
+		msg = (char *)malloc(len + 1);
+		len = msg ? vsnprintf(msg, len + 1, fmt, ap) : -1;
+	}
+
 	/* len does not include the final \0 */
 	if (len < 0)
 		goto end;
 	__tracepoint_cb_lttng_ust_tracef___event(msg, len,
 		LTTNG_UST_CALLER_IP());
-	free(msg);
 end:
+	if (msg != local_buf)
+		free(msg);
 	va_end(ap);
 }
diff --git a/liblttng-ust/tracelog.c b/liblttng-ust/tracelog.c
index 65fc87ed..a5d110fa 100644
--- a/liblttng-ust/tracelog.c
+++ b/liblttng-ust/tracelog.c
@@ -35,19 +35,25 @@ 
 			const char *fmt, ...) \
 	{ \
 		va_list ap; \
-		char *msg; \
+		char local_buf[512]; \
+		char *msg = local_buf; \
 		int len; \
 		\
 		va_start(ap, fmt); \
-		len = vasprintf(&msg, fmt, ap); \
+		len = vsnprintf(local_buf, sizeof(local_buf), fmt, ap); \
+		if (len >= sizeof(local_buf)) { \
+			msg = (char *)malloc(len + 1); \
+			len = msg ? vsnprintf(msg, len + 1, fmt, ap) : -1; \
+		} \
 		/* len does not include the final \0 */ \
 		if (len < 0) \
 			goto end; \
 		__tracepoint_cb_lttng_ust_tracelog___##level(file, \
 			line, func, msg, len, \
 			LTTNG_UST_CALLER_IP()); \
-		free(msg); \
 	end: \
+		if (msg != local_buf) \
+			free(msg); \
 		va_end(ap); \
 	}