| Message ID | 20210910163857.324696-3-y.karadz@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Modifications of some 'hist' APIs | expand |
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. > +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. > +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. -- Steve > + const char *sort_key, ...) > { > char **list = NULL; > char **tmp;
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. If I'm building a histogram via some interactive method, and have created a histogram instance. How do I add a new key before executing it? -- Steve
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; >
On 10.09.21 г. 23:04, 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. > > If I'm building a histogram via some interactive method, and have created a > histogram instance. How do I add a new key before executing it? Is this comment about adding 'key' or about adding 'sort key'? Thanks! Yordan > > -- Steve >
On Mon, 13 Sep 2021 15:26:51 +0300 Yordan Karadzhov <y.karadz@gmail.com> wrote: > > 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()'? Nothing is something to replace? Or we remove this completely, and have a: tracefs_hist_reset_sort_keys() Which removes all sort keys, and let you to start with a clean plate. Thus, the above would really just be "reset" followed by "add". -- Steve
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); +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); } +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 * @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, + 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 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(-)