----- Original Message -----
From: "Guy Harris"
To: "Ronnie Sahlberg" Cc: <ethereal-dev@xxxxxxxxxxxx>
Sent: Friday, June 15, 2001 5:13 AM
Subject: Re: [Ethereal-dev] first patch for ip_to_str/tvb_get_ptr removal
> > If ethereal were to start using COMPOSITEs this would greatly increase
the
> > emory footprint of ethereal
>
> I.e., Ethereal uses so little memory that merely allocating a chunk of
> memory to put the data behind *one* COMPOSITE tvbuff into a single
> contiguous buffer will greatly increase Ethereal's memory footprint?
>
> (Note that tvbuffs do not, and are not supposed to, live forever, and
> when they're freed - which should happen as soon as Ethereal frees up
> the "epan_dissect_t" for the frame it's dissecting, which should be
> after it's done processing the protocol tree or after you select a
> different frame - the allocated buffer will be freed; if it doesn't get
> freed, that's a bug.
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.
tvb_get_ptr() is also a unaesthetical backdoor into the packet. I think more
and more that it should be removed
thus forcing all access to the packet payload be used through proper
accessors.
I want COMPOSITEs to be useful and I plan to use COMPOSITEs, but I belive
that usage of tvb_get_ptr() potentially
can make any COMPOSITE usage too expensive and useless. This is why I want
tvb_get_ptr() to go away.
Currently COMPOSITEs are not used at all inside ethereal so currently this
would
not be an issue. However, if COMPOSITEs are to be used in the future then I
think
this will become an issue.
TVBUFFs are freed when their usage count reaches zero and this is when the
real_data
buffer (for TVBUFF_REAL_DATA) should also be g_free()d.
This is currently not the case but this does not matter since noone uses
COMPOSITEs.
However, when using COMPOSITEs, the usage count for the backing
TVBUFF_REAL_DATA
tvbuffs will increase since they now also are referenced by a COMPOSITE,
this means that when
COMPOSITEs are used, some REAL_DATA tvbuffs will live a little longer.
Consider the IP reassembly if this would be implemented as COMPOSITEs.
This would add the tvbuff's for the individual fragments to the COMPOSITE
representing the
defragmented packet. Since the COMPOSITE references the backing REAL_DATA
tvbuffs, their usage count
is incremented and thus the REAL_DATA tvbuff's will live at least until the
COMPOSITE is freed (which
would then automatically free all the backing REAL_DATA tvbuffs).
So even today with the current implementation, tvbuffs can live a lot longer
than just during the dissection of the current packet
if they are appended to a COMPOSITE.
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.
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.
Another problem is that tvb_get_ptr() is used quite frequently,
tvb_get_ptr() will currently cause all packets that are COMPOSITEs
to be memcpy() to a g_malloc() buffer. See file.c, or the header
checksumming in ip, etc.
I think that if COMPOSITEs are ever going to be used, then one needs to get
rid of all tvb_get_ptr() calls outside tvbuff.c
and make the function static.
I am prepared to do this work.
>
> I.e., it's not the case that memory will fill up with those chunks of
> memory.)
I can think of several applications of COMPOSITEs which will make the
backing REAL_DATA tvbuffsmuch longer lived than
they are today.
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.
Please reconsider to apply the patch. I really want to get rid of
tvb_get_ptr() and celaning up and removing the instances when it is
used together with ip_to_str() is the first step.
best regards
ronnie sahlberg
