Wireshark · Wireshark-dev: Re: [Wireshark-dev] [PATCH] Re: Freeing memory of se_alloc'ated object

[Max Dmitrichenko <dmitrmax@xxxxxxxxx>]

2011/5/3 ronnie sahlberg <ronniesahlberg@xxxxxxxxx>:
> I think registering a destructor for an allocated is very useful, but
> it would be very uncommon.
> Most allocations never need a destructur, so it shouldnt be made
> mandatory in the allocation functions.

As it is implemented now, it's not mandatory - just a branch of already
existing functions.

> So instead of creating a new API for allocations with a destructor I
> would suggest adding a new _add_destructor call instead.
> That way you can add destructors to existing objects after they are
> allocated, post allocation time.

Ok. I see you, guys, like the idea of dynamic feature management :)
Both ways have right to exist, but this one is more error prone.
Copy-pasting the code can add two destructors call to the same object.

> something like :
>
>    object = se_alloc(...);
>    se_set_emem_destructor(object,  void (*your_callback)(void
> *object), void *private_data);
>
>
> Then this can just add the destructor to a linked list of
> struct se_destructors {
>    struct se_destructors *next;
>    void *object;
>    void *private_data;
>    void (*callback)(void *object, void *private_data);
> };

Well, it was the first idea that came up when I started to implement this.
But when I looked into emem's code, I found the other way to implement
this. Generally I have no objections but one - read below.

> I threw a private_data pointer in there too.
> Once you have callbacks you almost always enbd up wanting to pass data
> like this to it too.
While it is very common to use such private_data pointers, when talking
about callbacks, it is generally a bad design practice if the object being
destructed needs some external context.

While you've made a statement that destructor's will be rarely used among
all the Wireshark code, the feature of private_data will be rarely used among
destructors' code ;)

Anyway, the feature introduced in my patch can be reimplemented. But
may I ask the dumb question? Who decides here what is going to the trunk
and what it should like?

I'm ready to (re-)implement any design of this feature but need some
concentrated piece of absolute wisdom from local Guru :)

--
  Max