| Message ID | 20210924095702.151826-3-y.karadz@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Modifications of some 'hist' APIs | expand |
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. > 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
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 >
On Fri, 24 Sep 2021 19:56:41 +0300 Yordan Karadzhov <y.karadz@gmail.com> wrote: > > 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. Looking at this now, I think "set" is a better name. I'll modify it. -- Steve
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, ...); 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;
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(-)