Wireshark · Ethereal-dev: Re: [Ethereal-dev] ememification of tvb_get_tring() and friends

[ronnie sahlberg <ronniesahlberg@xxxxxxxxx>]

>So :
>tvb_get_ephemeral_string() + tvb_get_sessional_string()
>tvb_get_ephemeral_faked_unicode() + ...
>
>or any better suggestions?

Everyone happy with this naming convention   tvb_get_ephemeral_...()
for the allocations with packet dissection allocation lifetime?

If not and someone has a better naming convention, it should really
just be a matter of some sed magic to change it anyway.




On 7/29/05, ronnie sahlberg <ronniesahlberg@xxxxxxxxx> wrote:
> On 7/29/05, LEGO <luis.ontanon@xxxxxxxxx> wrote:
> > Ephemeral, it is a little too long but IMHO it gets right to the point.
> >
> > ephemeral_alloc()
> > ephemeral_memdup()
> > ephemeral_strdup()
> > ephemeral_strndup()
> > ephemeral_strdup_printf()
> > tvb_get_ephemeral_string()
> > tvb_ephemeral_memdup()
> >
> >
> > While for the per session ones I thought in "sessional" that is
> > something in between seasonal and session :-P
> 
> While it is good to have an api that is orthogonal, while i have been looking at
> tvb_get_string() uses   virtually every single use is very temporary in scope.
> I.e. grab the string, process or display it and then immediately free it.
> 
> Would it be useful to add two such functions
> tvb_get_ephemeral_string()  and
> tvb_get_sessional_string()
> considering that there will be virtually no users of the second one?
> Instead of just a single one  tvb_get_string() that is essentially
> tvb_get_ephemeral_string() ?
> 
> Ok,   I have no strong feelings either way.
> 
> So :
> tvb_get_ephemeral_string() + tvb_get_sessional_string()
> tvb_get_ephemeral_faked_unicode() + ...
> 
> or any better suggestions?
> 
> >
> > Luis
> >
> >
> > 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
> > >
> > > _______________________________________________
> > > Ethereal-dev mailing list
> > > Ethereal-dev@xxxxxxxxxxxx
> > > http://www.ethereal.com/mailman/listinfo/ethereal-dev
> > >
> >
> >
> > --
> > This information is top security. When you have read it, destroy yourself.
> > -- Marshall McLuhan
> >
> > _______________________________________________
> > Ethereal-dev mailing list
> > Ethereal-dev@xxxxxxxxxxxx
> > http://www.ethereal.com/mailman/listinfo/ethereal-dev
> >
>