Re: [PATCH] readline extension bug fixes and enhancements

From: Date: Wed, 21 Mar 2012 08:03:46 +0000
Subject: Re: [PATCH] readline extension bug fixes and enhancements
References: 1 2  Groups: php.internals 
Request: Send a blank email to [email protected] to get a copy of this message
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



Thread (4 messages)

« previous php.internals (#59111) next »