Hello Henning, Sorry hit reply only, first time.
Would you like me still to update to use mcd_free or leave it with you? Best, Charles On 4 October 2013 11:40, Henning Westerholt <[email protected]>wrote: > Am Freitag, 4. Oktober 2013, 09:40:17 schrieb Charles Chance: > > Yes, agreed - I'll update it today. Henning Westerholt was original > author > > if he'd like to review before I commit? > > Hello Charles, > > I'll take a look today to it as well, thank you. > > Regards, > > Henning > > > On 4 Oct 2013 09:12, "Daniel-Constantin Mierla" <[email protected]> > wrote: > > > Hello, > > > > > > for consistency, it might be good that mcd_free() is used instead of > > > pkg_free when mcd_memory is set, because it is supposed to be the pair > of > > > mcd_malloc() which is used to allocate the memory. mcd_free() is a > wrapper > > > around pkg_free(), so works like current patch, but it is safer for > future > > > changes to mcd_*() functions. > > > > > > These are remarks just looking at the current code, I am not familiar > with > > > memcached lib and api. > > > > > > Cheers, > > > Daniel > > > > > > On 10/4/13 9:34 AM, Federico Cabiddu wrote: > > > > > > Hi Charles, > > > I just tried the patch running the test I used to reproduce the leak. > > > It's ok, the memory debug shows that the memory is correctly released. > > > I tested with memory parameter set to 0 and to 1. > > > Thank you. > > > > > > Best regards, > > > > > > Federico > > > > > > On Fri, Oct 4, 2013 at 1:00 AM, Charles Chance < > > > > > > [email protected]> wrote: > > >> Hi Federico/Dragos, > > >> > > >> Thank you both for your input. I have placed the call to (pkg_)free > > >> > > >> into a separate function and called it where necessary from each of > the > > >> other functions. > > >> > > >> I haven't had chance to test yet - if you have time, please apply the > > >> > > >> attached diff and let me know if the leak is fixed and I will commit > to > > >> master. Otherwise, I will test myself over the weekend. > > >> > > >> Thanks again, > > >> > > >> Charles > > >> > > >> On 3 October 2013 21:30, Dragos Oancea <[email protected]> wrote: > > >>> Hi Charles, > > >>> > > >>> In the function where Daniel just made the fix for the memory leak > (int > > >>> pv_get_mcd_value()>>> > > >>> ) , just before existing it with 0 , we added something like the > > >>> > > >>> following : > > >>> if (mcd_memory) { > > >>> > > >>> pkg_free(return_value); > > >>> > > >>> } else { > > >>> > > >>> free(return_value); > > >>> > > >>> } > > >>> > > >>> It looks like it does not leak anymore, but please-double check if > we > > >>> > > >>> are free-ing it in the right place. > > >>> > > >>> Regards, > > >>> > > >>> Dragos > > >>> > > >>> ------------------------------ > > >>> > > >>> *From:* Charles Chance <[email protected]> > > >>> > > >>> *To:* Daniel-Constantin Mierla <[email protected]> > > >>> *Cc:* sr-dev <[email protected]> > > >>> *Sent:* Thursday, October 3, 2013 7:27 PM > > >>> *Subject:* Re: [sr-dev] memory leak in memcached module > > >>> > > >>> I can take a look this evening. Assuming nobody has already > started? > > >>> > > >>> Best, > > >>> Charles > > >>> > > >>> On 2 Oct 2013 20:23, "Daniel-Constantin Mierla" <[email protected]> > > >>> > > >>> wrote: > > >>> > > >>> Hello, > > >>> > > >>> there is (still) a memory leak in memcached module, discovered on a > > >>> report by Dragos Oancea. > > >>> > > >>> The pkg usage logs are like: > > >>> > > >>> 0(24328) NOTICE: qm_status: 19010. N address=0x7fb23683bc98 > > >>> frag=0x7fb23683bc68 size=8 used=1 > > >>> > > >>> 0(24328) NOTICE: qm_status: alloc'd from memcached: > > >>> ../../parser/../ut.h: pkg_str_dup(733) > > >>> > > >>> 0(24328) NOTICE: qm_status: start check=f0f0f0f0, end > check= > > >>> > > >>> c0c0c0c0, abcdefed > > >>> > > >>> 0(24328) NOTICE: qm_status: 19011. N address=0x7fb23683bd00 > > >>> > > >>> frag=0x7fb23683bcd0 size=48 used=1 > > >>> > > >>> 0(24328) NOTICE: qm_status: alloc'd from memcached: > > >>> memcached.c: mcd_malloc(127) > > >>> > > >>> 0(24328) NOTICE: qm_status: start check=f0f0f0f0, end > check= > > >>> > > >>> c0c0c0c0, abcdefed > > >>> > > >>> 0(24328) NOTICE: qm_status: 19012. N address=0x7fb23683bd90 > > >>> > > >>> frag=0x7fb23683bd60 size=8 used=1 > > >>> > > >>> 0(24328) NOTICE: qm_status: alloc'd from memcached: > > >>> ../../parser/../ut.h: pkg_str_dup(733) > > >>> > > >>> 0(24328) NOTICE: qm_status: start check=f0f0f0f0, end > check= > > >>> > > >>> c0c0c0c0, abcdefed > > >>> > > >>> 0(24328) NOTICE: qm_status: 19013. N address=0x7fb23683bdf8 > > >>> > > >>> frag=0x7fb23683bdc8 size=48 used=1 > > >>> > > >>> 0(24328) NOTICE: qm_status: alloc'd from memcached: > > >>> memcached.c: mcd_malloc(127) > > >>> > > >>> 0(24328) NOTICE: qm_status: start check=f0f0f0f0, end > check= > > >>> > > >>> c0c0c0c0, abcdefed > > >>> > > >>> 0(24328) NOTICE: qm_status: 19014. N address=0x7fb23683be88 > > >>> > > >>> frag=0x7fb23683be58 size=8 used=1 > > >>> > > >>> 0(24328) NOTICE: qm_status: alloc'd from memcached: > > >>> memcached.c: mcd_malloc(127) > > >>> > > >>> 0(24328) NOTICE: qm_status: start check=f0f0f0f0, end > check= > > >>> > > >>> c0c0c0c0, abcdefed > > >>> > > >>> 0(24328) NOTICE: qm_status: 19015. N address=0x7fb23683bef0 > > >>> > > >>> frag=0x7fb23683bec0 size=16 used=1 > > >>> > > >>> 0(24328) NOTICE: qm_status: alloc'd from memcached: > > >>> ../../parser/../ut.h: pkg_str_dup(733) > > >>> > > >>> 0(24328) NOTICE: qm_status: start check=f0f0f0f0, end > check= > > >>> > > >>> c0c0c0c0, abcdefed > > >>> > > >>> 0(24328) NOTICE: qm_status: 19016. N address=0x7fb23683bf60 > > >>> > > >>> frag=0x7fb23683bf30 size=8 used=1 > > >>> > > >>> 0(24328) NOTICE: qm_status: alloc'd from memcached: > > >>> memcached.c: mcd_malloc(127) > > >>> > > >>> 0(24328) NOTICE: qm_status: start check=f0f0f0f0, end > check= > > >>> > > >>> c0c0c0c0, abcdefed > > >>> > > >>> 0(24328) NOTICE: qm_status: 19017. N address=0x7fb23683bfc8 > > >>> > > >>> frag=0x7fb23683bf98 size=24 used=1 > > >>> > > >>> 0(24328) NOTICE: qm_status: alloc'd from memcached: > > >>> ../../parser/../ut.h: pkg_str_dup(733) > > >>> > > >>> 0(24328) NOTICE: qm_status: start check=f0f0f0f0, end > check= > > >>> > > >>> c0c0c0c0, abcdefed > > >>> > > >>> The one related to pkg_str_dup() should be fixed by the commit: > > >>> > > >>> - > > >>> > http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=6faf > > >>> 12653c1db9f011b1826061824c831bda3f58 > > >>> > > >>> The other one is related to mcd_malloc(), which I guess it is due to > the > > >>> function that returns the value from memchache, memcached_get() used > in > > >>> > > >>> pv_get_mcd_value_helper() -- the returned object has to be freed by > > >>> > > >>> calling code, according to: > > >>> > > >>> - http://docs.libmemcached.org/memcached_get.html > > >>> > > >>> Since the libmemcached was initialized with wrappers around pkg > > >>> malloc/free, I expect the respective free function has to be used to > > >>> free > > >>> the result. > > >>> > > >>> Can any of memcached devs check my commit and investigate further the > > >>> second leak? > > >>> > > >>> Cheers, > > >>> Daniel > > >>> > > >>> -- > > >>> Daniel-Constantin Mierla - http://www.asipto.com > > >>> http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda > > >>> Kamailio Advanced Trainings - Berlin, Nov 25-28; Miami, Nov 18-20, > 2013 > > >>> > > >>> - more details about Kamailio trainings at http://www.asipto.com - > > >>> > > >>> _______________________________________________ > > >>> sr-dev mailing list > > >>> [email protected] > > >>> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev > > >>> > > >>> www.sipcentric.com > > >>> > > >>> Follow us on twitter @sipcentric <http://twitter.com/sipcentric> > > >>> > > >>> Sipcentric Ltd. Company registered in England & Wales no. 7365592. > > >>> Registered office: Unit 10 iBIC, Birmingham Science Park, Holt Court > > >>> South, Birmingham B7 4EJ. > > >>> > > >>> _______________________________________________ > > >>> sr-dev mailing list > > >>> [email protected] > > >>> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev > > >>> > > >>> > > >>> > > >>> _______________________________________________ > > >>> sr-dev mailing list > > >>> [email protected] > > >>> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev > > >> > > >> www.sipcentric.com > > >> > > >> Follow us on twitter @sipcentric <http://twitter.com/sipcentric> > > >> > > >> Sipcentric Ltd. Company registered in England & Wales no. 7365592. > > >> Registered office: Unit 10 iBIC, Birmingham Science Park, Holt Court > > >> South, Birmingham B7 4EJ. > > >> > > >> _______________________________________________ > > >> sr-dev mailing list > > >> [email protected] > > >> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev > > > > > > _______________________________________________ > > > sr-dev mailing > > > [email protected]:// > lists.sip-router.org/cgi-bin/mailma > > > n/listinfo/sr-dev > > > > > > > > > -- > > > Daniel-Constantin Mierla - > > > http://www.asipto.comhttp://twitter.com/#!/miconda - > > > http://www.linkedin.com/in/miconda Kamailio Advanced Trainings - > Berlin, > > > Nov 25-28; Miami, Nov 18-20, 2013> > > > - more details about Kamailio trainings at http://www.asipto.com - > > > > > > _______________________________________________ > > > sr-dev mailing list > > > [email protected] > > > http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev > > -- *Charles Chance* Managing Director t. 0121 285 4400 m. 07932 063 891 -- www.sipcentric.com Follow us on twitter @sipcentric <http://twitter.com/sipcentric> Sipcentric Ltd. Company registered in England & Wales no. 7365592. Registered office: Unit 10 iBIC, Birmingham Science Park, Holt Court South, Birmingham B7 4EJ.
_______________________________________________ sr-dev mailing list [email protected] http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
