Re: [PATCH 5/6] libtracefs: Option's bit masks to be owned by the instance

[Yordan Karadzhov <y.karadz@gmail.com>]

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
>>
>