| Message ID | 20210812005546.910833-2-rostedt@goodmis.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libtracefs: Make hist variable names unique | expand |
On 12.08.21 г. 3:55, Steven Rostedt wrote: > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> > > The 'hist' triggers expect that all variables are unique. If two synthetic > events are created, it is possible that they will use the same variable > names, and this can break the logic for synthetic events. Add a random > number to the argument names that will help prevent that from happening. > There's no guarantee that there wont be collisions, but the chances of > that happening is 1 in 65535. If this is a problem, we could possibly look > for variables that are already in use. > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > --- > src/tracefs-hist.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/src/tracefs-hist.c b/src/tracefs-hist.c > index 7f9cf3820611..8783ef4364bd 100644 > --- a/src/tracefs-hist.c > +++ b/src/tracefs-hist.c > @@ -13,6 +13,8 @@ > #include <errno.h> > #include <fcntl.h> > #include <limits.h> > +#include <time.h> > +#include <sys/types.h> > > #include "tracefs.h" > #include "tracefs-local.h" > @@ -558,6 +560,7 @@ struct tracefs_synth { > unsigned int end_parens; > unsigned int end_state; > int *start_type; > + char arg_name[16]; > int arg_cnt; > }; > > @@ -957,7 +960,15 @@ static char *new_arg(struct tracefs_synth *synth) > char *arg; > int ret; > > - ret = asprintf(&arg, "__arg__%d", cnt); > + /* Create a unique argument name */ > + if (!synth->arg_name[0]) { > + srand(time(NULL)); Nit: Have in mind that time(NULL) has 1 second resolution. Fast consecutive calls (within a second) of this function can generate identical random numbers. This can be mitigated if we do something like this: struct timeval now; gettimeofday(&now, NULL); srand(now.tv_usec); -- Yordan > + ret = rand() + gettid(); > + /* Truncate so that ret is at most 65535 (total 13 bytes) */ > + ret &= (1 << 16) - 1; > + sprintf(synth->arg_name, "__arg_%d_", ret); > + } > + ret = asprintf(&arg, "%s%d", synth->arg_name, cnt); > if (ret < 0) > return NULL; > >
On Thu, 12 Aug 2021 11:34:57 +0300 Yordan Karadzhov <y.karadz@gmail.com> wrote: > > @@ -957,7 +960,15 @@ static char *new_arg(struct tracefs_synth *synth) > > char *arg; > > int ret; > > > > - ret = asprintf(&arg, "__arg__%d", cnt); > > + /* Create a unique argument name */ > > + if (!synth->arg_name[0]) { > > + srand(time(NULL)); > > Nit: Have in mind that time(NULL) has 1 second resolution. Fast consecutive calls (within a second) of this function can > generate identical random numbers. > This can be mitigated if we do something like this: > > struct timeval now; > > gettimeofday(&now, NULL); > srand(now.tv_usec); So you are saying that if one thread created two synthetic events within a second, then this could give the same value. Yeah, I can see that could happen. I was hoping to avoid the declaring the "now" and calling gettimeofday(). Also, looking more into this, I see that rand() is not safe in thread context (it may not be a problem, but there's no guarantee), and perhaps we should just open code it, to be on the safe side. Thanks for the review. -- Steve
diff --git a/src/tracefs-hist.c b/src/tracefs-hist.c index 7f9cf3820611..8783ef4364bd 100644 --- a/src/tracefs-hist.c +++ b/src/tracefs-hist.c @@ -13,6 +13,8 @@ #include <errno.h> #include <fcntl.h> #include <limits.h> +#include <time.h> +#include <sys/types.h> #include "tracefs.h" #include "tracefs-local.h" @@ -558,6 +560,7 @@ struct tracefs_synth { unsigned int end_parens; unsigned int end_state; int *start_type; + char arg_name[16]; int arg_cnt; }; @@ -957,7 +960,15 @@ static char *new_arg(struct tracefs_synth *synth) char *arg; int ret; - ret = asprintf(&arg, "__arg__%d", cnt); + /* Create a unique argument name */ + if (!synth->arg_name[0]) { + srand(time(NULL)); + ret = rand() + gettid(); + /* Truncate so that ret is at most 65535 (total 13 bytes) */ + ret &= (1 << 16) - 1; + sprintf(synth->arg_name, "__arg_%d_", ret); + } + ret = asprintf(&arg, "%s%d", synth->arg_name, cnt); if (ret < 0) return NULL;