Messages in this thread | | | Date | Sun, 12 Apr 2009 12:04:50 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH 4/7] tracing/filters: allow user to specify a filter val to be string |
| |
* Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 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.
Agreed. The user input itself is unambigously parse-able - and we should not work around parsing limitations by complicating the user input.
The failure situation that Li found should obviously be fixed.
Ingo
| |