Messages in this thread |  | | Date | Mon, 20 Jul 2009 08:59:18 +0800 | From | Li Zefan <> | Subject | Re: [PATCH] tracing/filters: Improve subsystem filter |
| |
>> -static void filter_free_subsystem_preds(struct event_subsystem *system) >> +/* >> + * flag == 0: remove all events' filter >> + * flag == 1: clear filter->no_reset >> + * flag == 2: remove all preds with no_reset == false >> + */ > > Once an option overlap the boolean binary range, it's better > to start thinking about an enum type, for better self-explaining code. >
Ok.
I was lazy to think of proper names for those enums..
>> +static void filter_free_subsystem_preds(struct event_subsystem *system, >> + int flag) >> { >> struct ftrace_event_call *call; >> >> @@ -428,6 +434,14 @@ static void filter_free_subsystem_preds(struct event_subsystem *system) >> if (!call->define_fields) >> continue; >> >> + if (flag == 1) { >> + call->filter->no_reset = false; >> + continue; >> + } >> + >> + if (flag == 2 && call->filter->no_reset == true) > > Or simply if (flag == 2 && call->filter->no_reset) >
Sure.
>> + continue; >> + >> if (!strcmp(call->system, system->name)) { >> filter_disable_preds(call); >> remove_filter_string(call->filter); >> @@ -529,7 +543,8 @@ static filter_pred_fn_t select_comparison_fn(int op, int field_size, >> >> static int filter_add_pred(struct filter_parse_state *ps, >> struct ftrace_event_call *call, >> - struct filter_pred *pred) >> + struct filter_pred *pred, >> + bool apply) > > > bool dry_run sounds perhaps more intuitive for what is happening there.
I guess you "try_run". ;)
> Because "apply" is surprising in a "add_thing" function, it's like > a field to confirm that we know what what we are doing :) > > (May be I start to become a PITA with my naming worries, especially > since I'm usually not good at it in my patches :) >
I'll take "try_run". Naming is often hard for me.
> Otherwise, the rest looks good. >
Thanks!
|  |