Hi Tao
Thanks for your reviews!
On 10/26/21 18:22, Tao Zhou wrote:
> Hi Daniel,
>
> On Mon, Oct 25, 2021 at 07:40:33PM +0200, Daniel Bristot de Oliveira wrote:
>
>> This is a set of utils and tracer helper functions. They are used by
>> rtla mostly to parse config, display data and some trace operations that
>> are not part of libtracefs (because they are only useful it for this
>> case).
>>
>> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
>> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>> Cc: Tom Zanussi <zanussi@xxxxxxxxxx>
>> Cc: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
>> Cc: Juri Lelli <juri.lelli@xxxxxxxxxx>
>> Cc: Clark Williams <williams@xxxxxxxxxx>
>> Cc: John Kacur <jkacur@xxxxxxxxxx>
>> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>> Cc: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
>> Cc: Daniel Bristot de Oliveira <bristot@xxxxxxxxxx>
>> Cc: linux-rt-users@xxxxxxxxxxxxxxx
>> Cc: linux-trace-devel@xxxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Signed-off-by: Daniel Bristot de Oliveira <bristot@xxxxxxxxxx>
>> ---
>> tools/tracing/rtla/src/trace.c | 219 +++++++++++++++++
>> tools/tracing/rtla/src/trace.h | 27 ++
>> tools/tracing/rtla/src/utils.c | 433 +++++++++++++++++++++++++++++++++
>> tools/tracing/rtla/src/utils.h | 56 +++++
>> 4 files changed, 735 insertions(+)
>> create mode 100644 tools/tracing/rtla/src/trace.c
>> create mode 100644 tools/tracing/rtla/src/trace.h
>> create mode 100644 tools/tracing/rtla/src/utils.c
>> create mode 100644 tools/tracing/rtla/src/utils.h
>>
>> diff --git a/tools/tracing/rtla/src/trace.c b/tools/tracing/rtla/src/trace.c
>> new file mode 100644
>> index 000000000000..0f66e99fef0d
>> --- /dev/null
>> +++ b/tools/tracing/rtla/src/trace.c
>> @@ -0,0 +1,219 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +#define _GNU_SOURCE
>> +#include <sys/sendfile.h>
>> +#include <tracefs.h>
>> +#include <signal.h>
>> +#include <stdlib.h>
>> +#include <unistd.h>
>> +#include <errno.h>
>> +
>> +#include "trace.h"
>> +#include "utils.h"
>> +
>> +/*
>> + * enable_tracer_by_name - enable a tracer on the given instance
>> + */
>> +int enable_tracer_by_name(struct tracefs_instance *inst, const char *tracer)
>> +{
>> + enum tracefs_tracers t;
>> + int retval;
>> +
>> + t = TRACEFS_TRACER_CUSTOM;
>> +
>> + debug_msg("enabling %s tracer\n", tracer);
>> +
>> + retval = tracefs_tracer_set(inst, t, tracer);
>> + if (retval < 0) {
>> + if (errno == ENODEV)
>> + err_msg("tracer %s not found!\n", tracer);
>> +
>> + err_msg("failed to enable the tracer %s\n", tracer);
>> + return -1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * disable_tracer - set nop tracer to the insta
>> + */
>> +void disable_tracer(struct tracefs_instance *inst)
>> +{
>> + enum tracefs_tracers t = TRACEFS_TRACER_NOP;
>> + int retval;
>> +
>> + retval = tracefs_tracer_set(inst, t);
>
> Thank you for share. Also not know much about trace..
> Nits below.
>
> enable_tracer_by_name() call tracefs_tracer_set(inst, t, tracer).
> Is tracefs_tracer_set() called here lack a NULL; like:
>
> tracefs_tracer_set(inst, t, NULL)
>
> Or I am wrong.
I am just following documentation:
https://git.kernel.org/pub/scm/libs/libtrace/libtracefs.git/tree/Documentation/libtracefs-tracer.txt#n14
>> + if (retval < 0)
>> + err_msg("oops, error disabling tracer\n");
>> +}
>> +
>> +/*
>> + * create_instance - create a trace instance with *instance_name
>> + */
>> +struct tracefs_instance *create_instance(char *instance_name)
>> +{
>> + return tracefs_instance_create(instance_name);
>> +}
>> +
>> +/*
>> + * destroy_instance - remove a trace instance and free the data
>> + */
>> +void destroy_instance(struct tracefs_instance *inst)
>> +{
>> + tracefs_instance_destroy(inst);
>> + tracefs_instance_free(inst);
>> +}
>> +
>> +/*
>> + * save_trace_to_file - save the trace output of the instance to the file
>> + */
>> +int save_trace_to_file(struct tracefs_instance *inst, const char *filename)
>> +{
>> + const char *file = "trace";
>> + mode_t mode = 0644;
>> + char *buffer[4096];
>> + int out_fd, in_fd;
>> + int retval = -1;
>> +
>> + in_fd = tracefs_instance_file_open(inst, file, O_RDONLY);
>> + if (in_fd < 0) {
>> + err_msg("Failed to open trace file\n");
>> + return -1;
>> + }
>> +
>> + out_fd = creat(filename, mode);
>> + if (out_fd < 0) {
>> + err_msg("Failed to create output file %s\n", filename);
>> + goto out_close;
>> + }
>> +
>> + do {
>> + retval = read(in_fd, buffer, sizeof(buffer));
>> + if (read <= 0)
>
> check "retval" not read. Like:
>
> if (retval <= 0)
I bet vim's ^p helped on that.... fixing in a new version.
>> + goto out_close;
>> +
>> + retval = write(out_fd, buffer, retval);
>> + if (retval < 0)
>> + goto out_close;
>> + } while (retval > 0);
>> +
>> + retval = 0;
>> + close(out_fd);
>> +out_close:
>
> And this out_close: label put before "close(out_fd);". Like:
>
> retval = 0;
> out_close:
> close(out_fd);
> close(in_fd);
Actually, I need to add another label, one to close onlu in_fd, and another to
close all... anyway, yep, there is an error here.
Thanks
-- Daniel
