Hello,
On 10/4/13 2:43 PM, Henning Westerholt wrote:
Am Freitag, 4. Oktober 2013, 12:16:32 schrieb Charles Chance:
Sorry hit reply only, first time.
Would you like me still to update to use mcd_free or leave it with you?
Hi Charles,
just commit the patch, looks good for me. The only small thing I noticed is that you changed the indention in the variable definition in pv_set_mcd_expire from tabs to space, would be great if you could adapt this.
With regards to the other wrapper (mcd_free) that Daniel mentioned, this is only used as callback function for the library. I'll clarify this in the comments that this is the purpose of this code.
my comment was related to the fact that I haven't seen an explicit pkg_malloc() for the returned value from the libmacached function. I assumed it is going to use the callback mcd_malloc(), based on libmemcached api docs, which say that the result has to be released by caller.
From that perspective, as the result is allocated with mcd_malloc(), it looks more consistent if is going to be freed with mcd_free(). A change in mcd_malloc() would naturally trigger an appropriate change in mcd_free() (e.g., using shm_malloc() instead of pkg_malloc()), while now if someone changes mcd_malloc() and mcd_free() would have to know that has to change the pkg_free() in the other part of the code.
So, in short, using pkg_free() now solves the issue, it is ok, just that doesn't look coherent.
Cheers, Daniel