On Sun, May 12, 2013 at 3:02 PM, Pascal Quantin
<pascal.quantin@xxxxxxxxx> wrote:
> 2013/5/12 Evan Huus <eapache@xxxxxxxxx>
>>
>> On Sun, May 12, 2013 at 12:34 PM, Anders Broman <a.broman@xxxxxxxxxxxx>
>> wrote:
>> > If the tap is extracting information from the packet and a dissector bug
>> > occurs before that
>> > information is extracted that information will always be missing from
>> > the
>> > tap so having
>> > "correct" information from a tap is impossible if there are dissector
>> > bugs -
>> > right?
>>
>> Correct, but the tap should still get what information there is for
>> that packet so that it can calculate statistics for the protocol (this
>> was bug 8321 - the tap wasn't even getting called for malformed
>> packets).
>>
>> On Sun, May 12, 2013 at 12:39 PM, Pascal Quantin
>> <pascal.quantin@xxxxxxxxx> wrote:
>> > Hi Evan,
>> >
>> > I'm not sure I understood your proposal properly.
>> > The issue with bug 8321 was that the tap was never called (due to the
>> > exception) and the issue with bug 8610 was that context information was
>> > missing when the tap was called.
>> > When looking at the comments in tap.h, it appears that
>> > tap_queue_packet() is
>> > designed to be called once that dissection of the current packet is
>> > finished
>> > (so that packet_info and the private data structures are completely
>> > filled).
>> > As we saw with bug 8321, it does not work if an exception occurs in the
>> > middle of the dissection.
>>
>> So this is very interesting - I took a closer look at the tap
>> internals and it's already doing what I was suggesting, but that means
>> I no longer understand bug 8610 at all.
>>
>> Here's my understanding of what happens: a dissector calls
>> tap_queue_packet with pointers to pinfo and some private data
>> structure. All this does is add the packet (and pointers) to a list -
>> the tap callback isn't actually called yet. Once the entire packet has
>> been dissected (and any appropriate exceptions caught),
>> tap_push_tapped_queue is called and takes care of actually calling all
>> the callbacks.
>>
>> Bug 8321 still makes sense - an exception was being thrown before the
>> packet was ever queued with tap_queue_packet. However, I'm no longer
>> following bug 8610. The packet queue only has pointers to the pinfo
>> and other structure, so it shouldn't matter when the tap gets queued -
>> more dissection of the same packet should only update the structures
>> being pointed to, so the taps should automatically see all the
>> necessary data.
>>
>> I don't understand why moving the tap_queue_packet earlier caused data
>> to be missing, since that data should have been available anyways by
>> my reading. The pointers passed to the tap callback didn't change, and
>> the actual point when the callback was called didn't change.
>>
>> What am I missing?
>
>
> I had another look at the code and the DTMF info that was missing in bug
> 8610 comes from the rtpevent tap and not from the rtp tap. The rtp listener
> needs the data from the rtpevent listener. That's why the order does matter.
Ah, OK. So that was actually a fairly special case then of
dependencies between taps. Does anyone know of other inter-tap
dependencies we should be aware of? If so, is it worth adding some
sort of simple dependency logic to the tap framework so that, for
example, the rtp tap can be added before rtpevent (without all the
messy try/catch logic) but still called afterwards?
Thanks,
Evan
