* [PATCH 0/6] Refactor the APIs for tracing options @ 2021-04-02 13:19 Yordan Karadzhov (VMware) 2021-04-02 13:19 ` [PATCH 1/6] libtracefs: Fix issues with tracefs_option_id() Yordan Karadzhov (VMware) ` (5 more replies) 0 siblings, 6 replies; 16+ messages in thread From: Yordan Karadzhov (VMware) @ 2021-04-02 13:19 UTC (permalink / raw) To: rostedt; +Cc: linux-trace-devel, tz.stoyanov, Yordan Karadzhov (VMware) Yordan Karadzhov (VMware) (6): libtracefs: Fix issues with tracefs_option_id() libtracefs: Modify the arguments of tracefs_option_is_set() libtracefs: Encapsulate "struct tracefs_options_mask" libtracefs: Move the "options" code to tracefs-instance.c libtracefs: Option's bit masks to be owned by the instance libtracefs: Rename tracefs_option_is_set() Documentation/libtracefs-option-get.txt | 4 +- include/tracefs.h | 15 +- src/tracefs-instance.c | 266 +++++++++++++++++++++++ src/tracefs-tools.c | 276 ------------------------ 4 files changed, 273 insertions(+), 288 deletions(-) -- 2.27.0 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/6] libtracefs: Fix issues with tracefs_option_id() 2021-04-02 13:19 [PATCH 0/6] Refactor the APIs for tracing options Yordan Karadzhov (VMware) @ 2021-04-02 13:19 ` Yordan Karadzhov (VMware) 2021-04-02 13:19 ` [PATCH 2/6] libtracefs: Modify the arguments of tracefs_option_is_set() Yordan Karadzhov (VMware) ` (4 subsequent siblings) 5 siblings, 0 replies; 16+ messages in thread From: Yordan Karadzhov (VMware) @ 2021-04-02 13:19 UTC (permalink / raw) To: rostedt; +Cc: linux-trace-devel, tz.stoyanov, Yordan Karadzhov (VMware) Few mistakes have been made when introducing this API function. First of all, its declaration is missing from the header file. In addition to this the argument type is missing "const". Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- include/tracefs.h | 1 + src/tracefs-tools.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/include/tracefs.h b/include/tracefs.h index 05bd0ef..fbd7db5 100644 --- a/include/tracefs.h +++ b/include/tracefs.h @@ -145,5 +145,6 @@ bool tracefs_option_is_enabled(struct tracefs_instance *instance, enum tracefs_o int tracefs_option_enable(struct tracefs_instance *instance, enum tracefs_option_id id); int tracefs_option_diasble(struct tracefs_instance *instance, enum tracefs_option_id id); const char *tracefs_option_name(enum tracefs_option_id id); +enum tracefs_option_id tracefs_option_id(const char *name); #endif /* _TRACE_FS_H */ diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c index e2dfc7b..51a7971 100644 --- a/src/tracefs-tools.c +++ b/src/tracefs-tools.c @@ -181,7 +181,7 @@ const char *tracefs_option_name(enum tracefs_option_id id) * Returns trace option ID or TRACEFS_OPTION_INVALID in case of an error or * unknown option name. */ -enum tracefs_option_id tracefs_option_id(char *name) +enum tracefs_option_id tracefs_option_id(const char *name) { int i; -- 2.27.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/6] libtracefs: Modify the arguments of tracefs_option_is_set() 2021-04-02 13:19 [PATCH 0/6] Refactor the APIs for tracing options Yordan Karadzhov (VMware) 2021-04-02 13:19 ` [PATCH 1/6] libtracefs: Fix issues with tracefs_option_id() Yordan Karadzhov (VMware) @ 2021-04-02 13:19 ` Yordan Karadzhov (VMware) 2021-04-02 13:19 ` [PATCH 3/6] libtracefs: Encapsulate "struct tracefs_options_mask" Yordan Karadzhov (VMware) ` (3 subsequent siblings) 5 siblings, 0 replies; 16+ messages in thread From: Yordan Karadzhov (VMware) @ 2021-04-02 13:19 UTC (permalink / raw) To: rostedt; +Cc: linux-trace-devel, tz.stoyanov, Yordan Karadzhov (VMware) This function is supposed to take as argument a mask returned by "tracefs_options_get_supported()" or "tracefs_options_get_enabled()", hence this argument must be a pointer. Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- include/tracefs.h | 4 ++-- src/tracefs-tools.c | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/include/tracefs.h b/include/tracefs.h index fbd7db5..5c4e50d 100644 --- a/include/tracefs.h +++ b/include/tracefs.h @@ -136,8 +136,8 @@ struct tracefs_options_mask { }; void tracefs_option_set(struct tracefs_options_mask *options, enum tracefs_option_id id); void tracefs_option_clear(struct tracefs_options_mask *options, enum tracefs_option_id id); -bool tracefs_option_is_set(struct tracefs_options_mask options, enum tracefs_option_id id); - +bool tracefs_option_is_set(struct tracefs_options_mask *options, + enum tracefs_option_id id); struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instance *instance); bool tracefs_option_is_supported(struct tracefs_instance *instance, enum tracefs_option_id id); struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance *instance); diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c index 51a7971..3b5f773 100644 --- a/src/tracefs-tools.c +++ b/src/tracefs-tools.c @@ -359,10 +359,11 @@ bool tracefs_option_is_enabled(struct tracefs_instance *instance, enum tracefs_o * Returns true if an option with given id is set in the bitmask, * false if it is not set. */ -bool tracefs_option_is_set(struct tracefs_options_mask options, enum tracefs_option_id id) +bool tracefs_option_is_set(struct tracefs_options_mask *options, + enum tracefs_option_id id) { if (id > TRACEFS_OPTION_INVALID) - return options.mask & (1ULL << (id - 1)); + return options->mask & (1ULL << (id - 1)); return false; } -- 2.27.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/6] libtracefs: Encapsulate "struct tracefs_options_mask" 2021-04-02 13:19 [PATCH 0/6] Refactor the APIs for tracing options Yordan Karadzhov (VMware) 2021-04-02 13:19 ` [PATCH 1/6] libtracefs: Fix issues with tracefs_option_id() Yordan Karadzhov (VMware) 2021-04-02 13:19 ` [PATCH 2/6] libtracefs: Modify the arguments of tracefs_option_is_set() Yordan Karadzhov (VMware) @ 2021-04-02 13:19 ` Yordan Karadzhov (VMware) 2021-04-02 14:13 ` Steven Rostedt 2021-04-02 13:19 ` [PATCH 4/6] libtracefs: Move the "options" code to tracefs-instance.c Yordan Karadzhov (VMware) ` (2 subsequent siblings) 5 siblings, 1 reply; 16+ messages in thread From: Yordan Karadzhov (VMware) @ 2021-04-02 13:19 UTC (permalink / raw) To: rostedt; +Cc: linux-trace-devel, tz.stoyanov, Yordan Karadzhov (VMware) The definition of the mask gets hidden from the user. This way we will be able to modify this definition in the future, without breaking the API. Such a modification will be compulsory, if too many new tracing options are added in the future. Note that encapsulating the mask definition, requires two API methods to be eliminated, however those methods have no particular use-cases for the moment. Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- include/tracefs.h | 6 +----- src/tracefs-tools.c | 33 +++++++++++---------------------- 2 files changed, 12 insertions(+), 27 deletions(-) diff --git a/include/tracefs.h b/include/tracefs.h index 5c4e50d..0665e8d 100644 --- a/include/tracefs.h +++ b/include/tracefs.h @@ -131,11 +131,7 @@ enum tracefs_option_id { }; #define TRACEFS_OPTION_MAX (TRACEFS_OPTION_VERBOSE + 1) -struct tracefs_options_mask { - unsigned long long mask; -}; -void tracefs_option_set(struct tracefs_options_mask *options, enum tracefs_option_id id); -void tracefs_option_clear(struct tracefs_options_mask *options, enum tracefs_option_id id); +struct tracefs_options_mask; bool tracefs_option_is_set(struct tracefs_options_mask *options, enum tracefs_option_id id); struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instance *instance); diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c index 3b5f773..11a4c8c 100644 --- a/src/tracefs-tools.c +++ b/src/tracefs-tools.c @@ -19,6 +19,10 @@ #define TRACE_CTRL "tracing_on" +struct tracefs_options_mask { + unsigned long long mask; +}; + static const char * const options_map[] = { "unknown", "annotate", @@ -197,6 +201,13 @@ enum tracefs_option_id tracefs_option_id(const char *name) return TRACEFS_OPTION_INVALID; } +static void tracefs_option_set(struct tracefs_options_mask *options, + enum tracefs_option_id id) +{ + if (options && id > TRACEFS_OPTION_INVALID) + options->mask |= (1ULL << (id - 1)); +} + static struct tracefs_options_mask *trace_get_options(struct tracefs_instance *instance, bool enabled) { @@ -366,25 +377,3 @@ bool tracefs_option_is_set(struct tracefs_options_mask *options, return options->mask & (1ULL << (id - 1)); return false; } - -/** - * tracefs_option_set - Set option in options bitmask - * @options: Pointer to a bitmask with options - * @id: trace option id - */ -void tracefs_option_set(struct tracefs_options_mask *options, enum tracefs_option_id id) -{ - if (options && id > TRACEFS_OPTION_INVALID) - options->mask |= (1ULL << (id - 1)); -} - -/** - * tracefs_option_clear - Clear option from options bitmask - * @options: Pointer to a bitmask with options - * @id: trace option id - */ -void tracefs_option_clear(struct tracefs_options_mask *options, enum tracefs_option_id id) -{ - if (options && id > TRACEFS_OPTION_INVALID) - options->mask &= ~(1ULL << (id - 1)); -} -- 2.27.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/6] libtracefs: Encapsulate "struct tracefs_options_mask" 2021-04-02 13:19 ` [PATCH 3/6] libtracefs: Encapsulate "struct tracefs_options_mask" Yordan Karadzhov (VMware) @ 2021-04-02 14:13 ` Steven Rostedt 0 siblings, 0 replies; 16+ messages in thread From: Steven Rostedt @ 2021-04-02 14:13 UTC (permalink / raw) To: Yordan Karadzhov (VMware); +Cc: linux-trace-devel, tz.stoyanov On Fri, 2 Apr 2021 16:19:44 +0300 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > @@ -197,6 +201,13 @@ enum tracefs_option_id tracefs_option_id(const char *name) > return TRACEFS_OPTION_INVALID; > } > > +static void tracefs_option_set(struct tracefs_options_mask *options, > + enum tracefs_option_id id) Patch is fine, but lets rename this function to: trace_option_set() As functions named "tracefs_*()" should be used only by functions that are for the API or will be for the API (staging functions). -- Steve > +{ > + if (options && id > TRACEFS_OPTION_INVALID) > + options->mask |= (1ULL << (id - 1)); > +} > + > static struct tracefs_options_mask *trace_get_options(struct tracefs_instance *instance, > bool enabled) > { ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 4/6] libtracefs: Move the "options" code to tracefs-instance.c 2021-04-02 13:19 [PATCH 0/6] Refactor the APIs for tracing options Yordan Karadzhov (VMware) ` (2 preceding siblings ...) 2021-04-02 13:19 ` [PATCH 3/6] libtracefs: Encapsulate "struct tracefs_options_mask" Yordan Karadzhov (VMware) @ 2021-04-02 13:19 ` Yordan Karadzhov (VMware) 2021-04-02 14:17 ` Steven Rostedt 2021-04-02 13:19 ` [PATCH 5/6] libtracefs: Option's bit masks to be owned by the instance Yordan Karadzhov (VMware) 2021-04-02 13:19 ` [PATCH 6/6] libtracefs: Rename tracefs_option_is_set() Yordan Karadzhov (VMware) 5 siblings, 1 reply; 16+ messages in thread From: Yordan Karadzhov (VMware) @ 2021-04-02 13:19 UTC (permalink / raw) To: rostedt; +Cc: linux-trace-devel, tz.stoyanov, Yordan Karadzhov (VMware) We would like the instance object to own two bit masks (struct tracefs_options_mask) associated with all supported and all enabled options. However, the definition of the instance itself is not public, hence the code implementing the "options" related APIs has to in the same source file as the definition of the instance. Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- src/tracefs-instance.c | 266 +++++++++++++++++++++++++++++++++++++++++ src/tracefs-tools.c | 266 ----------------------------------------- 2 files changed, 266 insertions(+), 266 deletions(-) diff --git a/src/tracefs-instance.c b/src/tracefs-instance.c index 0df313c..49645eb 100644 --- a/src/tracefs-instance.c +++ b/src/tracefs-instance.c @@ -18,6 +18,10 @@ #include "tracefs.h" #include "tracefs-local.h" +struct tracefs_options_mask { + unsigned long long mask; +}; + #define FLAG_INSTANCE_NEWLY_CREATED (1 << 0) struct tracefs_instance { char *trace_dir; @@ -587,3 +591,265 @@ out: free(all_clocks); return ret; } + +static const char * const options_map[] = { + "unknown", + "annotate", + "bin", + "blk_cgname", + "blk_cgroup", + "blk_classic", + "block", + "context-info", + "disable_on_free", + "display-graph", + "event-fork", + "funcgraph-abstime", + "funcgraph-cpu", + "funcgraph-duration", + "funcgraph-irqs", + "funcgraph-overhead", + "funcgraph-overrun", + "funcgraph-proc", + "funcgraph-tail", + "func_stack_trace", + "function-fork", + "function-trace", + "graph-time", + "hex", + "irq-info", + "latency-format", + "markers", + "overwrite", + "pause-on-trace", + "printk-msg-only", + "print-parent", + "raw", + "record-cmd", + "record-tgid", + "sleep-time", + "stacktrace", + "sym-addr", + "sym-offset", + "sym-userobj", + "trace_printk", + "userstacktrace", + "verbose" }; + +/** + * tracefs_option_name - Get trace option name from id + * @id: trace option id + * + * Returns string with option name, or "unknown" in case of not known option id. + * The returned string must *not* be freed. + */ +const char *tracefs_option_name(enum tracefs_option_id id) +{ + /* Make sure options map contains all the options */ + BUILD_BUG_ON(ARRAY_SIZE(options_map) != TRACEFS_OPTION_MAX); + + if (id < TRACEFS_OPTION_MAX) + return options_map[id]; + + return options_map[0]; +} + +/** + * tracefs_option_id - Get trace option ID from name + * @name: trace option name + * + * Returns trace option ID or TRACEFS_OPTION_INVALID in case of an error or + * unknown option name. + */ +enum tracefs_option_id tracefs_option_id(const char *name) +{ + int i; + + if (!name) + return TRACEFS_OPTION_INVALID; + + for (i = 0; i < TRACEFS_OPTION_MAX; i++) { + if (strlen(name) == strlen(options_map[i]) && + !strcmp(options_map[i], name)) + return i; + } + + return TRACEFS_OPTION_INVALID; +} + +static void tracefs_option_set(struct tracefs_options_mask *options, + enum tracefs_option_id id) +{ + if (options && id > TRACEFS_OPTION_INVALID) + options->mask |= (1ULL << (id - 1)); +} + +static struct tracefs_options_mask *trace_get_options(struct tracefs_instance *instance, + bool enabled) +{ + struct tracefs_options_mask *bitmask; + enum tracefs_option_id id; + char file[PATH_MAX]; + struct dirent *dent; + char *dname = NULL; + DIR *dir = NULL; + long long val; + + bitmask = calloc(1, sizeof(struct tracefs_options_mask)); + if (!bitmask) + return NULL; + dname = tracefs_instance_get_file(instance, "options"); + if (!dname) + goto error; + dir = opendir(dname); + if (!dir) + goto error; + + while ((dent = readdir(dir))) { + if (*dent->d_name == '.') + continue; + if (enabled) { + snprintf(file, PATH_MAX, "options/%s", dent->d_name); + if (tracefs_instance_file_read_number(instance, file, &val) != 0 || + val != 1) + continue; + } + id = tracefs_option_id(dent->d_name); + if (id != TRACEFS_OPTION_INVALID) + tracefs_option_set(bitmask, id); + } + closedir(dir); + tracefs_put_tracing_file(dname); + + return bitmask; + +error: + if (dir) + closedir(dir); + tracefs_put_tracing_file(dname); + free(bitmask); + return NULL; +} + +/** + * tracefs_options_get_supported - Get all supported trace options in given instance + * @instance: ftrace instance, can be NULL for the top instance + * + * Returns allocated bitmask structure with all trace options, supported in given + * instance, or NULL in case of an error. The returned structure must be freed with free() + */ +struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instance *instance) +{ + return trace_get_options(instance, false); +} + +/** + * tracefs_options_get_enabled - Get all currently enabled trace options in given instance + * @instance: ftrace instance, can be NULL for the top instance + * + * Returns allocated bitmask structure with all trace options, enabled in given + * instance, or NULL in case of an error. The returned structure must be freed with free() + */ +struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance *instance) +{ + return trace_get_options(instance, true); +} + +static int trace_config_option(struct tracefs_instance *instance, + enum tracefs_option_id id, bool set) +{ + char *set_str = set ? "1" : "0"; + char file[PATH_MAX]; + const char *name; + + name = tracefs_option_name(id); + if (!name) + return -1; + + snprintf(file, PATH_MAX, "options/%s", name); + if (strlen(set_str) != tracefs_instance_file_write(instance, file, set_str)) + return -1; + return 0; +} + +/** + * tracefs_option_enable - Enable trace option + * @instance: ftrace instance, can be NULL for the top instance + * @id: trace option id + * + * Returns -1 in case of an error or 0 otherwise + */ +int tracefs_option_enable(struct tracefs_instance *instance, enum tracefs_option_id id) +{ + return trace_config_option(instance, id, true); +} + +/** + * tracefs_option_diasble - Disable trace option + * @instance: ftrace instance, can be NULL for the top instance + * @id: trace option id + * + * Returns -1 in case of an error or 0 otherwise + */ +int tracefs_option_diasble(struct tracefs_instance *instance, enum tracefs_option_id id) +{ + return trace_config_option(instance, id, false); +} + +/** + * tracefs_option_is_supported - Check if an option is supported + * @instance: ftrace instance, can be NULL for the top instance + * @id: trace option id + * + * Returns true if an option with given id is supported by the system, false if + * it is not supported. + */ +bool tracefs_option_is_supported(struct tracefs_instance *instance, enum tracefs_option_id id) +{ + const char *name = tracefs_option_name(id); + char file[PATH_MAX]; + + if (!name) + return false; + snprintf(file, PATH_MAX, "options/%s", name); + return tracefs_file_exists(instance, file); +} + +/** + * tracefs_option_is_enabled - Check if an option is enabled in given instance + * @instance: ftrace instance, can be NULL for the top instance + * @id: trace option id + * + * Returns true if an option with given id is enabled in the given instance, + * false if it is not enabled. + */ +bool tracefs_option_is_enabled(struct tracefs_instance *instance, enum tracefs_option_id id) +{ + const char *name = tracefs_option_name(id); + char file[PATH_MAX]; + long long res; + + if (!name) + return false; + snprintf(file, PATH_MAX, "options/%s", name); + if (!tracefs_instance_file_read_number(instance, file, &res) && res) + return true; + + return false; +} + +/** + * tracefs_option_is_set - Check if given option is set in the bitmask + * @options: Options bitmask + * @id: trace option id + * + * Returns true if an option with given id is set in the bitmask, + * false if it is not set. + */ +bool tracefs_option_is_set(struct tracefs_options_mask *options, + enum tracefs_option_id id) +{ + if (id > TRACEFS_OPTION_INVALID) + return options->mask & (1ULL << (id - 1)); + return false; +} diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c index 11a4c8c..8f763a4 100644 --- a/src/tracefs-tools.c +++ b/src/tracefs-tools.c @@ -19,54 +19,6 @@ #define TRACE_CTRL "tracing_on" -struct tracefs_options_mask { - unsigned long long mask; -}; - -static const char * const options_map[] = { - "unknown", - "annotate", - "bin", - "blk_cgname", - "blk_cgroup", - "blk_classic", - "block", - "context-info", - "disable_on_free", - "display-graph", - "event-fork", - "funcgraph-abstime", - "funcgraph-cpu", - "funcgraph-duration", - "funcgraph-irqs", - "funcgraph-overhead", - "funcgraph-overrun", - "funcgraph-proc", - "funcgraph-tail", - "func_stack_trace", - "function-fork", - "function-trace", - "graph-time", - "hex", - "irq-info", - "latency-format", - "markers", - "overwrite", - "pause-on-trace", - "printk-msg-only", - "print-parent", - "raw", - "record-cmd", - "record-tgid", - "sleep-time", - "stacktrace", - "sym-addr", - "sym-offset", - "sym-userobj", - "trace_printk", - "userstacktrace", - "verbose" }; - static int trace_on_off(int fd, bool on) { const char *val = on ? "1" : "0"; @@ -159,221 +111,3 @@ int tracefs_trace_off_fd(int fd) return -1; return trace_on_off(fd, false); } - -/** - * tracefs_option_name - Get trace option name from id - * @id: trace option id - * - * Returns string with option name, or "unknown" in case of not known option id. - * The returned string must *not* be freed. - */ -const char *tracefs_option_name(enum tracefs_option_id id) -{ - /* Make sure options map contains all the options */ - BUILD_BUG_ON(ARRAY_SIZE(options_map) != TRACEFS_OPTION_MAX); - - if (id < TRACEFS_OPTION_MAX) - return options_map[id]; - - return options_map[0]; -} - -/** - * tracefs_option_id - Get trace option ID from name - * @name: trace option name - * - * Returns trace option ID or TRACEFS_OPTION_INVALID in case of an error or - * unknown option name. - */ -enum tracefs_option_id tracefs_option_id(const char *name) -{ - int i; - - if (!name) - return TRACEFS_OPTION_INVALID; - - for (i = 0; i < TRACEFS_OPTION_MAX; i++) { - if (strlen(name) == strlen(options_map[i]) && - !strcmp(options_map[i], name)) - return i; - } - - return TRACEFS_OPTION_INVALID; -} - -static void tracefs_option_set(struct tracefs_options_mask *options, - enum tracefs_option_id id) -{ - if (options && id > TRACEFS_OPTION_INVALID) - options->mask |= (1ULL << (id - 1)); -} - -static struct tracefs_options_mask *trace_get_options(struct tracefs_instance *instance, - bool enabled) -{ - struct tracefs_options_mask *bitmask; - enum tracefs_option_id id; - char file[PATH_MAX]; - struct dirent *dent; - char *dname = NULL; - DIR *dir = NULL; - long long val; - - bitmask = calloc(1, sizeof(struct tracefs_options_mask)); - if (!bitmask) - return NULL; - dname = tracefs_instance_get_file(instance, "options"); - if (!dname) - goto error; - dir = opendir(dname); - if (!dir) - goto error; - - while ((dent = readdir(dir))) { - if (*dent->d_name == '.') - continue; - if (enabled) { - snprintf(file, PATH_MAX, "options/%s", dent->d_name); - if (tracefs_instance_file_read_number(instance, file, &val) != 0 || - val != 1) - continue; - } - id = tracefs_option_id(dent->d_name); - if (id != TRACEFS_OPTION_INVALID) - tracefs_option_set(bitmask, id); - } - closedir(dir); - tracefs_put_tracing_file(dname); - - return bitmask; - -error: - if (dir) - closedir(dir); - tracefs_put_tracing_file(dname); - free(bitmask); - return NULL; -} - -/** - * tracefs_options_get_supported - Get all supported trace options in given instance - * @instance: ftrace instance, can be NULL for the top instance - * - * Returns allocated bitmask structure with all trace options, supported in given - * instance, or NULL in case of an error. The returned structure must be freed with free() - */ -struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instance *instance) -{ - return trace_get_options(instance, false); -} - -/** - * tracefs_options_get_enabled - Get all currently enabled trace options in given instance - * @instance: ftrace instance, can be NULL for the top instance - * - * Returns allocated bitmask structure with all trace options, enabled in given - * instance, or NULL in case of an error. The returned structure must be freed with free() - */ -struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance *instance) -{ - return trace_get_options(instance, true); -} - -static int trace_config_option(struct tracefs_instance *instance, - enum tracefs_option_id id, bool set) -{ - char *set_str = set ? "1" : "0"; - char file[PATH_MAX]; - const char *name; - - name = tracefs_option_name(id); - if (!name) - return -1; - - snprintf(file, PATH_MAX, "options/%s", name); - if (strlen(set_str) != tracefs_instance_file_write(instance, file, set_str)) - return -1; - return 0; -} - -/** - * tracefs_option_enable - Enable trace option - * @instance: ftrace instance, can be NULL for the top instance - * @id: trace option id - * - * Returns -1 in case of an error or 0 otherwise - */ -int tracefs_option_enable(struct tracefs_instance *instance, enum tracefs_option_id id) -{ - return trace_config_option(instance, id, true); -} - -/** - * tracefs_option_diasble - Disable trace option - * @instance: ftrace instance, can be NULL for the top instance - * @id: trace option id - * - * Returns -1 in case of an error or 0 otherwise - */ -int tracefs_option_diasble(struct tracefs_instance *instance, enum tracefs_option_id id) -{ - return trace_config_option(instance, id, false); -} - -/** - * tracefs_option_is_supported - Check if an option is supported - * @instance: ftrace instance, can be NULL for the top instance - * @id: trace option id - * - * Returns true if an option with given id is supported by the system, false if - * it is not supported. - */ -bool tracefs_option_is_supported(struct tracefs_instance *instance, enum tracefs_option_id id) -{ - const char *name = tracefs_option_name(id); - char file[PATH_MAX]; - - if (!name) - return false; - snprintf(file, PATH_MAX, "options/%s", name); - return tracefs_file_exists(instance, file); -} - -/** - * tracefs_option_is_enabled - Check if an option is enabled in given instance - * @instance: ftrace instance, can be NULL for the top instance - * @id: trace option id - * - * Returns true if an option with given id is enabled in the given instance, - * false if it is not enabled. - */ -bool tracefs_option_is_enabled(struct tracefs_instance *instance, enum tracefs_option_id id) -{ - const char *name = tracefs_option_name(id); - char file[PATH_MAX]; - long long res; - - if (!name) - return false; - snprintf(file, PATH_MAX, "options/%s", name); - if (!tracefs_instance_file_read_number(instance, file, &res) && res) - return true; - - return false; -} - -/** - * tracefs_option_is_set - Check if given option is set in the bitmask - * @options: Options bitmask - * @id: trace option id - * - * Returns true if an option with given id is set in the bitmask, - * false if it is not set. - */ -bool tracefs_option_is_set(struct tracefs_options_mask *options, - enum tracefs_option_id id) -{ - if (id > TRACEFS_OPTION_INVALID) - return options->mask & (1ULL << (id - 1)); - return false; -} -- 2.27.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 4/6] libtracefs: Move the "options" code to tracefs-instance.c 2021-04-02 13:19 ` [PATCH 4/6] libtracefs: Move the "options" code to tracefs-instance.c Yordan Karadzhov (VMware) @ 2021-04-02 14:17 ` Steven Rostedt 2021-04-02 14:20 ` Steven Rostedt 2021-04-02 16:01 ` Yordan Karadzhov 0 siblings, 2 replies; 16+ messages in thread From: Steven Rostedt @ 2021-04-02 14:17 UTC (permalink / raw) To: Yordan Karadzhov (VMware); +Cc: linux-trace-devel, tz.stoyanov On Fri, 2 Apr 2021 16:19:45 +0300 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > We would like the instance object to own two bit masks (struct > tracefs_options_mask) associated with all supported and all enabled > options. However, the definition of the instance itself is not public, > hence the code implementing the "options" related APIs has to in the > same source file as the definition of the instance. The above is not true. We can keep the code in tracefs-tools.c and make it "public" for the internal use of the library, but not part of the API. To do so, you place shared structures and functions in include/tracefs-local.h, and mark all non static functions with "__hidden" which will keep it from being used outside the library. Do you still feel that the moving of this code is needed? -- Steve > > Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> > --- > src/tracefs-instance.c | 266 +++++++++++++++++++++++++++++++++++++++++ > src/tracefs-tools.c | 266 ----------------------------------------- > 2 files changed, 266 insertions(+), 266 deletions(-) ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 4/6] libtracefs: Move the "options" code to tracefs-instance.c 2021-04-02 14:17 ` Steven Rostedt @ 2021-04-02 14:20 ` Steven Rostedt 2021-04-02 16:01 ` Yordan Karadzhov 1 sibling, 0 replies; 16+ messages in thread From: Steven Rostedt @ 2021-04-02 14:20 UTC (permalink / raw) To: Yordan Karadzhov (VMware); +Cc: linux-trace-devel, tz.stoyanov On Fri, 2 Apr 2021 10:17:56 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > Do you still feel that the moving of this code is needed? The rest of the patch series looks fine. Can you send a v2? I'll let you and Tzvetomir decide if you want the code from this patch moved to the instance file. But at a minimum, lets rename tracefs_option_set() to trace_option_set(). Thanks! -- Steve ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 4/6] libtracefs: Move the "options" code to tracefs-instance.c 2021-04-02 14:17 ` Steven Rostedt 2021-04-02 14:20 ` Steven Rostedt @ 2021-04-02 16:01 ` Yordan Karadzhov 1 sibling, 0 replies; 16+ messages in thread From: Yordan Karadzhov @ 2021-04-02 16:01 UTC (permalink / raw) To: Steven Rostedt; +Cc: linux-trace-devel, tz.stoyanov On 2.04.21 г. 17:17, Steven Rostedt wrote: > On Fri, 2 Apr 2021 16:19:45 +0300 > "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > >> We would like the instance object to own two bit masks (struct >> tracefs_options_mask) associated with all supported and all enabled >> options. However, the definition of the instance itself is not public, >> hence the code implementing the "options" related APIs has to in the >> same source file as the definition of the instance. > The above is not true. We can keep the code in tracefs-tools.c and make it > "public" for the internal use of the library, but not part of the API. To > do so, you place shared structures and functions in include/tracefs-local.h, > and mark all non static functions with "__hidden" which will keep it from > being used outside the library. > > Do you still feel that the moving of this code is needed? Hi Steven, Thanks a lot for the review! I wasn't sure if it is OK to move the "tracefs_instance" struct to "include/tracefs-local.h". If this is the case, then I will drop this patch and send v2. cheers, Yordan > > -- Steve > > >> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> >> --- >> src/tracefs-instance.c | 266 +++++++++++++++++++++++++++++++++++++++++ >> src/tracefs-tools.c | 266 ----------------------------------------- >> 2 files changed, 266 insertions(+), 266 deletions(-) ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 5/6] libtracefs: Option's bit masks to be owned by the instance 2021-04-02 13:19 [PATCH 0/6] Refactor the APIs for tracing options Yordan Karadzhov (VMware) ` (3 preceding siblings ...) 2021-04-02 13:19 ` [PATCH 4/6] libtracefs: Move the "options" code to tracefs-instance.c Yordan Karadzhov (VMware) @ 2021-04-02 13:19 ` Yordan Karadzhov (VMware) 2021-04-05 10:50 ` Tzvetomir Stoyanov 2021-04-05 10:56 ` Tzvetomir Stoyanov 2021-04-02 13:19 ` [PATCH 6/6] libtracefs: Rename tracefs_option_is_set() Yordan Karadzhov (VMware) 5 siblings, 2 replies; 16+ messages in thread From: Yordan Karadzhov (VMware) @ 2021-04-02 13:19 UTC (permalink / raw) To: rostedt; +Cc: linux-trace-devel, tz.stoyanov, Yordan Karadzhov (VMware) If the instance owns two mask objects, we no longer need to dynamically allocate memory in tracefs_options_get_supported() and tracefs_options_get_enabled(). This will simplify the code on the caller side, since the user is no longer responsible for freeing those masks. Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- Documentation/libtracefs-option-get.txt | 4 +--- include/tracefs.h | 6 +++--- src/tracefs-instance.c | 24 ++++++++++++------------ 3 files changed, 16 insertions(+), 18 deletions(-) diff --git a/Documentation/libtracefs-option-get.txt b/Documentation/libtracefs-option-get.txt index 9b3cb56..3290f24 100644 --- a/Documentation/libtracefs-option-get.txt +++ b/Documentation/libtracefs-option-get.txt @@ -52,14 +52,13 @@ EXAMPLE -- #include <tracefs.h> ... -struct tracefs_options_mask *options; +const struct tracefs_options_mask *options; ... options = tracefs_options_get_supported(NULL); if (!options) { /* Failed to get supported options */ } else { ... - free(options); } ... options = tracefs_options_get_enabled(NULL); @@ -67,7 +66,6 @@ if (!options) { /* Failed to get options, enabled in the top instance */ } else { ... - free(options); } ... diff --git a/include/tracefs.h b/include/tracefs.h index 0665e8d..9efa91a 100644 --- a/include/tracefs.h +++ b/include/tracefs.h @@ -132,11 +132,11 @@ enum tracefs_option_id { #define TRACEFS_OPTION_MAX (TRACEFS_OPTION_VERBOSE + 1) struct tracefs_options_mask; -bool tracefs_option_is_set(struct tracefs_options_mask *options, +bool tracefs_option_is_set(const struct tracefs_options_mask *options, enum tracefs_option_id id); -struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instance *instance); +const struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instance *instance); bool tracefs_option_is_supported(struct tracefs_instance *instance, enum tracefs_option_id id); -struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance *instance); +const struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance *instance); bool tracefs_option_is_enabled(struct tracefs_instance *instance, enum tracefs_option_id id); int tracefs_option_enable(struct tracefs_instance *instance, enum tracefs_option_id id); int tracefs_option_diasble(struct tracefs_instance *instance, enum tracefs_option_id id); diff --git a/src/tracefs-instance.c b/src/tracefs-instance.c index 49645eb..9cb4b6d 100644 --- a/src/tracefs-instance.c +++ b/src/tracefs-instance.c @@ -27,6 +27,8 @@ struct tracefs_instance { char *trace_dir; char *name; int flags; + struct tracefs_options_mask supported_opts; + struct tracefs_options_mask enabled_opts; }; /** @@ -695,9 +697,8 @@ static struct tracefs_options_mask *trace_get_options(struct tracefs_instance *i DIR *dir = NULL; long long val; - bitmask = calloc(1, sizeof(struct tracefs_options_mask)); - if (!bitmask) - return NULL; + bitmask = enabled? &instance->enabled_opts : &instance->supported_opts; + dname = tracefs_instance_get_file(instance, "options"); if (!dname) goto error; @@ -727,7 +728,6 @@ error: if (dir) closedir(dir); tracefs_put_tracing_file(dname); - free(bitmask); return NULL; } @@ -735,10 +735,10 @@ error: * tracefs_options_get_supported - Get all supported trace options in given instance * @instance: ftrace instance, can be NULL for the top instance * - * Returns allocated bitmask structure with all trace options, supported in given - * instance, or NULL in case of an error. The returned structure must be freed with free() + * Returns bitmask structure with all trace options, supported in given instance, + * or NULL in case of an error. */ -struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instance *instance) +const struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instance *instance) { return trace_get_options(instance, false); } @@ -747,10 +747,10 @@ struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instan * tracefs_options_get_enabled - Get all currently enabled trace options in given instance * @instance: ftrace instance, can be NULL for the top instance * - * Returns allocated bitmask structure with all trace options, enabled in given - * instance, or NULL in case of an error. The returned structure must be freed with free() + * Returns allocated bitmask structure with all trace options, enabled in given instance, + * or NULL in case of an error. */ -struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance *instance) +const struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance *instance) { return trace_get_options(instance, true); } @@ -758,7 +758,7 @@ struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance static int trace_config_option(struct tracefs_instance *instance, enum tracefs_option_id id, bool set) { - char *set_str = set ? "1" : "0"; + const char *set_str = set ? "1" : "0"; char file[PATH_MAX]; const char *name; @@ -846,7 +846,7 @@ bool tracefs_option_is_enabled(struct tracefs_instance *instance, enum tracefs_o * Returns true if an option with given id is set in the bitmask, * false if it is not set. */ -bool tracefs_option_is_set(struct tracefs_options_mask *options, +bool tracefs_option_is_set(const struct tracefs_options_mask *options, enum tracefs_option_id id) { if (id > TRACEFS_OPTION_INVALID) -- 2.27.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 5/6] libtracefs: Option's bit masks to be owned by the instance 2021-04-02 13:19 ` [PATCH 5/6] libtracefs: Option's bit masks to be owned by the instance Yordan Karadzhov (VMware) @ 2021-04-05 10:50 ` Tzvetomir Stoyanov 2021-04-05 15:01 ` Yordan Karadzhov 2021-04-05 10:56 ` Tzvetomir Stoyanov 1 sibling, 1 reply; 16+ messages in thread From: Tzvetomir Stoyanov @ 2021-04-05 10:50 UTC (permalink / raw) To: Yordan Karadzhov (VMware); +Cc: Steven Rostedt, Linux Trace Devel On Fri, Apr 2, 2021 at 4:20 PM Yordan Karadzhov (VMware) <y.karadz@gmail.com> wrote: > > If the instance owns two mask objects, we no longer need to > dynamically allocate memory in tracefs_options_get_supported() and > tracefs_options_get_enabled(). This will simplify the code on the > caller side, since the user is no longer responsible for freeing > those masks. > > Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> > --- > Documentation/libtracefs-option-get.txt | 4 +--- > include/tracefs.h | 6 +++--- > src/tracefs-instance.c | 24 ++++++++++++------------ > 3 files changed, 16 insertions(+), 18 deletions(-) > > diff --git a/Documentation/libtracefs-option-get.txt b/Documentation/libtracefs-option-get.txt > index 9b3cb56..3290f24 100644 > --- a/Documentation/libtracefs-option-get.txt > +++ b/Documentation/libtracefs-option-get.txt > @@ -52,14 +52,13 @@ EXAMPLE > -- > #include <tracefs.h> > ... > -struct tracefs_options_mask *options; > +const struct tracefs_options_mask *options; > ... > options = tracefs_options_get_supported(NULL); > if (!options) { > /* Failed to get supported options */ > } else { > ... > - free(options); > } > .. The description of the return value of tracefs_options_get_supported() and tracefs_options_get_enabled() should be changed also, as it suggests to free the return value; > options = tracefs_options_get_enabled(NULL); > @@ -67,7 +66,6 @@ if (!options) { > /* Failed to get options, enabled in the top instance */ > } else { > ... > - free(options); > } > ... > > diff --git a/include/tracefs.h b/include/tracefs.h > index 0665e8d..9efa91a 100644 > --- a/include/tracefs.h > +++ b/include/tracefs.h > @@ -132,11 +132,11 @@ enum tracefs_option_id { > #define TRACEFS_OPTION_MAX (TRACEFS_OPTION_VERBOSE + 1) > > struct tracefs_options_mask; > -bool tracefs_option_is_set(struct tracefs_options_mask *options, > +bool tracefs_option_is_set(const struct tracefs_options_mask *options, > enum tracefs_option_id id); > -struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instance *instance); > +const struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instance *instance); > bool tracefs_option_is_supported(struct tracefs_instance *instance, enum tracefs_option_id id); > -struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance *instance); > +const struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance *instance); > bool tracefs_option_is_enabled(struct tracefs_instance *instance, enum tracefs_option_id id); > int tracefs_option_enable(struct tracefs_instance *instance, enum tracefs_option_id id); > int tracefs_option_diasble(struct tracefs_instance *instance, enum tracefs_option_id id); > diff --git a/src/tracefs-instance.c b/src/tracefs-instance.c > index 49645eb..9cb4b6d 100644 > --- a/src/tracefs-instance.c > +++ b/src/tracefs-instance.c > @@ -27,6 +27,8 @@ struct tracefs_instance { > char *trace_dir; > char *name; > int flags; > + struct tracefs_options_mask supported_opts; > + struct tracefs_options_mask enabled_opts; > }; > > /** > @@ -695,9 +697,8 @@ static struct tracefs_options_mask *trace_get_options(struct tracefs_instance *i > DIR *dir = NULL; > long long val; > > - bitmask = calloc(1, sizeof(struct tracefs_options_mask)); > - if (!bitmask) > - return NULL; > + bitmask = enabled? &instance->enabled_opts : &instance->supported_opts; This makes the API not thread safe. There should be no problem with supported options, as they cannot be changed at runtime. I think it is OK to limit the APIs , as I see no use case to call them from multiple threads on the same instance, but this should be described in the documentation. > + > dname = tracefs_instance_get_file(instance, "options"); > if (!dname) > goto error; > @@ -727,7 +728,6 @@ error: > if (dir) > closedir(dir); > tracefs_put_tracing_file(dname); > - free(bitmask); > return NULL; > } > > @@ -735,10 +735,10 @@ error: > * tracefs_options_get_supported - Get all supported trace options in given instance > * @instance: ftrace instance, can be NULL for the top instance > * > - * Returns allocated bitmask structure with all trace options, supported in given > - * instance, or NULL in case of an error. The returned structure must be freed with free() > + * Returns bitmask structure with all trace options, supported in given instance, > + * or NULL in case of an error. > */ > -struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instance *instance) > +const struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instance *instance) > { > return trace_get_options(instance, false); As supported options cannot be changed at runtime, they can be read only once. The next calls can return the already initialised bitmask with supported options. > } > @@ -747,10 +747,10 @@ struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instan > * tracefs_options_get_enabled - Get all currently enabled trace options in given instance > * @instance: ftrace instance, can be NULL for the top instance > * > - * Returns allocated bitmask structure with all trace options, enabled in given > - * instance, or NULL in case of an error. The returned structure must be freed with free() > + * Returns allocated bitmask structure with all trace options, enabled in given instance, > + * or NULL in case of an error. > */ > -struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance *instance) > +const struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance *instance) > { > return trace_get_options(instance, true); > } > @@ -758,7 +758,7 @@ struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance > static int trace_config_option(struct tracefs_instance *instance, > enum tracefs_option_id id, bool set) > { > - char *set_str = set ? "1" : "0"; > + const char *set_str = set ? "1" : "0"; > char file[PATH_MAX]; > const char *name; > > @@ -846,7 +846,7 @@ bool tracefs_option_is_enabled(struct tracefs_instance *instance, enum tracefs_o > * Returns true if an option with given id is set in the bitmask, > * false if it is not set. > */ > -bool tracefs_option_is_set(struct tracefs_options_mask *options, > +bool tracefs_option_is_set(const struct tracefs_options_mask *options, > enum tracefs_option_id id) > { > if (id > TRACEFS_OPTION_INVALID) > -- > 2.27.0 > -- Tzvetomir (Ceco) Stoyanov VMware Open Source Technology Center ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 5/6] libtracefs: Option's bit masks to be owned by the instance 2021-04-05 10:50 ` Tzvetomir Stoyanov @ 2021-04-05 15:01 ` Yordan Karadzhov 2021-04-05 15:10 ` Steven Rostedt 0 siblings, 1 reply; 16+ messages in thread From: Yordan Karadzhov @ 2021-04-05 15:01 UTC (permalink / raw) To: Tzvetomir Stoyanov; +Cc: Steven Rostedt, Linux Trace Devel On 5.04.21 г. 13:50, Tzvetomir Stoyanov wrote: > On Fri, Apr 2, 2021 at 4:20 PM Yordan Karadzhov (VMware) > <y.karadz@gmail.com> wrote: >> If the instance owns two mask objects, we no longer need to >> dynamically allocate memory in tracefs_options_get_supported() and >> tracefs_options_get_enabled(). This will simplify the code on the >> caller side, since the user is no longer responsible for freeing >> those masks. >> >> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> >> --- >> Documentation/libtracefs-option-get.txt | 4 +--- >> include/tracefs.h | 6 +++--- >> src/tracefs-instance.c | 24 ++++++++++++------------ >> 3 files changed, 16 insertions(+), 18 deletions(-) >> >> diff --git a/Documentation/libtracefs-option-get.txt b/Documentation/libtracefs-option-get.txt >> index 9b3cb56..3290f24 100644 >> --- a/Documentation/libtracefs-option-get.txt >> +++ b/Documentation/libtracefs-option-get.txt >> @@ -52,14 +52,13 @@ EXAMPLE >> -- >> #include <tracefs.h> >> ... >> -struct tracefs_options_mask *options; >> +const struct tracefs_options_mask *options; >> ... >> options = tracefs_options_get_supported(NULL); >> if (!options) { >> /* Failed to get supported options */ >> } else { >> ... >> - free(options); >> } >> .. > The description of the return value of tracefs_options_get_supported() > and tracefs_options_get_enabled() should be changed also, as it > suggests to free the return value; > >> options = tracefs_options_get_enabled(NULL); >> @@ -67,7 +66,6 @@ if (!options) { >> /* Failed to get options, enabled in the top instance */ >> } else { >> ... >> - free(options); >> } >> ... >> >> diff --git a/include/tracefs.h b/include/tracefs.h >> index 0665e8d..9efa91a 100644 >> --- a/include/tracefs.h >> +++ b/include/tracefs.h >> @@ -132,11 +132,11 @@ enum tracefs_option_id { >> #define TRACEFS_OPTION_MAX (TRACEFS_OPTION_VERBOSE + 1) >> >> struct tracefs_options_mask; >> -bool tracefs_option_is_set(struct tracefs_options_mask *options, >> +bool tracefs_option_is_set(const struct tracefs_options_mask *options, >> enum tracefs_option_id id); >> -struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instance *instance); >> +const struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instance *instance); >> bool tracefs_option_is_supported(struct tracefs_instance *instance, enum tracefs_option_id id); >> -struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance *instance); >> +const struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance *instance); >> bool tracefs_option_is_enabled(struct tracefs_instance *instance, enum tracefs_option_id id); >> int tracefs_option_enable(struct tracefs_instance *instance, enum tracefs_option_id id); >> int tracefs_option_diasble(struct tracefs_instance *instance, enum tracefs_option_id id); >> diff --git a/src/tracefs-instance.c b/src/tracefs-instance.c >> index 49645eb..9cb4b6d 100644 >> --- a/src/tracefs-instance.c >> +++ b/src/tracefs-instance.c >> @@ -27,6 +27,8 @@ struct tracefs_instance { >> char *trace_dir; >> char *name; >> int flags; >> + struct tracefs_options_mask supported_opts; >> + struct tracefs_options_mask enabled_opts; >> }; >> >> /** >> @@ -695,9 +697,8 @@ static struct tracefs_options_mask *trace_get_options(struct tracefs_instance *i >> DIR *dir = NULL; >> long long val; >> >> - bitmask = calloc(1, sizeof(struct tracefs_options_mask)); >> - if (!bitmask) >> - return NULL; >> + bitmask = enabled? &instance->enabled_opts : &instance->supported_opts; > This makes the API not thread safe. There should be no problem with > supported options, as they cannot be changed at runtime. I think it is > OK to limit the APIs , as I see no use case to call them from multiple > threads on the same instance, but this should be described in the > documentation. Discussing is this API is thread safe or not is not really relevant. Even if you make tracefs_options_get_enabled() thread safe, this does not change anything since the options can be already changed by the time when you examine the bits of the mask. > >> + >> dname = tracefs_instance_get_file(instance, "options"); >> if (!dname) >> goto error; >> @@ -727,7 +728,6 @@ error: >> if (dir) >> closedir(dir); >> tracefs_put_tracing_file(dname); >> - free(bitmask); >> return NULL; >> } >> >> @@ -735,10 +735,10 @@ error: >> * tracefs_options_get_supported - Get all supported trace options in given instance >> * @instance: ftrace instance, can be NULL for the top instance >> * >> - * Returns allocated bitmask structure with all trace options, supported in given >> - * instance, or NULL in case of an error. The returned structure must be freed with free() >> + * Returns bitmask structure with all trace options, supported in given instance, >> + * or NULL in case of an error. >> */ >> -struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instance *instance) >> +const struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instance *instance) >> { >> return trace_get_options(instance, false); > As supported options cannot be changed at runtime, they can be read > only once. The next calls can return the already initialised bitmask > with supported options. If you already have the mask, you do not need to call this function second time. May be if the user intentionally calls the function again, we should allow him to regenerate the mask. What do you think? Thanks! Yordan > >> } >> @@ -747,10 +747,10 @@ struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instan >> * tracefs_options_get_enabled - Get all currently enabled trace options in given instance >> * @instance: ftrace instance, can be NULL for the top instance >> * >> - * Returns allocated bitmask structure with all trace options, enabled in given >> - * instance, or NULL in case of an error. The returned structure must be freed with free() >> + * Returns allocated bitmask structure with all trace options, enabled in given instance, >> + * or NULL in case of an error. >> */ >> -struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance *instance) >> +const struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance *instance) >> { >> return trace_get_options(instance, true); >> } >> @@ -758,7 +758,7 @@ struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance >> static int trace_config_option(struct tracefs_instance *instance, >> enum tracefs_option_id id, bool set) >> { >> - char *set_str = set ? "1" : "0"; >> + const char *set_str = set ? "1" : "0"; >> char file[PATH_MAX]; >> const char *name; >> >> @@ -846,7 +846,7 @@ bool tracefs_option_is_enabled(struct tracefs_instance *instance, enum tracefs_o >> * Returns true if an option with given id is set in the bitmask, >> * false if it is not set. >> */ >> -bool tracefs_option_is_set(struct tracefs_options_mask *options, >> +bool tracefs_option_is_set(const struct tracefs_options_mask *options, >> enum tracefs_option_id id) >> { >> if (id > TRACEFS_OPTION_INVALID) >> -- >> 2.27.0 >> > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 5/6] libtracefs: Option's bit masks to be owned by the instance 2021-04-05 15:01 ` Yordan Karadzhov @ 2021-04-05 15:10 ` Steven Rostedt 0 siblings, 0 replies; 16+ messages in thread From: Steven Rostedt @ 2021-04-05 15:10 UTC (permalink / raw) To: Yordan Karadzhov; +Cc: Tzvetomir Stoyanov, Linux Trace Devel On Mon, 5 Apr 2021 18:01:41 +0300 Yordan Karadzhov <y.karadz@gmail.com> wrote: > > This makes the API not thread safe. There should be no problem with > > supported options, as they cannot be changed at runtime. I think it is > > OK to limit the APIs , as I see no use case to call them from multiple > > threads on the same instance, but this should be described in the > > documentation. > > Discussing is this API is thread safe or not is not really relevant. > Even if you make tracefs_options_get_enabled() thread safe, this does > not change anything since the options can be already changed by the time > when you examine the bits of the mask. You are correct, but you also do not want the update of the bitmask to be corrupted. Thus I think a pthread_mutex() should be added to the updating of the value, such that we don't have a read/modify/write corruption. Sure the reader of the mask may get some stale data if its being updated. But it shouldn't get corrupted data. void tracefs_option_set(struct tracefs_options_mask *options, enum tracefs_option_id id) { if (options && id > TRACEFS_OPTION_INVALID) options->mask |= (1ULL << (id - 1)); } If two threads are doing this to the same mask, you can have an option set that gets cleared by another writer, and even though the option is set after the call, it will be clear in the mask. That is, it's not stale data, it's incorrect data. That is, the readers after that corruption will get an option not set, even though it is. -- Steve ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 5/6] libtracefs: Option's bit masks to be owned by the instance 2021-04-02 13:19 ` [PATCH 5/6] libtracefs: Option's bit masks to be owned by the instance Yordan Karadzhov (VMware) 2021-04-05 10:50 ` Tzvetomir Stoyanov @ 2021-04-05 10:56 ` Tzvetomir Stoyanov 2021-04-05 15:03 ` Yordan Karadzhov 1 sibling, 1 reply; 16+ messages in thread From: Tzvetomir Stoyanov @ 2021-04-05 10:56 UTC (permalink / raw) To: Yordan Karadzhov (VMware); +Cc: Steven Rostedt, Linux Trace Devel On Fri, Apr 2, 2021 at 4:20 PM Yordan Karadzhov (VMware) <y.karadz@gmail.com> wrote: > > If the instance owns two mask objects, we no longer need to > dynamically allocate memory in tracefs_options_get_supported() and > tracefs_options_get_enabled(). This will simplify the code on the > caller side, since the user is no longer responsible for freeing > those masks. > > Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> > --- > Documentation/libtracefs-option-get.txt | 4 +--- > include/tracefs.h | 6 +++--- > src/tracefs-instance.c | 24 ++++++++++++------------ > 3 files changed, 16 insertions(+), 18 deletions(-) > > diff --git a/Documentation/libtracefs-option-get.txt b/Documentation/libtracefs-option-get.txt > index 9b3cb56..3290f24 100644 > --- a/Documentation/libtracefs-option-get.txt > +++ b/Documentation/libtracefs-option-get.txt > @@ -52,14 +52,13 @@ EXAMPLE > -- > #include <tracefs.h> > ... > -struct tracefs_options_mask *options; > +const struct tracefs_options_mask *options; > ... > options = tracefs_options_get_supported(NULL); > if (!options) { > /* Failed to get supported options */ > } else { > ... > - free(options); > } > ... > options = tracefs_options_get_enabled(NULL); > @@ -67,7 +66,6 @@ if (!options) { > /* Failed to get options, enabled in the top instance */ > } else { > ... > - free(options); > } > ... > > diff --git a/include/tracefs.h b/include/tracefs.h > index 0665e8d..9efa91a 100644 > --- a/include/tracefs.h > +++ b/include/tracefs.h > @@ -132,11 +132,11 @@ enum tracefs_option_id { > #define TRACEFS_OPTION_MAX (TRACEFS_OPTION_VERBOSE + 1) > > struct tracefs_options_mask; > -bool tracefs_option_is_set(struct tracefs_options_mask *options, > +bool tracefs_option_is_set(const struct tracefs_options_mask *options, > enum tracefs_option_id id); > -struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instance *instance); > +const struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instance *instance); > bool tracefs_option_is_supported(struct tracefs_instance *instance, enum tracefs_option_id id); > -struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance *instance); > +const struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance *instance); > bool tracefs_option_is_enabled(struct tracefs_instance *instance, enum tracefs_option_id id); > int tracefs_option_enable(struct tracefs_instance *instance, enum tracefs_option_id id); > int tracefs_option_diasble(struct tracefs_instance *instance, enum tracefs_option_id id); > diff --git a/src/tracefs-instance.c b/src/tracefs-instance.c > index 49645eb..9cb4b6d 100644 > --- a/src/tracefs-instance.c > +++ b/src/tracefs-instance.c > @@ -27,6 +27,8 @@ struct tracefs_instance { > char *trace_dir; > char *name; > int flags; > + struct tracefs_options_mask supported_opts; > + struct tracefs_options_mask enabled_opts; > }; > > /** > @@ -695,9 +697,8 @@ static struct tracefs_options_mask *trace_get_options(struct tracefs_instance *i > DIR *dir = NULL; > long long val; > > - bitmask = calloc(1, sizeof(struct tracefs_options_mask)); > - if (!bitmask) > - return NULL; > + bitmask = enabled? &instance->enabled_opts : &instance->supported_opts; The "instance" argument can be NULL, for the top instance. There should be global enabled_opts and supported_opts, to be used in that case. [..] -- Tzvetomir (Ceco) Stoyanov VMware Open Source Technology Center ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 5/6] libtracefs: Option's bit masks to be owned by the instance 2021-04-05 10:56 ` Tzvetomir Stoyanov @ 2021-04-05 15:03 ` Yordan Karadzhov 0 siblings, 0 replies; 16+ messages in thread From: Yordan Karadzhov @ 2021-04-05 15:03 UTC (permalink / raw) To: Tzvetomir Stoyanov; +Cc: Steven Rostedt, Linux Trace Devel On 5.04.21 г. 13:56, Tzvetomir Stoyanov wrote: > On Fri, Apr 2, 2021 at 4:20 PM Yordan Karadzhov (VMware) > <y.karadz@gmail.com> wrote: >> If the instance owns two mask objects, we no longer need to >> dynamically allocate memory in tracefs_options_get_supported() and >> tracefs_options_get_enabled(). This will simplify the code on the >> caller side, since the user is no longer responsible for freeing >> those masks. >> >> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> >> --- >> Documentation/libtracefs-option-get.txt | 4 +--- >> include/tracefs.h | 6 +++--- >> src/tracefs-instance.c | 24 ++++++++++++------------ >> 3 files changed, 16 insertions(+), 18 deletions(-) >> >> diff --git a/Documentation/libtracefs-option-get.txt b/Documentation/libtracefs-option-get.txt >> index 9b3cb56..3290f24 100644 >> --- a/Documentation/libtracefs-option-get.txt >> +++ b/Documentation/libtracefs-option-get.txt >> @@ -52,14 +52,13 @@ EXAMPLE >> -- >> #include <tracefs.h> >> ... >> -struct tracefs_options_mask *options; >> +const struct tracefs_options_mask *options; >> ... >> options = tracefs_options_get_supported(NULL); >> if (!options) { >> /* Failed to get supported options */ >> } else { >> ... >> - free(options); >> } >> ... >> options = tracefs_options_get_enabled(NULL); >> @@ -67,7 +66,6 @@ if (!options) { >> /* Failed to get options, enabled in the top instance */ >> } else { >> ... >> - free(options); >> } >> ... >> >> diff --git a/include/tracefs.h b/include/tracefs.h >> index 0665e8d..9efa91a 100644 >> --- a/include/tracefs.h >> +++ b/include/tracefs.h >> @@ -132,11 +132,11 @@ enum tracefs_option_id { >> #define TRACEFS_OPTION_MAX (TRACEFS_OPTION_VERBOSE + 1) >> >> struct tracefs_options_mask; >> -bool tracefs_option_is_set(struct tracefs_options_mask *options, >> +bool tracefs_option_is_set(const struct tracefs_options_mask *options, >> enum tracefs_option_id id); >> -struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instance *instance); >> +const struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instance *instance); >> bool tracefs_option_is_supported(struct tracefs_instance *instance, enum tracefs_option_id id); >> -struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance *instance); >> +const struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance *instance); >> bool tracefs_option_is_enabled(struct tracefs_instance *instance, enum tracefs_option_id id); >> int tracefs_option_enable(struct tracefs_instance *instance, enum tracefs_option_id id); >> int tracefs_option_diasble(struct tracefs_instance *instance, enum tracefs_option_id id); >> diff --git a/src/tracefs-instance.c b/src/tracefs-instance.c >> index 49645eb..9cb4b6d 100644 >> --- a/src/tracefs-instance.c >> +++ b/src/tracefs-instance.c >> @@ -27,6 +27,8 @@ struct tracefs_instance { >> char *trace_dir; >> char *name; >> int flags; >> + struct tracefs_options_mask supported_opts; >> + struct tracefs_options_mask enabled_opts; >> }; >> >> /** >> @@ -695,9 +697,8 @@ static struct tracefs_options_mask *trace_get_options(struct tracefs_instance *i >> DIR *dir = NULL; >> long long val; >> >> - bitmask = calloc(1, sizeof(struct tracefs_options_mask)); >> - if (!bitmask) >> - return NULL; >> + bitmask = enabled? &instance->enabled_opts : &instance->supported_opts; > The "instance" argument can be NULL, for the top instance. There > should be global enabled_opts and supported_opts, to be used in that > case. Yes, this is a bug and I have it in v2 as well. Seems like I will need to send v3. Thanks a lot! Y. > > [..] > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 6/6] libtracefs: Rename tracefs_option_is_set() 2021-04-02 13:19 [PATCH 0/6] Refactor the APIs for tracing options Yordan Karadzhov (VMware) ` (4 preceding siblings ...) 2021-04-02 13:19 ` [PATCH 5/6] libtracefs: Option's bit masks to be owned by the instance Yordan Karadzhov (VMware) @ 2021-04-02 13:19 ` Yordan Karadzhov (VMware) 5 siblings, 0 replies; 16+ messages in thread From: Yordan Karadzhov (VMware) @ 2021-04-02 13:19 UTC (permalink / raw) To: rostedt; +Cc: linux-trace-devel, tz.stoyanov, Yordan Karadzhov (VMware) The old name of the function is potentially confusing. Indeed the function do not check anything about the option itself (is it supported or is it enabled). The function only check if inside the mask the bit that correspond to a given option is set. Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- include/tracefs.h | 4 ++-- src/tracefs-instance.c | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/include/tracefs.h b/include/tracefs.h index 9efa91a..05d2edb 100644 --- a/include/tracefs.h +++ b/include/tracefs.h @@ -132,8 +132,8 @@ enum tracefs_option_id { #define TRACEFS_OPTION_MAX (TRACEFS_OPTION_VERBOSE + 1) struct tracefs_options_mask; -bool tracefs_option_is_set(const struct tracefs_options_mask *options, - enum tracefs_option_id id); +bool tracefs_option_mask_is_set(const struct tracefs_options_mask *options, + enum tracefs_option_id id); const struct tracefs_options_mask *tracefs_options_get_supported(struct tracefs_instance *instance); bool tracefs_option_is_supported(struct tracefs_instance *instance, enum tracefs_option_id id); const struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance *instance); diff --git a/src/tracefs-instance.c b/src/tracefs-instance.c index 9cb4b6d..f37b024 100644 --- a/src/tracefs-instance.c +++ b/src/tracefs-instance.c @@ -839,15 +839,15 @@ bool tracefs_option_is_enabled(struct tracefs_instance *instance, enum tracefs_o } /** - * tracefs_option_is_set - Check if given option is set in the bitmask + * tracefs_option_mask_is_set - Check if given option is set in the bitmask * @options: Options bitmask * @id: trace option id * * Returns true if an option with given id is set in the bitmask, * false if it is not set. */ -bool tracefs_option_is_set(const struct tracefs_options_mask *options, - enum tracefs_option_id id) +bool tracefs_option_mask_is_set(const struct tracefs_options_mask *options, + enum tracefs_option_id id) { if (id > TRACEFS_OPTION_INVALID) return options->mask & (1ULL << (id - 1)); -- 2.27.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-04-05 15:10 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-02 13:19 [PATCH 0/6] Refactor the APIs for tracing options Yordan Karadzhov (VMware) 2021-04-02 13:19 ` [PATCH 1/6] libtracefs: Fix issues with tracefs_option_id() Yordan Karadzhov (VMware) 2021-04-02 13:19 ` [PATCH 2/6] libtracefs: Modify the arguments of tracefs_option_is_set() Yordan Karadzhov (VMware) 2021-04-02 13:19 ` [PATCH 3/6] libtracefs: Encapsulate "struct tracefs_options_mask" Yordan Karadzhov (VMware) 2021-04-02 14:13 ` Steven Rostedt 2021-04-02 13:19 ` [PATCH 4/6] libtracefs: Move the "options" code to tracefs-instance.c Yordan Karadzhov (VMware) 2021-04-02 14:17 ` Steven Rostedt 2021-04-02 14:20 ` Steven Rostedt 2021-04-02 16:01 ` Yordan Karadzhov 2021-04-02 13:19 ` [PATCH 5/6] libtracefs: Option's bit masks to be owned by the instance Yordan Karadzhov (VMware) 2021-04-05 10:50 ` Tzvetomir Stoyanov 2021-04-05 15:01 ` Yordan Karadzhov 2021-04-05 15:10 ` Steven Rostedt 2021-04-05 10:56 ` Tzvetomir Stoyanov 2021-04-05 15:03 ` Yordan Karadzhov 2021-04-02 13:19 ` [PATCH 6/6] libtracefs: Rename tracefs_option_is_set() Yordan Karadzhov (VMware)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).