Re: [PATCH v2 2/4] libtracefs: Transform tracefs_hist_add_sort_key()

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

On 24.09.21 г. 18:51, Tzvetomir Stoyanov wrote:
> On Fri, Sep 24, 2021 at 12:59 PM Yordan Karadzhov (VMware)
> <y.karadz@gmail.com> wrote:
>>
>> The current version of the API makes it hard to add multiple sort keys
>> to a histogram. The only way to do this is to use the variadic arguments,
>> however in order to do this the caller has to know the number of sort
>> keys at compile time, because the method overwrite all previously added
>> keys. The problem is addressed by creating a tracefs_hist_add_sort_key()
>> into two methods - one that overwrite and one that does not.
>> The current version of 'tracefs_hist_add_sort_key()' gets renamed to
>> 'tracefs_hist_set_sort_key()' without introducing any functional changes.
>> In the same time a new 'tracefs_hist_add_sort_key()' function is
>> defined. The new function can add one new sort key to the list of
>> previously added sort keys.
>>
>> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
>> ---
>>   include/tracefs.h  |  4 +++-
>>   src/tracefs-hist.c | 33 ++++++++++++++++++++++++++++++---
>>   2 files changed, 33 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/tracefs.h b/include/tracefs.h
>> index 5c4141e..230bc41 100644
>> --- a/include/tracefs.h
>> +++ b/include/tracefs.h
>> @@ -333,7 +333,9 @@ int tracefs_hist_add_key(struct tracefs_hist *hist, const char *key,
>>                           enum tracefs_hist_key_type type);
>>   int tracefs_hist_add_value(struct tracefs_hist *hist, const char *value);
>>   int tracefs_hist_add_sort_key(struct tracefs_hist *hist,
>> -                             const char *sort_key, ...);
>> +                             const char *sort_key);
>> +int tracefs_hist_reset_sort_key(struct tracefs_hist *hist,
>> +                               const char *sort_key, ...);
> 
> The new name of the function, mentioned in the patch description, is
>      tracefs_hist_set_sort_key()
> I like the name with "set" instead of "reset". The behaviour of the
> function should be described in the function comments - to stress that
> the old keys are overwritten.
> 

Steven suggested to name it 'reset' in the review of the first version.
Both 'set' and 'reset' are OK for me.

Thanks!
Yordan


>>   int tracefs_hist_sort_key_direction(struct tracefs_hist *hist,
>>                                      const char *sort_key,
>>                                      enum tracefs_hist_sort_direction dir);
>> diff --git a/src/tracefs-hist.c b/src/tracefs-hist.c
>> index ea12204..a7c20de 100644
>> --- a/src/tracefs-hist.c
>> +++ b/src/tracefs-hist.c
>> @@ -437,16 +437,43 @@ add_sort_key(struct tracefs_hist *hist, const char *sort_key, char **list)
>>   /**
>>    * tracefs_hist_add_sort_key - add a key for sorting the histogram
>>    * @hist: The histogram to add the sort key to
>> + * @sort_key: The key to sort
>> + *
>> + * Call the function to add to the list of sort keys of the histohram in
>> + * the order of priority that the keys would be sorted on output.
>> + *
>> + * Returns 0 on success, -1 on error.
>> + */
>> +int tracefs_hist_add_sort_key(struct tracefs_hist *hist,
>> +                             const char *sort_key)
>> +{
>> +       char **list = hist->sort;
>> +
>> +       if (!hist || !sort_key)
>> +               return -1;
>> +
>> +       list = add_sort_key(hist, sort_key, hist->sort);
>> +       if (!list)
>> +               return -1;
>> +
>> +       hist->sort = list;
>> +
>> +       return 0;
>> +}
>> +
>> +/**
>> + * tracefs_hist_reset_sort_key - set a key for sorting the histogram
>> + * @hist: The histogram to set the sort key to
>>    * @sort_key: The key to sort (and the strings after it)
>>    *  Last one must be NULL.
>>    *
>> - * Add a list of sort keys in the order of priority that the
>> + * Set a list of sort keys in the order of priority that the
>>    * keys would be sorted on output. Keys must be added first.
>>    *
>>    * Returns 0 on success, -1 on error.
>>    */
>> -int tracefs_hist_add_sort_key(struct tracefs_hist *hist,
>> -                             const char *sort_key, ...)
>> +int tracefs_hist_reset_sort_key(struct tracefs_hist *hist,
>> +                               const char *sort_key, ...)
>>   {
>>          char **list = NULL;
>>          char **tmp;
>> --
>> 2.30.2
>>
> 
> 
> --
> Tzvetomir (Ceco) Stoyanov
> VMware Open Source Technology Center
>