[PATCH] perf trace: Remove return value of trace__fprintf_tp_fields

[PATCH] perf trace: Remove return value of trace__fprintf_tp_fields

[Benjamin Peterson]

The return value of this function was meaningless and therefore ignored by
the caller. Remove it.

Signed-off-by: Benjamin Peterson <benjamin@engflow.com>
---
This is a small followup cleanup to my earlier commit 5fb8e56542a3 ("perf
trace: avoid garbage when not printing a trace event's arguments").
---
 tools/perf/builtin-trace.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 6a1a128fe645014d0347ad4ec3e0c9e77ec59aee..246be66fd59a4b9d76e4d3c42b68d0a444ca366a 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -3022,8 +3022,8 @@ static void bpf_output__fprintf(struct trace *trace,
 	++trace->nr_events_printed;
 }
 
-static size_t trace__fprintf_tp_fields(struct trace *trace, struct evsel *evsel, struct perf_sample *sample,
-				       struct thread *thread, void *augmented_args, int augmented_args_size)
+static void trace__fprintf_tp_fields(struct trace *trace, struct evsel *evsel, struct perf_sample *sample,
+				     struct thread *thread, void *augmented_args, int augmented_args_size)
 {
 	char bf[2048];
 	size_t size = sizeof(bf);
@@ -3088,7 +3088,7 @@ static size_t trace__fprintf_tp_fields(struct trace *trace, struct evsel *evsel,
 		printed += syscall_arg_fmt__scnprintf_val(arg, bf + printed, size - printed, &syscall_arg, val);
 	}
 
-	return printed + fprintf(trace->output, "%.*s", (int)printed, bf);
+	fprintf(trace->output, "%.*s", (int)printed, bf);
 }
 
 static int trace__event_handler(struct trace *trace, struct evsel *evsel,

---
base-commit: eefa7a9c069908412f8f5d15833901d1b46ae1b2
change-id: 20241212-void-fprintf_tp_fields-8aaae2f5525f

Best regards,
-- 
Benjamin Peterson <benjamin@engflow.com>

Re: [PATCH] perf trace: Remove return value of trace__fprintf_tp_fields

[Howard Chu]

Hello Benjamin,

On Thu, Dec 12, 2024 at 7:56 PM Benjamin Peterson <benjamin@engflow.com> wrote:
>
> The return value of this function was meaningless and therefore ignored by
> the caller. Remove it.
>
> Signed-off-by: Benjamin Peterson <benjamin@engflow.com>
> ---
> This is a small followup cleanup to my earlier commit 5fb8e56542a3 ("perf
> trace: avoid garbage when not printing a trace event's arguments").
> ---
>  tools/perf/builtin-trace.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 6a1a128fe645014d0347ad4ec3e0c9e77ec59aee..246be66fd59a4b9d76e4d3c42b68d0a444ca366a 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -3022,8 +3022,8 @@ static void bpf_output__fprintf(struct trace *trace,
>         ++trace->nr_events_printed;
>  }
>
> -static size_t trace__fprintf_tp_fields(struct trace *trace, struct evsel *evsel, struct perf_sample *sample,
> -                                      struct thread *thread, void *augmented_args, int augmented_args_size)
> +static void trace__fprintf_tp_fields(struct trace *trace, struct evsel *evsel, struct perf_sample *sample,
> +                                    struct thread *thread, void *augmented_args, int augmented_args_size)
>  {
>         char bf[2048];
>         size_t size = sizeof(bf);
> @@ -3088,7 +3088,7 @@ static size_t trace__fprintf_tp_fields(struct trace *trace, struct evsel *evsel,
>                 printed += syscall_arg_fmt__scnprintf_val(arg, bf + printed, size - printed, &syscall_arg, val);
>         }
>
> -       return printed + fprintf(trace->output, "%.*s", (int)printed, bf);
> +       fprintf(trace->output, "%.*s", (int)printed, bf);
>  }
>
>  static int trace__event_handler(struct trace *trace, struct evsel *evsel,
>
> ---
> base-commit: eefa7a9c069908412f8f5d15833901d1b46ae1b2
> change-id: 20241212-void-fprintf_tp_fields-8aaae2f5525f
>
> Best regards,
> --
> Benjamin Peterson <benjamin@engflow.com>
>
>

Thank you for the follow-up patch, LGTM. Built and tested.

perf $ ./perf test "perf trace"
109: perf trace enum augmentation tests                              : Ok
110: perf trace exit race                                            : Ok

Reviewed-by: Howard Chu <howardchu95@gmail.com>

Thanks,
Howard

Re: [PATCH] perf trace: Remove return value of trace__fprintf_tp_fields

[Howard Chu]

Hello fellow maintainers,

Just a slight ping.

Thanks,
Howard

On Fri, Dec 13, 2024 at 10:06 AM Howard Chu <howardchu95@gmail.com> wrote:
>
> Hello Benjamin,
>
> On Thu, Dec 12, 2024 at 7:56 PM Benjamin Peterson <benjamin@engflow.com> wrote:
> >
> > The return value of this function was meaningless and therefore ignored by
> > the caller. Remove it.
> >
> > Signed-off-by: Benjamin Peterson <benjamin@engflow.com>
> > ---
> > This is a small followup cleanup to my earlier commit 5fb8e56542a3 ("perf
> > trace: avoid garbage when not printing a trace event's arguments").
> > ---
> >  tools/perf/builtin-trace.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > index 6a1a128fe645014d0347ad4ec3e0c9e77ec59aee..246be66fd59a4b9d76e4d3c42b68d0a444ca366a 100644
> > --- a/tools/perf/builtin-trace.c
> > +++ b/tools/perf/builtin-trace.c
> > @@ -3022,8 +3022,8 @@ static void bpf_output__fprintf(struct trace *trace,
> >         ++trace->nr_events_printed;
> >  }
> >
> > -static size_t trace__fprintf_tp_fields(struct trace *trace, struct evsel *evsel, struct perf_sample *sample,
> > -                                      struct thread *thread, void *augmented_args, int augmented_args_size)
> > +static void trace__fprintf_tp_fields(struct trace *trace, struct evsel *evsel, struct perf_sample *sample,
> > +                                    struct thread *thread, void *augmented_args, int augmented_args_size)
> >  {
> >         char bf[2048];
> >         size_t size = sizeof(bf);
> > @@ -3088,7 +3088,7 @@ static size_t trace__fprintf_tp_fields(struct trace *trace, struct evsel *evsel,
> >                 printed += syscall_arg_fmt__scnprintf_val(arg, bf + printed, size - printed, &syscall_arg, val);
> >         }
> >
> > -       return printed + fprintf(trace->output, "%.*s", (int)printed, bf);
> > +       fprintf(trace->output, "%.*s", (int)printed, bf);
> >  }
> >
> >  static int trace__event_handler(struct trace *trace, struct evsel *evsel,
> >
> > ---
> > base-commit: eefa7a9c069908412f8f5d15833901d1b46ae1b2
> > change-id: 20241212-void-fprintf_tp_fields-8aaae2f5525f
> >
> > Best regards,
> > --
> > Benjamin Peterson <benjamin@engflow.com>
> >
> >
>
> Thank you for the follow-up patch, LGTM. Built and tested.
>
> perf $ ./perf test "perf trace"
> 109: perf trace enum augmentation tests                              : Ok
> 110: perf trace exit race                                            : Ok
>
> Reviewed-by: Howard Chu <howardchu95@gmail.com>
>
> Thanks,
> Howard

Re: [PATCH] perf trace: Remove return value of trace__fprintf_tp_fields

[Namhyung Kim]

Hello,

On Tue, Jan 21, 2025 at 07:01:53PM -0800, Howard Chu wrote:
> Hello fellow maintainers,
> 
> Just a slight ping.

Thanks for the reminder.

> 
> Thanks,
> Howard
> 
> On Fri, Dec 13, 2024 at 10:06 AM Howard Chu <howardchu95@gmail.com> wrote:
> >
> > Hello Benjamin,
> >
> > On Thu, Dec 12, 2024 at 7:56 PM Benjamin Peterson <benjamin@engflow.com> wrote:
> > >
> > > The return value of this function was meaningless and therefore ignored by
> > > the caller. Remove it.

In fact, I don't see the reason to merge this, as it's the convention to
return the number of characters it prints.  Yes, it's not used but not
much reason to remove it.

Thanks,
Namhyung

> > >
> > > Signed-off-by: Benjamin Peterson <benjamin@engflow.com>
> > > ---
> > > This is a small followup cleanup to my earlier commit 5fb8e56542a3 ("perf
> > > trace: avoid garbage when not printing a trace event's arguments").
> > > ---
> > >  tools/perf/builtin-trace.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > > index 6a1a128fe645014d0347ad4ec3e0c9e77ec59aee..246be66fd59a4b9d76e4d3c42b68d0a444ca366a 100644
> > > --- a/tools/perf/builtin-trace.c
> > > +++ b/tools/perf/builtin-trace.c
> > > @@ -3022,8 +3022,8 @@ static void bpf_output__fprintf(struct trace *trace,
> > >         ++trace->nr_events_printed;
> > >  }
> > >
> > > -static size_t trace__fprintf_tp_fields(struct trace *trace, struct evsel *evsel, struct perf_sample *sample,
> > > -                                      struct thread *thread, void *augmented_args, int augmented_args_size)
> > > +static void trace__fprintf_tp_fields(struct trace *trace, struct evsel *evsel, struct perf_sample *sample,
> > > +                                    struct thread *thread, void *augmented_args, int augmented_args_size)
> > >  {
> > >         char bf[2048];
> > >         size_t size = sizeof(bf);
> > > @@ -3088,7 +3088,7 @@ static size_t trace__fprintf_tp_fields(struct trace *trace, struct evsel *evsel,
> > >                 printed += syscall_arg_fmt__scnprintf_val(arg, bf + printed, size - printed, &syscall_arg, val);
> > >         }
> > >
> > > -       return printed + fprintf(trace->output, "%.*s", (int)printed, bf);
> > > +       fprintf(trace->output, "%.*s", (int)printed, bf);
> > >  }
> > >
> > >  static int trace__event_handler(struct trace *trace, struct evsel *evsel,
> > >
> > > ---
> > > base-commit: eefa7a9c069908412f8f5d15833901d1b46ae1b2
> > > change-id: 20241212-void-fprintf_tp_fields-8aaae2f5525f
> > >
> > > Best regards,
> > > --
> > > Benjamin Peterson <benjamin@engflow.com>
> > >
> > >
> >
> > Thank you for the follow-up patch, LGTM. Built and tested.
> >
> > perf $ ./perf test "perf trace"
> > 109: perf trace enum augmentation tests                              : Ok
> > 110: perf trace exit race                                            : Ok
> >
> > Reviewed-by: Howard Chu <howardchu95@gmail.com>
> >
> > Thanks,
> > Howard

Re: [PATCH] perf trace: Remove return value of trace__fprintf_tp_fields

[Benjamin Peterson]

On Wed, Jan 22, 2025 at 11:08 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hello,
>
> On Tue, Jan 21, 2025 at 07:01:53PM -0800, Howard Chu wrote:
> > Hello fellow maintainers,
> >
> > Just a slight ping.
>
> Thanks for the reminder.

Thanks for taking a look.

>
> >
> > Thanks,
> > Howard
> >
> > On Fri, Dec 13, 2024 at 10:06 AM Howard Chu <howardchu95@gmail.com> wrote:
> > >
> > > Hello Benjamin,
> > >
> > > On Thu, Dec 12, 2024 at 7:56 PM Benjamin Peterson <benjamin@engflow.com> wrote:
> > > >
> > > > The return value of this function was meaningless and therefore ignored by
> > > > the caller. Remove it.
>
> In fact, I don't see the reason to merge this, as it's the convention to
> return the number of characters it prints.  Yes, it's not used but not
> much reason to remove it.

The main reason to remove the return value is that it has no useful
interpretation and is therefore confusing. To wit, the function
returns two times the number of bytes it prints.

Re: [PATCH] perf trace: Remove return value of trace__fprintf_tp_fields

[Namhyung Kim]

On Wed, Jan 22, 2025 at 11:19:05AM -0800, Benjamin Peterson wrote:
> On Wed, Jan 22, 2025 at 11:08 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Hello,
> >
> > On Tue, Jan 21, 2025 at 07:01:53PM -0800, Howard Chu wrote:
> > > Hello fellow maintainers,
> > >
> > > Just a slight ping.
> >
> > Thanks for the reminder.
> 
> Thanks for taking a look.
> 
> >
> > >
> > > Thanks,
> > > Howard
> > >
> > > On Fri, Dec 13, 2024 at 10:06 AM Howard Chu <howardchu95@gmail.com> wrote:
> > > >
> > > > Hello Benjamin,
> > > >
> > > > On Thu, Dec 12, 2024 at 7:56 PM Benjamin Peterson <benjamin@engflow.com> wrote:
> > > > >
> > > > > The return value of this function was meaningless and therefore ignored by
> > > > > the caller. Remove it.
> >
> > In fact, I don't see the reason to merge this, as it's the convention to
> > return the number of characters it prints.  Yes, it's not used but not
> > much reason to remove it.
> 
> The main reason to remove the return value is that it has no useful
> interpretation and is therefore confusing. To wit, the function
> returns two times the number of bytes it prints.

Oh, ok.  Then please just fix it to return the correct number.

Thanks,
Namhyung

Re: [PATCH] perf trace: Remove return value of trace__fprintf_tp_fields

[Benjamin Peterson]

On Thu, Jan 23, 2025 at 3:48 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Wed, Jan 22, 2025 at 11:19:05AM -0800, Benjamin Peterson wrote:
> > On Wed, Jan 22, 2025 at 11:08 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > Hello,
> > >
> > > On Tue, Jan 21, 2025 at 07:01:53PM -0800, Howard Chu wrote:
> > > > Hello fellow maintainers,
> > > >
> > > > Just a slight ping.
> > >
> > > Thanks for the reminder.
> >
> > Thanks for taking a look.
> >
> > >
> > > >
> > > > Thanks,
> > > > Howard
> > > >
> > > > On Fri, Dec 13, 2024 at 10:06 AM Howard Chu <howardchu95@gmail.com> wrote:
> > > > >
> > > > > Hello Benjamin,
> > > > >
> > > > > On Thu, Dec 12, 2024 at 7:56 PM Benjamin Peterson <benjamin@engflow.com> wrote:
> > > > > >
> > > > > > The return value of this function was meaningless and therefore ignored by
> > > > > > the caller. Remove it.
> > >
> > > In fact, I don't see the reason to merge this, as it's the convention to
> > > return the number of characters it prints.  Yes, it's not used but not
> > > much reason to remove it.
> >
> > The main reason to remove the return value is that it has no useful
> > interpretation and is therefore confusing. To wit, the function
> > returns two times the number of bytes it prints.
>
> Oh, ok.  Then please just fix it to return the correct number.

Alright, I've submitted v2 that simply propagates the return value of fprintf.