Sure,
the ep_tvb_get_string() ep_tvb_fake_unicode() are just temporary
names until all uses of tvb_get_string() and friends have been
manually audited and converted to the new semantics.
Once all calls have been audited and converted i will change tha
names back and drop the ep_ prefix (which is only used to be able to
distinguish audited and non-audited calls).
The functions i plan to convert in this first phase would be
tvb_get_string() (almost finished)
tvb_fake_unicode() (almost finished)
tvb_get_text()
tvb_get_stringz()
It will take a week or two more before im finished and will do the big
rename-back
so please provide suggestions to what they should be renamed to.
On 7/29/05, Ulf Lamping <ulf.lamping@xxxxxx> wrote:
> 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
>
