Ethereal-dev: [Ethereal-dev] Re: Order of tap listeners (PATCH)

Note: This archive is from the project's previous web site, ethereal.com. This list is no longer active.

Date Index Thread Index Other Months All Mailing Lists
Date Prev Date Next Thread Prev Thread Next
From: "Lars Ruoff" <lars.ruoff@xxxxxxx>
Date: Mon, 28 Mar 2005 17:26:04 +0200

Hi Ronnie,
thanks for the proposal, but consider this:
My problem is not with the order of how 2 *different* tap listeners (tap_A and tap_B) are called. But my problem is with the *same* tap listener (tap_A) getting called twice for a frame which carries 2 times a packet of a certain kind (i.e. packet_A)! In the actual implementation, the tap is called twice, one time for each packet, but in *reverse* order of which the packets were dissected! This is problematic for taps that manage state info between successive packets of same kind (example: rtp_analysis). My patch fixes this by assuring at least the right order (=order of dissection) in this case. 


Is there actually tap listener features you are planning that would
need a defined order of calling of tap listeners or is it just a
generic request for 'it-might-be-useful'.
I'm working on an experimental SRP (Spectralink)-dissector that carries multiple occurences of RTP packets in a single frame, and i would like to be able to use the rtp_analysis tap on these SRP-"embedded" RTP packets. Currently (without the proposed patch), rtp_analysis gets stuck because it sees the packets in wrong order!
But i think this is would be usefull for *any* taps that are likely to get called for multiple packets in the same frame. 

regards,
Lars Ruoff


Message: 23
Date: Mon, 28 Mar 2005 07:28:43 -0400
From: ronnie sahlberg <ronniesahlberg@xxxxxxxxx>
Subject: [Ethereal-dev] Re: Order of tap listeners (PATCH)
To: Ethereal development <ethereal-dev@xxxxxxxxxxxx>
Message-ID: <c9a3e454050328032842c68a76@xxxxxxxxxxxxxx>
Content-Type: text/plain; charset=ISO-8859-1

I dont think that would work.

There is no guarantee that a dissector calls the tap framework after
it has called its subdissectors instead of before calling them.

I think instead you should go for a different approach.

First,  whan an application is registering a tap listener, this is
likely a very infrequent call so performance would be non-critical
(happens when user selects something from the menu).
But when the framework needs to call every listener for queued data
this would a frequent function (happens after potentially every
packet)

So we have to make sure the second case is fast while it doesnt matter
much if the first case is fast or not.


proposal :

Code a patch that adds a priority field to the tap listener list.
Add a comment that when new entries are inserted into the list it is
important that the ordered-by-priority property of the list is
maintained  or else undefined behaviour is invoked. (and ethereal
execv() nethack)

Add a priority parameter as such to the register_tap_listener parameters.
Change all existing tap listnerers to pass 0 as the priority parameter.

Inside the tap framework, keep using the existing list but instead of
always adding new tap listeners to the head of the list, change this
function to add the tap listener to be the first one in the list with
the given priotiry or higher.
I.e. change register_tap_listener to always insert new tap listeners
so that the list is always sorted by priority.

This will make register_tap_listener() slightly slower than before
since it might have to walk a few entries in the list before it can
add the entry.
Since most tap listeners will register with priority==0 this is a
non-issue since these entries would be at the head of the list anyway
and no walking the list is required.


This also means that tap_push_tapped_queue()
will still be a simple walk a simple linked list once and thus still
be fast (which is good since this function is time critical).


Then document something like :
The tap listeners will be called by
priority in incrementing order. First all priority==0 listeners will
be called, second all priority 1 listeners etc etc.
Please note that the order is only defined for listeners of different
priority. Within a priority level the order in which listeners are
called is undefined.


This would work and still be fast.
It would be simple.
The change to the code would be minimal.
I.e.  three good things in one patch:-)
This would also allow the possibility of specifying explicit call
order for tap listeners if so required.



Is there actually tap listener features you are planning that would
need a defined order of calling of tap listeners or is it just a
generic request for 'it-might-be-useful'.




On Sat, 19 Mar 2005 22:14:36 +0100, Lars Ruoff <lars.ruoff@xxxxxxx> wrote:
>
> If no one objects, could we apply the following patch to tap.c, which
> brings
> the tapping in order with the packet dissection?
>
> tap.c:
> - Replaced fixed length linked list of tap_packet_t with static array.
> We advance in the array with every call to tap_queue_packet.
> In tap_push_tapped_queue we iterate over all queued packets in the same
> order.
> => The order of which the tap listeners will be called is the same as > the 
> order of which  the corresponding dissectors for the packet have been
> called. (README.tapping to be updated).
>
> Attached is relevant output of 'svn diff'.
>
> regards,
> Lars Ruoff
>
>
> > README.tapping says:
> > "After each individual packet has been completely dissected and all
> > dissectors have returned, all the tap listeners that has been flagged
> > to receive tap data during the dissection of the frame will be called > > in 
> > sequence.
> > The order of which the tap listeners will be called is not defined."
> >
> > Why can't we just define the order as the same order of the sequential
> calls
> > to tap_queue_packet, at least for a given tap listener?
> > That would help a lot with a statefull tap listeners that inspect
> multiple
> > packets beeing transported in the same frame.
> > Currently, i'm trying to tap on RTP packets over a Wifi-Connection > > where > > multiple RTP packets are encpasulated in a radio transport protocol. > > But 
> the
> > RTP tap listener sees the order mangled up and wrongly detects > > sequence 
> > errors.
> >
> > regards,
> > Lars Ruoff
> >
>
>