Re: REMINDER - stream wrappers in include_path
| From: | Dmitry Stogov | Date: | Mon, 24 Mar 2008 14:20:12 +0000 |
| Subject: | Re: REMINDER - stream wrappers in include_path | ||
| References: | 1 2 3 4 5 | Groups: | php.internals |
| Request: | Send a blank email to internals+get-36429@lists.php.net to get a copy of this message | ||
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 didn't test the patch with pecl/phar.
Could you explain why php goes into the endless loop?
Thanks. Dmitry.
Gregory Beaver wrote:
Dmitry Stogov wrote:Hi Greg, In your second patch you forgot "break", so it couldn't work. I don't see any reason in second patch at all, because you call php_resolve_patch() from _php_stream_open_wrapper_ex() anyway. The bad thing with the first part that it calls realpath() function twice for each include("relative_path.php") and it makes several file-system accesses (look intoYour patch is broken, it causes an endless loop in tests/include_path.phpt when running phar tests. Incidentally, I was unaware of the strace command (I've seen it bandied about on irc, but thought it was something else), so thank you for edumacatin me there. The attached patch has no additional syscalls over the current case for both existing and non-existing files, and actually works for phar as well :). It's also simpler (I removed the include stuff from zend_vm_def.h), although you may want to add it back in and bench it to see if it makes any difference in performance, since I found such a dramatic reduction in operations here with it. Gregstrace ... 2>&1| grep lstat). With your patch I see 6 syscalls more. So I would prefer my patch that is more efficient for regular files. The same patch with fixed white-spaces is attached.