Re: [PATCH v4 2/7] kernel-shark-qt: Add Plugin event handlers to session.

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 20 Sep 2018 16:29:00 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> On 20.09.2018 06:24, Steven Rostedt wrote:
> >> static size_t get_records(struct kshark_context *kshark_ctx,
> >>   			  struct rec_list ***rec_list, enum rec_type type)
> >>   {
> >> +	struct kshark_event_handler *evt_handler;
> >>   	struct event_filter *adv_filter;
> >>   	struct kshark_task_list *task;
> >>   	struct tep_record *rec;
> >> @@ -608,6 +618,17 @@ static size_t get_records(struct kshark_context *kshark_ctx,
> >>   
> >>   				entry = &temp_rec->entry;
> >>   				kshark_set_entry_values(kshark_ctx, rec, entry);
> >> +
> >> +				/* Execute all plugin-provided actions (if any). */
> >> +				evt_handler = kshark_ctx->event_handlers;
> >> +				while ((evt_handler = find_event_handler(evt_handler,
> >> +									 entry->event_id))) {
> >> +					evt_handler->event_func(kshark_ctx, rec, entry);
> >> +
> >> +					if ((evt_handler = evt_handler->next))  
> > Please break the above if, to remove the assignment. It's just easier
> > to read. That is:
> > 
> > 					event_handler = event_hanndler->next;
> > 					if (event_handler)  
> >> +						entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK;
> >> +				}
> >> +  
> 
> Actually this is a bug (typo). I meant to have

 :-)

That's actually one of the reasons the combination is looked down upon
in the Linux kernel. It's more likely to hide bugs from reviewers.

-- Steve

> 
> 					evt_handler = evt_handler->next;
> 					if (!evt_handler)
> 						entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK;
> 
> This way the KS_PLUGIN_UNTOUCHED flag gets unset only once and it is 
> guaranteed that the value of the bit cannot be changed by the plugin 
> callback function.
>