[asterisk-dev] [Code Review] Add regular expression blacklist filtering for manager events

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/673/#review2052
-----------------------------------------------------------


First and foremost, this change requires a documentation update to manager.conf.sample. Because of a lack of knowing what the feature is intended to do, exactly, some of my comments in the diff may actually be off-base.


/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/673/#comment4338>

    I'm getting mixed messages from this diff. First off, we have this file-scoped list called all_filters, which contains every filter specified for each user. Then, each user also has a list of filters which contain the filters that were specified in that user's category in manager.conf.
    
    All regex matching appears to be using the all_filters list, and the user->filter is never used at all. This means that if user Alice has regex filter A specified and Bob has regex filter B specified, then Bob will receive events that match either regex A or  regex B. Same goes for Alice as well. It seems to me this is an oversight and that in reality, Alice should only get events that match regex A and Bob should only get events that match regex B.



/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/673/#comment4340>

    Since a user may have multiple filters in place, it seems like it would make more sense to have an AST_LIST_HEAD_NOLOCK of event_filters here rather than just having a single event_filter pointer and having to traverse them using AST_LIST_NEXT.



/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/673/#comment4336>

    This function seems a bit off to me. Instead of traversing the all_filters list, it seems more sensible to walk the filters that are associated with s->session.



/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/673/#comment4335>

    The logic in this if statement seems to be reversed. match_filter returns 1 if there is a match, and this if statement appears to send the event if match_filter returns 0.



/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/673/#comment4337>

    This seems a bit strange to me. Why are all the filters getting erased if a user is found when parsing the file? Why not just delete the filters associated with the particular user?



/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/673/#comment4341>

    Minor nitpick: we typically try not to name variables with names that are C++ keywords. Instead of "new" use something like "new_filter"



/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/673/#comment4339>

    This is just...bizarre. Let's say that we have a single user that has two filters specified in his section of manager.conf and trace what happens. When we come across the filter1, new is filter 1, new->next will point to NULL, filter points to new (which is filter1), and then new gets put at the end of the all_filters list. So far so good; we have a single filter in a linked list and its next pointer points to NULL. Then, when filter2 is found in this user's category, new is filter 2, new->next points to filter, which is filter 1. Then filter 2 gets added to the end of the all_filters list, which already has filter1 in it. Adding filter2 to the end of the list causes filter1's next field to point to the new filter. At this point, if you tried to traverse all_filters, you would loop infinitely since both filters' next pointers point to the other filter in the list.


- Mark


On 2010-05-20 18:42:50, Jeff Peeler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/673/
> -----------------------------------------------------------
> 
> (Updated 2010-05-20 18:42:50)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> From the submitter:
> to allow better filtering of ami-events than configuring categories, I created an option "eventfilter" for manager-users. Events to be sent are matched against regexes specified in these options and are discarded if the regex matches (blacklisting).
> 
> 
> This addresses bug 14861.
>     https://issues.asterisk.org/view.php?id=14861
> 
> 
> Diffs
> -----
> 
>   /trunk/main/manager.c 264829 
> 
> Diff: https://reviewboard.asterisk.org/r/673/diff
> 
> 
> Testing
> -------
> 
> I ensured manager events are reported the same without filtering and works as expected with.
> 
> 
> Thanks,
> 
> Jeff
> 
>