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

["Lars Ruoff" <Lars.Ruoff@xxxxxxxxxx>]

Any other comments on this?

Lars

----- Original Message ----- 
From: "Lars Ruoff" <lars.ruoff@xxxxxxx>
To: "Ethereal-Dev" <ethereal-dev@xxxxxxxxxxxx>
Sent: Monday, March 28, 2005 5:26 PM
Subject: [Ethereal-dev] Re: Order of tap listeners (PATCH)


>
> 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
> > > >
> > >
> > >
> >
> >
>
> _______________________________________________
> Ethereal-dev mailing list
> Ethereal-dev@xxxxxxxxxxxx
> http://www.ethereal.com/mailman/listinfo/ethereal-dev