Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:59111
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain eformations.net from 74.125.83.42 cause and error)
Mime-Version: 1.0 (Apple Message framework v1257)
Content-Type: text/plain; charset=iso-8859-1
In-Reply-To: <1332272639.2383.692.camel@guybrush>
Date: Wed, 21 Mar 2012 10:03:46 +0200
Cc: internals@lists.php.net
Content-Transfer-Encoding: quoted-printable
Message-ID: <1BE14D30-B091-49B3-ACD3-4296C3C6E35B@eformations.net>
References: <11302143-55E0-48E9-AC30-943E6075E794@eformations.net> <1332272639.2383.692.camel@guybrush>
To: =?iso-8859-1?Q?Johannes_Schl=FCter?= <johannes@schlueters.de>
Subject: Re: [PHP-DEV] [PATCH] readline extension bug fixes and enhancements
From: osama.sorour@eformations.net (Osama Abu Elsorour)

Hello

On Mar 20, 2012, at 9:43 PM, Johannes Schl=FCter wrote:

>=20
> Have you read Nikita's and my comments on your mail from last week?
>=20

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=FCter 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 =3DNULL 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