| 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 |
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
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
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
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 --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:
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(-)