Mailing List Archive

On Wed, Sep 04, 2013 at 08:16:44PM +0900, Keisuke MORI wrote:
> Hi,
>
> The attached patch will fix a memory leak in ccm that occurs whenever a ccm
> client disconnect.

Thank you.

This may introduce double free for client_delete_all() now?

All this aparently useless indirection seems to be from a time
when client_destroy explicitly called into a ->ops->destroy "virtual
function". Which it no longer does.

So I think dropping the explicit calls to client_destroy, as well as
the other then useless indirection functions, but instead do a
g_hash_table_new_full with g_free in client_init would be the way to go.

Could you have a look?

> It would not affect to most of the installations because only crmd and cib
> are the client, but if you run any ccm client such as crm_node command
> periodically, ccm will increase its memory consumption.
>
> The valgrind outputs are also attached as the evidence of the leakage and
> the fix by the patch;
> The results are taken after crm_node command is executed 100 times.
>
> There still exists some definitely / indirectly / possibly lost , but as
> long as I've investigated they are all allocated only at the invocation
> time and not considered as a leak. Double checks are welcome.
>
> Thanks,
> --
> Keisuke MORI

Cheers,
Lars

--
: Lars Ellenberg
: LINBIT | Your Way to High Availability
: DRBD/HA support and consulting http://www.linbit.com

DRBD® and LINBIT® are registered trademarks of LINBIT, Austria.
_______________________________________________________
Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org
http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
Home Page: http://linux-ha.org/

Hi,

2013/9/6 Lars Ellenberg <lars.ellenberg@linbit.com>:
> On Wed, Sep 04, 2013 at 08:16:44PM +0900, Keisuke MORI wrote:
>> Hi,
>>
>> The attached patch will fix a memory leak in ccm that occurs whenever a ccm
>> client disconnect.
>
> Thank you.
>
> This may introduce double free for client_delete_all() now?

No, I do not think it does.
When an individual client exits, client_delete() removes the ipc
object from ccm_hashclient and hence
client_detete_all() will never call client_destroy() for the same ipc
object again.
The valgrind result did not complain regarding to this either.

Am I missing your point?


>
> All this aparently useless indirection seems to be from a time
> when client_destroy explicitly called into a ->ops->destroy "virtual
> function". Which it no longer does.
>
> So I think dropping the explicit calls to client_destroy, as well as
> the other then useless indirection functions, but instead do a
> g_hash_table_new_full with g_free in client_init would be the way to go.

It might be doable, but I do not think it is necessary to rewrite the
code for fixing this issue.

Thanks,


>
> Could you have a look?
>
>> It would not affect to most of the installations because only crmd and cib
>> are the client, but if you run any ccm client such as crm_node command
>> periodically, ccm will increase its memory consumption.
>>
>> The valgrind outputs are also attached as the evidence of the leakage and
>> the fix by the patch;
>> The results are taken after crm_node command is executed 100 times.
>>
>> There still exists some definitely / indirectly / possibly lost , but as
>> long as I've investigated they are all allocated only at the invocation
>> time and not considered as a leak. Double checks are welcome.
>>
>> Thanks,
>> --
>> Keisuke MORI
>
> Cheers,
> Lars
>
> --
> : Lars Ellenberg
> : LINBIT | Your Way to High Availability
> : DRBD/HA support and consulting http://www.linbit.com
>
> DRBD® and LINBIT® are registered trademarks of LINBIT, Austria.
> _______________________________________________________
> Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org
> http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
> Home Page: http://linux-ha.org/



--
Keisuke MORI
_______________________________________________________
Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org
http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
Home Page: http://linux-ha.org/