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

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

On 10.09.21 г. 23:01, Steven Rostedt wrote:
> On Fri, 10 Sep 2021 19:38:55 +0300
> "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 have to know the number of sort
>> keys at compile time, because the method overwrite all previously added
>> keys. The problem is addressed by splitting tracefs_hist_add_sort_key()
>> into two methods - one that overwrite and one that does not.
>>
>> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
>> ---
>>   include/tracefs.h  |  4 +++-
>>   src/tracefs-hist.c | 21 +++++++++++++++++++--
>>   2 files changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/tracefs.h b/include/tracefs.h
>> index 64fbb3f..c3fa1d6 100644
>> --- a/include/tracefs.h
>> +++ b/include/tracefs.h
>> @@ -303,7 +303,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, ...);
>> +			      char *sort_key);
> 
> Why did you remove the const? The add_sort_key() takes a const and makes a
> copy of it.

This is a mistake. It must be 'const'.

> 
>> +int tracefs_hist_sort_key(struct tracefs_hist *hist,
>> +			  const char *sort_key, ...);
>>   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 8501d64..2ea90d9 100644
>> --- a/src/tracefs-hist.c
>> +++ b/src/tracefs-hist.c
>> @@ -453,6 +453,23 @@ add_sort_key(struct tracefs_hist *hist, const char *sort_key, char **list)
>>   	return tracefs_list_add(list, sort_key);
>>   }
>>   
> 
> Needs a kerneldoc documentation header.

OK

> 
>> +int tracefs_hist_add_sort_key(struct tracefs_hist *hist,
>> +			      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_add_sort_key - add a key for sorting the histogram
> 
> The above name needs to be updated.
> 
>>    * @hist: The histogram to add the sort key to
>> @@ -464,8 +481,8 @@ add_sort_key(struct tracefs_hist *hist, const char *sort_key, char **list)
>>    *
>>    * Returns 0 on success, -1 on error.
>>    */
>> -int tracefs_hist_add_sort_key(struct tracefs_hist *hist,
>> -			      const char *sort_key, ...)
>> +int tracefs_hist_sort_key(struct tracefs_hist *hist,
> 
> How about if we call this:
> 
> 	tracefs_hist_replace_sort_keys()
> 
> I think that would be a more intuitive name.

The user may call this function with a histogram that has no sort keys added.
So there will be nothing to replace.
What about naming it 'tracefs_hist_set_sort_keys()'?

Thanks a lot!
Yordan

> 
> -- Steve
> 
>> +			  const char *sort_key, ...)
>>   {
>>   	char **list = NULL;
>>   	char **tmp;
>