On Thu, 12 Oct 2023 16:37:35 -0400
Nick Lowell <nicholas.low...@gmail.com> wrote:
Sorry, I was traveling when this was sent, and I missed it.
> I really appreciate the continued feedback. I was able to reproduce.
> I think I'm understanding better but still need some help.
> I am actually wondering if remove_filter_string(system->filter) should
You mean to return true if filter->filter_string was not NULL?
> also return bool as an OR'd input for sync.
> Should it be something like this?
>
> if (!strcmp(strstrip(filter_string), "0")) {
> - filter_free_subsystem_preds(dir, tr);
> - remove_filter_string(system->filter);
> + bool sync;
I would just make this an int;
> +
> + sync = filter_free_subsystem_preds(dir, tr);
> + sync = sync || remove_filter_string(system->filter);
And then just have:
sync |= remove_filter_string(system->filter);
> filter = system->filter;
> system->filter = NULL;
> - /* Ensure all filters are no longer used */
> - tracepoint_synchronize_unregister();
> + /* If nothing was freed, we do not need to sync */
> + if(sync) {
> + /* Ensure all filters are no longer used */
> + tracepoint_synchronize_unregister();
> + }
> filter_free_subsystem_filters(dir, tr);
> __free_filter(filter);
> goto out_unlock;
>
> > Maybe even pass in "sync" to the filter_free_subsystem_filters() to make
> > sure there were nothing to be freed, and do the WARN_ON_ONCE() then.
> >
> > __free_filter(filter);
> > goto out_unlock;
> > }
> >
> > -- Steve
>
> I'm not sure if I see the reasoning for the WARN_ON_ONCE() in
> filter_free_subsystem_filters()
> because it ends up checking the same if(!filter) just like
> filter_free_subsystem_preds() did earlier. It doesn't
> seem to do anything with system->filter. I actually wonder if !sync,
> could filter_free_subsystem_filters()
> be skipped altogether. Help me if I'm missing something.
The point is, code always changes. It's a bug if one of the filters had
content in filter_free_subsystem_filters() and sync is 0, hence the
WARN_ON_ONCE() if it does.
WARN_ON*()s are added to make sure the code is acting the way it is expected
to act. Yes, it should never trigger, but if it does, we know there's a bug
somewhere.
-- Steve
