Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:36433
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain chiaraquartet.net from 38.99.98.18 cause and error)
Message-ID: <47E7C3D3.3050404@chiaraquartet.net>
Date: Mon, 24 Mar 2008 10:08:03 -0500
User-Agent: Thunderbird 2.0.0.12 (X11/20080227)
MIME-Version: 1.0
To: Dmitry Stogov <dmitry@zend.com>
CC: internals Mailing List <internals@lists.php.net>, 
 Stanislav Malyshev <stas@zend.com>
References: <47E2F8FA.20107@chiaraquartet.net> <47E37C42.10308@zend.com> <47E3E7F1.3060200@chiaraquartet.net> <47E792E1.9070803@zend.com> <47E7AC60.1040705@chiaraquartet.net> <47E7B89C.7060708@zend.com> <47E7BD9C.3040506@chiaraquartet.net> <47E7C36D.106@zend.com>
In-Reply-To: <47E7C36D.106@zend.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] REMINDER - stream wrappers in include_path
From: greg@chiaraquartet.net (Gregory Beaver)

Dmitry Stogov wrote:
>
>
> Gregory Beaver wrote:
>> Dmitry Stogov wrote:
>>> REALPATH_FAILED looks like a hack :(
>>> PHP itself doesn't need it at all, according to your patch
>>> php_stream_open() may just return NULL immediately instead of setting
>>> of this flag.
>>> However I am not sure that it will work for all cases. The fact that
>>> we cannot resolve the path doesn't mean that we cannot open the file.
>>> (On some systems getcwd() may fail on some reasons).
>> I remember Solaris having issues with this, now that you mention it.
>>
>> However, the place that I added this already returned NULL if realpath
>> fails, it doesn't change the behavior, just moves the realpath to an
>> earlier position and eliminates an unnecessary double call.  However, if
>> we are comfortable having the double realpath on files not found in
>> include_path (this would only have a performance affect on apps with
>> lots of conditional loading of drivers where the majority are not
>> found), this is fine with me.  I've attached a patch that removes
>> REALPATH_FAILED so there are options.
>>
>> Another option would be to determine which OSes this can fail, and
>> simply not use REALPATH_FAILED on those OSes.
>>> I didn't test the patch with pecl/phar.
>>> Could you explain why php goes into the endless loop?
>> because the cut/paste to plain_wrapper.c:1419 needs a ptr = end, unlike
>> the code in fopen_wrappers.c.
>
> You are right.
>
>> Once you fix this problem, then the real
>> logic problem rears its head, which is that we go into plain_wrapper to
>> open an external stream wrapper.  It is far less efficient (about 4-6%
>> by my measurements) to do this, which is why I moved the include_path
>> parsing into streams.c
>
> Did you measure pecl/phar or regular files?
I only measured regular files, my concern is that we don't affect any
existing users, 100% of whom use regular files at the moment (obviously :).
>
>> This bug only happens when the last item in include_path is a stream
>> wrapper and the file is not found in include_path.
>
> In general I don't have objections except of changing
> zend_resolve_path() (which is a callback for ZE) into
> php_resolve_path(). Anyway I'll look into patch once again with fresh
> head.
>
> I would also ask Stas and other interested people to look into it. 

Thanks Dmitry,

Greg