diff mbox series

[v1] libtraceevent: Avoid a simple asprintf case

Message ID 20240509051317.1913001-1-irogers@google.com (mailing list archive)
State Accepted
Commit 8802f0f8761a9bdaf8c15223ce05389b7397ffbb
Headers show
Series [v1] libtraceevent: Avoid a simple asprintf case | expand

Commit Message

Ian Rogers May 9, 2024, 5:13 a.m. UTC
Avoid an asprintf for the single character TEP_EVENT_NEWLINE and
TEP_EVENT_DELIM case.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 src/event-parse.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Steven Rostedt May 16, 2024, 10:02 p.m. UTC | #1
On Wed,  8 May 2024 22:13:17 -0700
Ian Rogers <irogers@google.com> wrote:

> Avoid an asprintf for the single character TEP_EVENT_NEWLINE and
> TEP_EVENT_DELIM case.

This states what the patch does, but omits why. What's wrong with
asprintf() here?

-- Steve
Ian Rogers May 16, 2024, 10:09 p.m. UTC | #2
On Thu, May 16, 2024 at 3:03 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed,  8 May 2024 22:13:17 -0700
> Ian Rogers <irogers@google.com> wrote:
>
> > Avoid an asprintf for the single character TEP_EVENT_NEWLINE and
> > TEP_EVENT_DELIM case.
>
> This states what the patch does, but omits why. What's wrong with
> asprintf() here?

Nothing except overhead. I was tracking down an asprintf issue [1],
asprintf breaking the sanitizer stack traces on my debian based linux,
and using asprintf for the sake of copying a character was creating a
huge amount of noise for me. 2 lines-of-code extra means this change
is by one measure more complex but it removes special knowledge of
asprintf return values and the meaning of %c, so on the other hand it
is less complex. I won't be offended if it's not accepted.

Thanks,
Ian

[1] https://lore.kernel.org/lkml/20240509052015.1914670-1-irogers@google.com/

> -- Steve
Steven Rostedt May 16, 2024, 11:51 p.m. UTC | #3
On Thu, 16 May 2024 15:09:40 -0700
Ian Rogers <irogers@google.com> wrote:

> > This states what the patch does, but omits why. What's wrong with
> > asprintf() here?  
> 
> Nothing except overhead. I was tracking down an asprintf issue [1],
> asprintf breaking the sanitizer stack traces on my debian based linux,
> and using asprintf for the sake of copying a character was creating a
> huge amount of noise for me. 2 lines-of-code extra means this change
> is by one measure more complex but it removes special knowledge of
> asprintf return values and the meaning of %c, so on the other hand it
> is less complex. I won't be offended if it's not accepted.

I don't mind taking the patch, but the above description is needed in
the change log, as that explains the purpose of the patch.

You don't need to resend, I'll just cut and paste what you wrote above
to the commit message.

Thanks,

-- Steve
Ian Rogers May 28, 2024, 3:51 p.m. UTC | #4
On Thu, May 16, 2024 at 4:51 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Thu, 16 May 2024 15:09:40 -0700
> Ian Rogers <irogers@google.com> wrote:
>
> > > This states what the patch does, but omits why. What's wrong with
> > > asprintf() here?
> >
> > Nothing except overhead. I was tracking down an asprintf issue [1],
> > asprintf breaking the sanitizer stack traces on my debian based linux,
> > and using asprintf for the sake of copying a character was creating a
> > huge amount of noise for me. 2 lines-of-code extra means this change
> > is by one measure more complex but it removes special knowledge of
> > asprintf return values and the meaning of %c, so on the other hand it
> > is less complex. I won't be offended if it's not accepted.
>
> I don't mind taking the patch, but the above description is needed in
> the change log, as that explains the purpose of the patch.
>
> You don't need to resend, I'll just cut and paste what you wrote above
> to the commit message.

Thanks, the commit message LGTM.

Ian

> Thanks,
>
> -- Steve
diff mbox series

Patch

diff --git a/src/event-parse.c b/src/event-parse.c
index b6ae67e..16fdb46 100644
--- a/src/event-parse.c
+++ b/src/event-parse.c
@@ -1232,9 +1232,11 @@  static enum tep_event_type __read_token(struct tep_handle *tep, char **tok)
 	switch (type) {
 	case TEP_EVENT_NEWLINE:
 	case TEP_EVENT_DELIM:
-		if (asprintf(tok, "%c", ch) < 0)
+		*tok = malloc(2);
+		if (!*tok)
 			return TEP_EVENT_ERROR;
-
+		(*tok)[0] = ch;
+		(*tok)[1] = '\0';
 		return type;
 
 	case TEP_EVENT_OP: