Re: [PHP-DEV] SAPI refactoring
Hi Joe, On Mon, Nov 17, 2014 at 8:42 AM, Joe Watkins pthre...@pthreads.org wrote: On Sat, 2014-11-15 at 11:16 +0100, Florian Margaine wrote: Hi list, As of today, most of the SAPIs have a lot of code duplication with regards to HTTP headers parsing/handling. I'm about to completely duplicate a function from one SAPI to the other to add a feature to fpm (`getallheaders()`, see https://github.com/php/php-src/pull/523), and I'd like to avoid that. Should I create a `sapi/main/` folder for common functions across the SAPIs? Or should each SAPI remain completely independent from each other, even if it means duplicating code? Regards, Morning Florian, There is already a place to put shared SAPI code. main/SAPI.h main/SAPI.c There is also a SAPI_API decl that you will need to use. Thanks! Cheers Joe Cheers, -- Florian Margaine
Re: [PHP-DEV] Re: PHP-FPM state
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Le 15/11/2014 08:27, Remi Collet a écrit : I have also fixed: https://bugs.php.net/68428 listen.allowed_clients is IPv4 only SO PLEASE, everyone who can test these patches, please test and give feedback [1] Really lot of changes... :( Remi. -BEGIN PGP SIGNATURE- Version: GnuPG v2 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlRpvBsACgkQYUppBSnxahjUpQCbB+PMHKZcTdRgUUpsr9Tbwqj/ u0YAoIdXdVY9SVo6rTilmXXSTlLzdez2 =1SUp -END PGP SIGNATURE- -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
[PHP-DEV] Use zend_string* for op_array-arg_info[].name and class_name
Hi, Please review the patch https://gist.github.com/dstogov/47a39aff37f0a6441ea0 Thanks. Dmitry.
Re: [PHP-DEV] Re: PHP-FPM state
On 15/11/2014 11:11, Ferenc Kovacs wrote: 2014.11.15. 8:28 ezt írta (Remi Collet r...@fedoraproject.org): FPM state in 5.5.19 / 5.6.3 is quite bad :( Maybe I'm saying something unpopular, but wouldn't it be better to rollback the fpm changes, release 5.5.20 and 5.6.4 and keep fpm+ipv6 for master? Cheers -- Matteo Beccati Development Consulting - http://www.beccati.com/ -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Re: PHP-FPM state
On Nov 17, 2014 4:26 PM, Matteo Beccati p...@beccati.com wrote: On 15/11/2014 11:11, Ferenc Kovacs wrote: 2014.11.15. 8:28 ezt írta (Remi Collet r...@fedoraproject.org): FPM state in 5.5.19 / 5.6.3 is quite bad :( Maybe I'm saying something unpopular, but wouldn't it be better to rollback the fpm changes, release 5.5.20 and 5.6.4 and keep fpm+ipv6 for master? It could be a solution, while it brings its lot of issues too. As Remi said, it would be way better if we stop change so many things in patch releases. Cheers, Pierre
Re: [PHP-DEV] Re: PHP-FPM state
On Mon, Nov 17, 2014 at 12:38 PM, Pierre Joye pierre@gmail.com wrote: On Nov 17, 2014 4:26 PM, Matteo Beccati p...@beccati.com wrote: On 15/11/2014 11:11, Ferenc Kovacs wrote: 2014.11.15. 8:28 ezt írta (Remi Collet r...@fedoraproject.org): FPM state in 5.5.19 / 5.6.3 is quite bad :( Maybe I'm saying something unpopular, but wouldn't it be better to rollback the fpm changes, release 5.5.20 and 5.6.4 and keep fpm+ipv6 for master? It could be a solution, while it brings its lot of issues too. As Remi said, it would be way better if we stop change so many things in patch releases. Cheers, Pierre it wasn't really much change but a small change without proper review or test coverage. By Really lot of changes... :( Remi was referring that he had to do a bunch of changes to fix the multiple bugs reported by this small change in php-fpm and it would be really nice if others could review those. and as I mentioned, extending the fpm rest suite would be also important to avoid similar mistakes in the future. -- Ferenc Kovács @Tyr43l - http://tyrael.hu
Re: [PHP-DEV] Re: PHP-FPM state
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
Le 15/11/2014 11:11, Ferenc Kovacs a écrit :
Yeah, the test coverage of fpm is lacking, and while this PR
contained a test for testing the happy path (that you can listen on
ipv6) but there were no tests which could have helped us to spot
the problems introduced with the change.
The main issue with the FPM test suite, is that we need to be able to
run small FactCGI request.
Proposal : include the small FastCGI client library from
https://github.com/adoy/PHP-FastCGI-Client and use it.
See attached patch which add 3 tests (covering some of the recent bugs).
Do you think it is Ok ?
Remi.
-BEGIN PGP SIGNATURE-
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iEYEARECAAYFAlRqEyIACgkQYUppBSnxahjAQwCcDpTViQ59y6/nQuD+w4Rv9Wl2
xf0AoOogKnZwZmrVQPJ62LAEpSvOhaM2
=zHh0
-END PGP SIGNATURE-
From 7b2c5a9463be7cdaeeb3c2962a769c3ea78bacec Mon Sep 17 00:00:00 2001
From: Remi Collet r...@php.net
Date: Mon, 17 Nov 2014 16:19:56 +0100
Subject: [PATCH] Add fcgi client library from
https://github.com/adoy/PHP-FastCGI-Client and more FPM tests
---
sapi/fpm/tests/005.phpt| 58 +
sapi/fpm/tests/006.phpt| 58 +
sapi/fpm/tests/007.phpt| 69 ++
sapi/fpm/tests/fcgi.inc| 577 +
sapi/fpm/tests/include.inc | 24 +-
5 files changed, 785 insertions(+), 1 deletion(-)
create mode 100644 sapi/fpm/tests/005.phpt
create mode 100644 sapi/fpm/tests/006.phpt
create mode 100644 sapi/fpm/tests/007.phpt
create mode 100644 sapi/fpm/tests/fcgi.inc
diff --git a/sapi/fpm/tests/005.phpt b/sapi/fpm/tests/005.phpt
new file mode 100644
index 000..992b01f
--- /dev/null
+++ b/sapi/fpm/tests/005.phpt
@@ -0,0 +1,58 @@
+--TEST--
+FPM: Test IPv4 allowed clients
+--SKIPIF--
+?php include skipif.inc; ?
+--FILE--
+?php
+
+include include.inc;
+
+$logfile = dirname(__FILE__).'/php-fpm.log.tmp';
+$port = 9000+PHP_INT_SIZE;
+
+$cfg = EOT
+[global]
+error_log = $logfile
+[unconfined]
+listen = [::]:$port
+listen.allowed_clients = 127.0.0.1
+pm = dynamic
+pm.max_children = 5
+pm.start_servers = 2
+pm.min_spare_servers = 1
+pm.max_spare_servers = 3
+EOT;
+
+$fpm = run_fpm($cfg, $tail);
+if (is_resource($fpm)) {
+echo fgets($tail);
+echo fgets($tail);
+try {
+ run_request('127.0.0.1', $port);
+ echo IPv4 ok\n;
+ } catch (Exception $e) {
+ echo IPv4 error\n;
+ }
+try {
+ run_request('[::1]', $port);
+ echo IPv6 ok\n;
+ } catch (Exception $e) {
+ echo IPv6 error\n;
+ }
+proc_terminate($fpm);
+stream_get_contents($tail);
+fclose($tail);
+proc_close($fpm);
+}
+
+?
+--EXPECTF--
+[%d-%s-%d %d:%d:%d] NOTICE: fpm is running, pid %d
+[%d-%s-%d %d:%d:%d] NOTICE: ready to handle connections
+IPv4 ok
+IPv6 error
+--CLEAN--
+?php
+$logfile = dirname(__FILE__).'/php-fpm.log.tmp';
+@unlink($logfile);
+?
diff --git a/sapi/fpm/tests/006.phpt b/sapi/fpm/tests/006.phpt
new file mode 100644
index 000..0383f03
--- /dev/null
+++ b/sapi/fpm/tests/006.phpt
@@ -0,0 +1,58 @@
+--TEST--
+FPM: Test IPv6 allowed clients
+--SKIPIF--
+?php include skipif.inc; ?
+--FILE--
+?php
+
+include include.inc;
+
+$logfile = dirname(__FILE__).'/php-fpm.log.tmp';
+$port = 9000+PHP_INT_SIZE;
+
+$cfg = EOT
+[global]
+error_log = $logfile
+[unconfined]
+listen = [::]:$port
+listen.allowed_clients = ::1
+pm = dynamic
+pm.max_children = 5
+pm.start_servers = 2
+pm.min_spare_servers = 1
+pm.max_spare_servers = 3
+EOT;
+
+$fpm = run_fpm($cfg, $tail);
+if (is_resource($fpm)) {
+echo fgets($tail);
+echo fgets($tail);
+try {
+ run_request('127.0.0.1', $port);
+ echo IPv4 ok\n;
+ } catch (Exception $e) {
+ echo IPv4 error\n;
+ }
+try {
+ run_request('[::1]', $port);
+ echo IPv6 ok\n;
+ } catch (Exception $e) {
+ echo IPv6 error\n;
+ }
+proc_terminate($fpm);
+stream_get_contents($tail);
+fclose($tail);
+proc_close($fpm);
+}
+
+?
+--EXPECTF--
+[%d-%s-%d %d:%d:%d] NOTICE: fpm is running, pid %d
+[%d-%s-%d %d:%d:%d] NOTICE: ready to handle connections
+IPv4 error
+IPv6 ok
+--CLEAN--
+?php
+$logfile = dirname(__FILE__).'/php-fpm.log.tmp';
+@unlink($logfile);
+?
diff --git a/sapi/fpm/tests/007.phpt b/sapi/fpm/tests/007.phpt
new file mode 100644
index 000..616da1c
--- /dev/null
+++ b/sapi/fpm/tests/007.phpt
@@ -0,0 +1,69 @@
+--TEST--
+FPM: Test access_log
+--SKIPIF--
+?php include skipif.inc; ?
+--FILE--
+?php
+
+include include.inc;
+
+$logfile = dirname(__FILE__).'/php-fpm.log.tmp';
+$accfile = dirname(__FILE__).'/php-fpm.acc.tmp';
+$port = 9000+PHP_INT_SIZE;
+
+$cfg = EOT
+[global]
+error_log = $logfile
+[unconfined]
+listen = [::]:$port
+access.log = $accfile
+ping.path = /ping
+ping.response = pong
+pm = dynamic
+pm.max_children = 5
Re: [PHP-DEV] Re: PHP-FPM state
On Mon, Nov 17, 2014 at 4:24 PM, Remi Collet r...@fedoraproject.org wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Le 15/11/2014 11:11, Ferenc Kovacs a écrit : Yeah, the test coverage of fpm is lacking, and while this PR contained a test for testing the happy path (that you can listen on ipv6) but there were no tests which could have helped us to spot the problems introduced with the change. The main issue with the FPM test suite, is that we need to be able to run small FactCGI request. Proposal : include the small FastCGI client library from https://github.com/adoy/PHP-FastCGI-Client and use it. See attached patch which add 3 tests (covering some of the recent bugs). Do you think it is Ok ? Remi. -BEGIN PGP SIGNATURE- Version: GnuPG v2 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlRqEyIACgkQYUppBSnxahjAQwCcDpTViQ59y6/nQuD+w4Rv9Wl2 xf0AoOogKnZwZmrVQPJ62LAEpSvOhaM2 =zHh0 -END PGP SIGNATURE- -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php As it is pretty small/lightweight(single class) and has MIT license, I think it should be fine including it. Thanks for looking into these! -- Ferenc Kovács @Tyr43l - http://tyrael.hu
[PHP-DEV] Re: Use zend_string* for op_array-arg_info[].name and class_name
Hey: On Mon, Nov 17, 2014 at 5:25 PM, Dmitry Stogov dmi...@zend.com wrote: Hi, Please review the patch https://gist.github.com/dstogov/47a39aff37f0a6441ea0 I am not sure, why can't we build a zend_string arg-name from char * while doing register internal functions? then we can have the same zend_arg_info in runtime for both internal/user functions? thanks Thanks. Dmitry. -- Xinchen Hui @Laruence http://www.laruence.com/ -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
[PHP-DEV] Re: Use zend_string* for op_array-arg_info[].name and class_name
On Tue, Nov 18, 2014 at 6:01 AM, Xinchen Hui xinche...@zend.com wrote: Hey: On Mon, Nov 17, 2014 at 5:25 PM, Dmitry Stogov dmi...@zend.com wrote: Hi, Please review the patch https://gist.github.com/dstogov/47a39aff37f0a6441ea0 I am not sure, why can't we build a zend_string arg-name from char * while doing register internal functions? We can, but it'll take significant amount of additional memory and then we will have to free it on shutdown. Thanks. Dmitry. then we can have the same zend_arg_info in runtime for both internal/user functions? thanks Thanks. Dmitry. -- Xinchen Hui @Laruence http://www.laruence.com/
[PHP-DEV] Re: Use zend_string* for op_array-arg_info[].name and class_name
Sent from my iPhone On Nov 18, 2014, at 12:34 PM, Dmitry Stogov dmi...@zend.com wrote: On Tue, Nov 18, 2014 at 6:01 AM, Xinchen Hui xinche...@zend.com wrote: Hey: On Mon, Nov 17, 2014 at 5:25 PM, Dmitry Stogov dmi...@zend.com wrote: Hi, Please review the patch https://gist.github.com/dstogov/47a39aff37f0a6441ea0 I am not sure, why can't we build a zend_string arg-name from char * while doing register internal functions? We can, but it'll take significant amount of additional memory and then we will have to free it on shutdown. we only need do it in master process, and mark as interned(with hash precalculated), no write will be happened, so thanks to COW on fork, we won't need lots of extra memory. Thanks Thanks. Dmitry. then we can have the same zend_arg_info in runtime for both internal/user functions? thanks Thanks. Dmitry. -- Xinchen Hui @Laruence http://www.laruence.com/
[PHP-DEV] Re: Use zend_string* for op_array-arg_info[].name and class_name
On Tue, Nov 18, 2014 at 7:49 AM, Xinchen Hui larue...@gmail.com wrote: Sent from my iPhone On Nov 18, 2014, at 12:34 PM, Dmitry Stogov dmi...@zend.com wrote: On Tue, Nov 18, 2014 at 6:01 AM, Xinchen Hui xinche...@zend.com wrote: Hey: On Mon, Nov 17, 2014 at 5:25 PM, Dmitry Stogov dmi...@zend.com wrote: Hi, Please review the patch https://gist.github.com/dstogov/47a39aff37f0a6441ea0 I am not sure, why can't we build a zend_string arg-name from char * while doing register internal functions? We can, but it'll take significant amount of additional memory and then we will have to free it on shutdown. we only need do it in master process, and mark as interned(with hash precalculated), no write will be happened, so thanks to COW on fork, we won't need lots of extra memory. It's not true for Windows. On Linux we would copy arg_info from shared read-only segment into process heap memory, then some blocks may be COW or not depended on luck. Thanks. Dmitry. Thanks Thanks. Dmitry. then we can have the same zend_arg_info in runtime for both internal/user functions? thanks Thanks. Dmitry. -- Xinchen Hui @Laruence http://www.laruence.com/
Re: [PHP-DEV] Re: Use zend_string* for op_array-arg_info[].name and class_name
Hi! we only need do it in master process, and mark as interned(with hash precalculated), no write will be happened, so thanks to COW on fork, we won't need lots of extra memory. It's not true for Windows. On Linux we would copy arg_info from shared read-only segment into process heap memory, then some blocks may be COW or not depended on luck. How bad would it be? Given that significantly simplifies the code, and does not require to check and write two branches each time we deal with arginfo, maybe it is worth to spend some memory (given that we only spend it once on init)? -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] GitHub Pull Requests Triage Team
Hi! I like the label idea, it will help having a nice overall look on the PRs. When I myself find some time, I mainly browse through bugs.php.net and fix things and close issues. As an experiment, I went and added a bunch of labels to github: RFC - for RFC PRs bugfix - for fixes associated with bugs enhancement - for PRs that improve something that is not a bug, but not big enough to be RFC quickfix - small fixes like typos, etc. which can be reasonably merged by anyone after quick review tests - test-only PRs, these can also be usually merged as soon as they pass, since the more tests the better :) Please tell if you think we need more labels. Don't need too many of them but I think it'd help if someone having half an hour to contribute would be able to go to PRs, choose quickfix or typo labels and merge away without trying to read through gigantic RFC patches. -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
