Re: [PATCH] readline extension bug fixes and enhancements

Hello On Mar 20, 2012, at 9:43 PM, Johannes Schlüter wrote: > > Have you read Nikita's and my comments on your mail from last week? > I just did. There was a problem with my mailing list subscription (wasn't confirmed) so I thought it didn't go through. But the good thing is that I had ported it to 5.4 and found the memory corruption bug. Here are my answers to your comments: On Mar 14, 2012, at 10:28 PM, Johannes Schlüter wrote: > A) for licensing reasons we should try to keep as few readline only things as possible in there > (gpl vs. php license) I understand the issue of licensing but the extension already has readline-specific functionality so I thought why not. However if this is going to be a problem then I can take out all readline-sepcific functions into a separate module altogether and keep this one for editline functions only. > B) thread safty isn't an issue for readline but you still should do the init and deinit in > rinit/rshutdown not minit/mshutdown, probably also do only bind_key_functions =NULL in rinit and > init the hashtable on demand later. Roger that. > C) don't compare the r result of zend_hash_find and others against -1 or such but use > SUCCESS/FAILURE Roger that. > D) i don't really like new features in 5.3 anymore at this stage This last patch is ported to 5.4. > E) please log a bug and attach the patch so we can track this Roger that. After I clear out which direction to take based on your answers. And as for Nikita's comments: On Mar 14, 2012, at 10:47 PM, Nikita Popov wrote: > May I ask where you got this pattern for copying zvals from? Is this > kind of code shown in some tutorial / manual / etc? Such code is used > all other the place, instead of the MAKE_COPY_ZVAL macro and I wonder > why. Doing manual copying has a good chance of leaking memory. Just > from glancing I'd say that your code will leak if arg has a refcount > > 1 (but I could be wrong) as it does not PINIT the zval after it was > copied. Two points here: A) The new patch does not use this at all because it really isn't needed B) I got this from the same readline.c file lines 521 and 574 and 579. But I will change it accordingly