Messages in this thread Patch in this message | | | Date | Sat, 11 Apr 2009 16:35:34 +0200 | From | Frederic Weisbecker <> | Subject | Re: [PATCH 4/7] tracing/filters: allow user to specify a filter val to be string |
| |
Hi Li,
On Sat, Apr 11, 2009 at 03:53:05PM +0800, Li Zefan wrote: > Suppose we would like to trace all tasks named '123', but this > will fail: > # echo 'parent_comm == 123' > events/sched/sched_process_fork/filter > bash: echo: write error: Invalid argument > > With this patch, we allow it by: > # echo 'parent_comm == \123' > events/sched/sched_process_fork/filter > # cat events/sched/sched_process_fork/filter > parent_comm == 123
Well, IMHO, it would be rather better to just echo 'parent_comm == 123' and let it answer depending of which filter_pred_*() callback we have for the concerned field.
The culprit is this part in filter_parse():
pred->val = simple_strtoull(val_str, &tmp, 10); if (tmp == val_str) { pred->str_val = kstrdup(val_str, GFP_KERNEL); if (!pred->str_val) return -ENOMEM; }
The idea would be to not anymore base the check on simple_strtoull to guess whether this is a number or a string, making it act subsequently to this assumption, which is not the good assumption we must base our parsing yet.
Instead, we could let filter_parse only do the job of extracting the tokens and then fill the whole pred struct without yet bothering about the type of the value.
Thereafter we may defer the real value type checking on filter_add_pred() depending on the type of the concerned field:
if (is_string_field()) { add it as a string value; } else { do the check with simple_strtoull looks good? Then go to the number size switch.... ... }
You see?
I think it would be a saner basis of parsing.
Thanks, Frederic.
> > Signed-off-by: Li Zefan <lizf@cn.fujitsu.com> > --- > kernel/trace/trace_events_filter.c | 11 ++++++++++- > 1 files changed, 10 insertions(+), 1 deletions(-) > > diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c > index 49b3ef5..2bf4481 100644 > --- a/kernel/trace/trace_events_filter.c > +++ b/kernel/trace/trace_events_filter.c > @@ -351,12 +351,19 @@ oom: > return -ENOMEM; > } > > +/* > + * The filter format can be > + * - 0, which means remove all filter preds > + * - [||/&&] <field> ==/!= [\]<val> > + * > + * Note: '\' prevent a string type value beginning with a digit to > + * be treated as a number > + */ > int filter_parse(char **pbuf, struct filter_pred *pred) > { > char *tmp, *tok, *val_str = NULL; > int tok_n = 0; > > - /* field ==/!= number, or/and field ==/!= number, number */ > while ((tok = strsep(pbuf, " \n"))) { > if (tok_n == 0) { > if (!strcmp(tok, "0")) { > @@ -421,6 +428,8 @@ int filter_parse(char **pbuf, struct filter_pred *pred) > > pred->val = simple_strtoull(val_str, &tmp, 0); > if (tmp == val_str) { > + if (*val_str == '\\') > + val_str++; > pred->str_val = kstrdup(val_str, GFP_KERNEL); > if (!pred->str_val) > return -ENOMEM; > -- > 1.5.4.rc3 >
| |