----- Original Message -----
From: "Guy Harris"
To: "Ronnie Sahlberg"
Sent: Friday, June 15, 2001 7:11 PM
Subject: Re: [Ethereal-dev] first patch for ip_to_str/tvb_get_ptr removal
> On Fri, Jun 15, 2001 at 04:28:21PM +1000, Ronnie Sahlberg wrote:
> > tvb_get_ptr() is not really compatible with COMPOSITEs since the current
use
> > of this function
> > will guarantee that each and every COMPOSITE will be g_malloc() and
memcpy()
> > to a backing buffer and turned into
> > a normal REAL_DATA tvbuff.
>
> No.
>
> That needs to happen only if the item being accessed by "tvb_get_ptr()"
> crosses a tvbuff boundary; if that's not what happens now, that's just a
> bug.
>
> (Note also that "tvb_get_ptr()" is, in effect, just a wrapper around
> "ensure_contiguous()", and that's the routine that calls
> "composite_ensure_contiguous()", which is the routine that, *if*
> "check_offset_length_no_exception()" returns "false", makes a contiguous
> copy. Unless you plan to change the *other* routines that use it,
> merely eliminating "tvb_get_ptr()" won't be sufficient.)
OK.
Currently this includes all IP, TCP, UDP, etc dissectors since they all do a
tvb_get_ptr() on the entire packet in order to
calculate the checksum.
This would cause all defragmented IP packets in a COMPOSITE to automatically
be expensively converted to a REAL_DATA tvbuff.
If COMPOSITEs are to be useful this calls for implementing tvb_ipchksum(tvb,
offset, len) and get rid of tvb_get_ptr() in these
cases. Adjustment for UDP/TCP pseudoheaders can be trivially implemented as
a wrapper inside packet-tcp packet-udp (which calls tvb_ipchksum() and
manually (trivially) adds the pseudoheader to the result).
file.c :
Each and every packet ethereal is ever going to display gets called with
tvb_get_ptr() for the entire packet in order to display the hex pane. This
guarantees that every COMPOSITE always will be converted to a REAL_DATA one.
My belief is that tvb_get_ptr() is evil, it makes it too easy to cut corners
as some sort of catch all when no suitable accessor exists.
Example, since no tvb_get_ether() exists all dissectors that neeeds to read
a 6-byte MAC address use tvb_get_ptr() instead.
This is one reason why I thnink tvb_get_ptr() should go.
The other is that longed lived COMPOSITEs become close to useless if
tvb_get_ptr() are allowed,since if the packet is gong to be converted to
a REAL_DATA tvbuff anyway just because someone called tvb_get_ptr() one
might as well just ignore using COMPOSITEs and
g_malloc()+memcpy() everything into a REAL_DATA from the beginning.
>
> > The ugliness with the current IP reassembly is that it does a lot of
> > g_malloc() and memcpy(). By using a COMPOSITE
> > for the reassembled packet, all these g_malloc() and memcpy() are
> > eliminated.
>
> Other than, of course, the "g_malloc()" after the
>
> /* If we have reached this point, the packet is not defragmented
yet.
> * Save all payload in a buffer until we can defragment.
> * XXX - what if we didn't capture the entire fragment due
> * to a too-short snapshot length?
> */
>
> comment in "fragment_add()", as you have to have the data for *all*
> fragments in a given fragmented IP datagram available when *any* frame
> containing a fragment from that datagram is accessed; the only
> alternative would be to read the data for that frame from the capture
> file, but that might make random access to the capture file sufficiently
> common that the fact that random access to frames *before* the current
> frame is currently *extremely* expensive on compressed files a
> significant problem.
My thought was to inexpensively perform something like
tvb_add_to_composite(composite, position, tvb, offset, len)
This would cause composite to reference tvb, thus tvb would stay around
until composite was deallocated.
At least all g_malloc()+memcpy() would be eliminated.
IP reassembly would be much nicer if the tvbuffs from the fragment could be
put into a COMPOSITE instead of put in temporary buffers.
If the data is going to be remembered across packets anyway, why copy it to
temporary buffers? why not just keep the tvbuffs aroung a little longer than
usual?
>
> (There are, I think, ways of making it sufficiently inexpensive,
> involving replacing zlib with, say, modified zlib or gunzip code that
> works similarly to the way the compressed-Sniffer code works, but they
> haven't been implemented yet.)
>
> > If you use ip reassembly on say NFS v3/UDP reading/writing large files,
a
> > significant number of the captured packets will be ip fragments,
> > thus this would become a problem.
>
> But the composite tvbuff in question should be freed when you're done
> with the protocol tree for the reassembled packet.
>
> > An application to map all segments in a TCP sequence into a COMPOSITE
tvbuff
> > (TCP reassembly, ~~treat TCP session as sequence of bytes) would make
the
> > backing REAL_DATA tvbuffs very long lived. That would make them live
until a
> > new capture was started/loaded.
>
> An application to map *ALL* segments in a TCP sequence into a COMPOSITE
> tvbuff will, no matter *HOW* you do it, require that all those segments
> fit into memory. I have little doubt that there are capture files on
> which that will fail even if you *don't* create a single contiguous
> buffer containing all of them - capture files that currently *can* be
> read by ethereal.
Yes, all referenced tvbuffs as well as the real_data buffers for them will
effectivly be locked into memory until the composite is freed.
This requires the same memory storage as the current IP reassembly routine.
Currently it keeps this data around by creating dynamic buffers and copying
the data into them, with COMPOSITEs
no new buffers would be allocated, no memory would be copied.
