Re: [PATCH 4/7] tracing/filters: allow user to specify a filterval to be string

[Frederic Weisbecker]

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@xxxxxxxxxxxxxx>
> ---
>  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
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/