Ronnie Sahlberg wrote:
> Very interesting work.
> Though you access the tap : "tap" which have not been implemented yet, did
> you forget to attache the diff to packet-tcp.c?
Hmm... I included the physical file and not a patch for it. I'll have to look
into CVS diff's for directories.
> One comment, right now you seem not to use the facility for providing tap
> specific data using the
> fourth parameter to _packet().
> For much better performance and also to provide a mechanism for others that
> want to use the "tcp"
> tap point, maybe you should consider to change packet-tcp.[c|h] so that they
> have a
> definition of a tcp_header struct which is filled in by the tcp dissector.
> Then a pointer to this (static, must not live on the stack) structure could
> be passed to any tap-listeners
> listening on the "tcp" tap.
> You would then not need to do those find_first_integer() things in _packet()
> anymore, you could just cast
> the void*pri parameter to tcp_header* and just access the fields
> directly.
Very good point. I think I based the coding style off of tap-iostat.c, which by
necessity needs to operate a bit differently. I aimed to implement it quickly,
but I will try and rewrite it to work properly. (One side note: I believe
iostat requires that you include the thing you're looking for in the filter
expression... doing something like what I did for tcp parameters would eliminate
that requirement... it might make it more convenient for those who use iostat)
> Can you provide a patch for packet-tcp.[ch] that implements a tcp_header
> structure
> and that dissect_tcp() fills it in and then passes it to the tap system for
> listeners to access?
> Remember that the struct in dissect_tcp() holding the header must not live
> on the stack since the stackframe of dissect_tcp() does no longer exist when
> the tap listeners are called.
> (tcp_header i guess could just ignore any and all the options of the header)
I'll try to do that in the near future. I'll also try and take a look and see
what the tcp analysis uses. Maybe that whole thing (tcp sequence number
analysis) can be moved out to a tap (eventually). At the very least, it would
be a nice example tap to have. It adds to the protocol tree and to the protocol
description... features others may be interested in.
(I have further to say as side commentary here, but I think I'll save it for a
distinct e-mail)
> The second patch could be your current one for tcp,close reworked to utilize
> the fourth parameter
> in _packet().
I'll try to do that in the near future... :)
