ronnie sahlberg wrote:
List
during my crusade to ememify the tvb_get_string() (and friends such as
tvb_fake_unicode(), tvb_get_text(), tvb_get_stringz())
Hmmm, I found the function names chosen are very unintuitive at all. I
wouldn't know without reading the docs the difference between these
functions. And even worse the name tvb_fake_unicode() seems to be
completely odd in this row (at least to me). Should this be something
like tvb_get_faked_unicode() ?
i have found several obvious memleaks (and a lot more nonobvious like
where an exception is thrown between where tvb_get_string() is called
and before g_free()) which is good.
Handling the memleaks caused by exceptions isn't really obvious.
However, as it's name implies it should be an exception ...
And yes, preventing problems like this is a good thing :-)
Something that would imho be good since the number of both obvious and
non-obvious memleaks for these calls show that the api in using them
is too complex.
No. The problem is not complexity of the functions (their job is quite
simple) but simply wrong *naming* of the function(s) is the problem. The
function named tvb_get_string() gives no real hint that you have to free
the string by yourself afterwards.
As an opposite, the g_strdup named functions from glib (although glib is
itself a bit inconsistent in naming) makes it pretty clear that
something is duplicated and gives at least a hint to take care about the
function output. So if we had used something like tvb_strdup() would
have probably reduced our problems a lot on this topic. Of course, this
function name would be misleading with the new functionality you are
going to provide.
And again: Never underestimate the power of function and variable naming
to avoid such problems.
On the other hand, having at least four functions to get a string from a
tvb maybe proof you're probably right that the API is complex but for a
different reason :-)
it would probably also speed up ethereal a tiny tiny fraction reducing
the amount of g_free() calls made but that is likely insignificant.
I would think the performance gain will be minimal, but a step in the
right direction and better than nothing ;-)
once this is finished i plan not to have any explicit
ep_tvb_get_string() calls anymore. only the tvb_get_string() and
friends but with a different semantic on how/when memory is freed.
Since so very few functions actually need tvb_get_string() data with
longer than to the end of dissecting the current packet semantics
those other very few users can just do their own g_strdup()/g_free()
or when added ec_alloc() call (like ep_alloc() but with lifetime
until new capture is loaded).
comments?
The problem seems to be common and your solution to it seems reasonable.
I'm happy that someone takes a look at these things.
BTW: One could expect that the string will last as long as the tvb
exists. This was an idea I had when reading your mail. Why not attach
the strings to the tvb and free them when the tvb is freed? This seems
to be more obvious than your idea (at least to me) ... However, it might
be easier to implement that all memory blocks are related to the packet
dissection and freed afterwards as you've described your solution, so
there's no real difference in the two possibilities, so simply chose the
easier to implement one :-)
And what about changing the names of the functions to something that
makes it more obvious that the string will be gone after the packet was
processed?
I don't have a good idea about this naming, maybe something like:
transient, temp or alike, so this functions named like:
tvb_get_transient_string(), however, there must be a better name than my
suggestions ...
Regards, ULFL
