On Mon, 27 Nov 2017 16:23:12 -0800
Andi Kleen <a...@firstfloor.org> wrote:
> From: Andi Kleen <a...@linux.intel.com>
>
> Add a configurable node size to strlist, which allows users
> to store additional data in a str_node. Also add a new interface
> to add a new strlist node, and return the node, so additional
> data can be added.
>
> Signed-off-by: Andi Kleen <a...@linux.intel.com>
> ---
> tools/perf/util/rblist.c | 16 ++++++++++++++--
> tools/perf/util/rblist.h | 2 ++
> tools/perf/util/strlist.c | 15 ++++++++++++++-
> tools/perf/util/strlist.h | 8 ++++++++
> 4 files changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/rblist.c b/tools/perf/util/rblist.c
> index 0dfe27d99458..fa221e6b0932 100644
> --- a/tools/perf/util/rblist.c
> +++ b/tools/perf/util/rblist.c
> @@ -11,11 +11,13 @@
>
> #include "rblist.h"
>
> -int rblist__add_node(struct rblist *rblist, const void *new_entry)
> +int rblist__add_node_ptr(struct rblist *rblist, const void *new_entry,
> + struct rb_node **nodep)
> {
> struct rb_node **p = &rblist->entries.rb_node;
> struct rb_node *parent = NULL, *new_node;
>
> + *nodep = NULL;
> while (*p != NULL) {
> int rc;
>
> @@ -26,13 +28,16 @@ int rblist__add_node(struct rblist *rblist, const void
> *new_entry)
> p = &(*p)->rb_left;
> else if (rc < 0)
> p = &(*p)->rb_right;
> - else
> + else {
> + *nodep = parent;
> return -EEXIST;
> + }
> }
>
> new_node = rblist->node_new(rblist, new_entry);
> if (new_node == NULL)
> return -ENOMEM;
> + *nodep = new_node;
>
> rb_link_node(new_node, parent, p);
> rb_insert_color(new_node, &rblist->entries);
> @@ -41,6 +46,13 @@ int rblist__add_node(struct rblist *rblist, const void
> *new_entry)
> return 0;
> }
>
> +int rblist__add_node(struct rblist *rblist, const void *new_entry)
> +{
> + struct rb_node *nd;
> +
> + return rblist__add_node_ptr(rblist, new_entry, &nd);
> +}
> +
> void rblist__remove_node(struct rblist *rblist, struct rb_node *rb_node)
> {
> rb_erase(rb_node, &rblist->entries);
> diff --git a/tools/perf/util/rblist.h b/tools/perf/util/rblist.h
> index 4c8638a22571..2941e4295f63 100644
> --- a/tools/perf/util/rblist.h
> +++ b/tools/perf/util/rblist.h
> @@ -31,6 +31,8 @@ struct rblist {
> void rblist__init(struct rblist *rblist);
> void rblist__delete(struct rblist *rblist);
> int rblist__add_node(struct rblist *rblist, const void *new_entry);
> +int rblist__add_node_ptr(struct rblist *rblist, const void *new_entry,
> + struct rb_node **nodep);
> void rblist__remove_node(struct rblist *rblist, struct rb_node *rb_node);
> struct rb_node *rblist__find(struct rblist *rblist, const void *entry);
> struct rb_node *rblist__findnew(struct rblist *rblist, const void *entry);
> diff --git a/tools/perf/util/strlist.c b/tools/perf/util/strlist.c
> index 9de5434bb49e..68ef21c3797c 100644
> --- a/tools/perf/util/strlist.c
> +++ b/tools/perf/util/strlist.c
> @@ -18,7 +18,7 @@ struct rb_node *strlist__node_new(struct rblist *rblist,
> const void *entry)
> const char *s = entry;
> struct rb_node *rc = NULL;
> struct strlist *strlist = container_of(rblist, struct strlist, rblist);
> - struct str_node *snode = malloc(sizeof(*snode));
> + struct str_node *snode = malloc(strlist->node_size);
>
> if (snode != NULL) {
> if (strlist->dupstr) {
> @@ -66,6 +66,14 @@ int strlist__add(struct strlist *slist, const char
> *new_entry)
> return rblist__add_node(&slist->rblist, new_entry);
> }
>
> +struct str_node *strlist__add_node(struct strlist *slist, const char
> *new_entry)
> +{
> + struct rb_node *nd;
> +
> + rblist__add_node_ptr(&slist->rblist, new_entry, &nd);
> + return container_of(nd, struct str_node, rb_node);
> +}
It should check the result of rblist__add_node_ptr() and return NULL if an
error.
> +
> int strlist__load(struct strlist *slist, const char *filename)
> {
> char entry[1024];
> @@ -165,11 +173,15 @@ struct strlist *strlist__new(const char *list, const
> struct strlist_config *conf
> bool dupstr = true;
> bool file_only = false;
> const char *dirname = NULL;
> + size_t node_size = sizeof(struct str_node);
>
> if (config) {
> dupstr = !config->dont_dupstr;
> dirname = config->dirname;
> file_only = config->file_only;
> + node_size = config->node_size;
> + if (!node_size)
> + node_size = sizeof(struct str_node);
This is dangerous, node_size can be smaller than sizeof(struct str_node).
Instead, why don't we use "data_size" and set additional data size?
Also, we can add "u8 data[0]" at the end of struct str_node, so that
user can easily access the appended data.
> }
>
> rblist__init(&slist->rblist);
> @@ -179,6 +191,7 @@ struct strlist *strlist__new(const char *list, const
> struct strlist_config *conf
>
> slist->dupstr = dupstr;
> slist->file_only = file_only;
> + slist->node_size = node_size;
>
> if (list && strlist__parse_list(slist, list, dirname) != 0)
> goto out_error;
> diff --git a/tools/perf/util/strlist.h b/tools/perf/util/strlist.h
> index d58f1e08b170..fd407e11e124 100644
> --- a/tools/perf/util/strlist.h
> +++ b/tools/perf/util/strlist.h
> @@ -16,25 +16,33 @@ struct strlist {
> struct rblist rblist;
> bool dupstr;
> bool file_only;
> + size_t node_size;
> };
>
> /*
> * @file_only: When dirname is present, only consider entries as filenames,
> * that should not be added to the list if dirname/entry is not
> * found
> + * @node_size: Allocate extra space after str_node which can be used for
> other
> + * data. This is the complete size including str_node
> */
> struct strlist_config {
> bool dont_dupstr;
> bool file_only;
> const char *dirname;
> + size_t node_size;
> };
>
> +#define STRLIST_CONFIG_DEFAULT \
> + { false, false, NULL, sizeof(struct str_node) }
> +
> struct strlist *strlist__new(const char *slist, const struct strlist_config
> *config);
> void strlist__delete(struct strlist *slist);
>
> void strlist__remove(struct strlist *slist, struct str_node *sn);
> int strlist__load(struct strlist *slist, const char *filename);
> int strlist__add(struct strlist *slist, const char *str);
> +struct str_node *strlist__add_node(struct strlist *slist, const char *str);
Hmm, I see this is much more efficient, but from the programming point of view,
we can also use the combination of strlist__add() and strlist__find() for
that purpose.
BTW, what happen if we already have same string on strlist?
Thank you,
>
> struct str_node *strlist__entry(const struct strlist *slist, unsigned int
> idx);
> struct str_node *strlist__find(struct strlist *slist, const char *entry);
> --
> 2.13.6
>
--
Masami Hiramatsu <mhira...@kernel.org>
