Push Request #21

13 years ago by Jason Gerfen — view source

unread

Please bear with me as I am going to address a few things that (being new)
I seem to have over looked upon my initial pull request @
https://github.com/php/php-src/pull/21.

I have forked and submitted a push request to add native spkac
functionality per a feature request @ https://bugs.php.net/bug.php?id=38917.

It introduces five new functions:
openssl_spki_new()
openssl_spki_export()
openssl_spki_export_challenge()
openssl_spki_verify()
openssl_spki_details()

First I am not sure I should include the openssl_spki_details() function as
it may be overkill and the remainder of the PHP OpenSSL extension does not
have anything providing the modulus and other details. I am leaning towards
the omitting of this in next pull request.

Second. The warning message UC first or all LC has been brought up. The
remainder of the warnings messages do not use LC at all and I simply
followed this example.

Third. The adherence to C89 vs. C99 functions, I could not find references
to this in any of the internals docs. I very well could have missed it.

Fourth. A couple of you made mention of re-factoring to omit goto
statements. Does this mean the remainder of the ext/openssl/openssl.c file
should be re-factored as I originally used this as an example of creating
new functions which does include a few functions making use of goto
statements.

Thanks for any feedback you can provide

--
Jas

13 years ago by jason.gerfen@gmail.com — view source

unread

There really doesn't seem to be much interest in this proposed patch. Should I continue development efforts on closing this feature request?

I do also have a few questions regarding standards adherence, and memory leak methods of detection.

I ask about the memory leak detection as passing test scripts through valgrind only led me to find leaks in the openssl_pkey_new() function.

Thanks for any help you can provide

Jas

Please bear with me as I am going to address a few things that (being new) I seem to have over looked upon my initial pull request @ https://github.com/php/php-src/pull/21.

I have forked and submitted a push request to add native spkac functionality per a feature request @ https://bugs.php.net/bug.php?id=38917.

It introduces five new functions:
openssl_spki_new()
openssl_spki_export()
openssl_spki_export_challenge()
openssl_spki_verify()
openssl_spki_details()

First I am not sure I should include the openssl_spki_details() function as it may be overkill and the remainder of the PHP OpenSSL extension does not have anything providing the modulus and other details. I am leaning towards the omitting of this in next pull request.

Second. The warning message UC first or all LC has been brought up. The remainder of the warnings messages do not use LC at all and I simply followed this example.

Third. The adherence to C89 vs. C99 functions, I could not find references to this in any of the internals docs. I very well could have missed it.

Fourth. A couple of you made mention of re-factoring to omit goto statements. Does this mean the remainder of the ext/openssl/openssl.c file should be re-factored as I originally used this as an example of creating new functions which does include a few functions making use of goto statements.

Thanks for any feedback you can provide

--
Jas

13 years ago by Stas Malyshev — view source

unread

Hi!

There really doesn't seem to be much interest in this proposed patch. Should I continue development efforts on closing this feature request?

Can't say anything here - I don't use these APIs personally, but maybe
other people need them. No idea :)

I do also have a few questions regarding standards adherence, and memory leak methods of detection.

I ask about the memory leak detection as passing test scripts through valgrind only led me to find leaks in the openssl_pkey_new() function.

There's two levels of leak detection - internal memory management &
valgrind. How to work with them:

Internal. Compile php with --enable-debug and run your tests. Leaks
will be reported by php.
External. Set USE_ZEND_ALLOC environment variable to 0. Set
ZEND_DONT_UNLOAD_MODULES to 1. Run it under valgrind, see what it reports.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by Yasuo Ohgaki — view source

unread

2012/4/12 Stas Malyshev smalyshev@sugarcrm.com:

Hi!

There really doesn't seem to be much interest in this proposed patch. Should I continue development efforts on closing this feature request?

Can't say anything here - I don't use these APIs personally, but maybe
other people need them. No idea :)

I do also have a few questions regarding standards adherence, and memory leak methods of detection.

I ask about the memory leak detection as passing test scripts through valgrind only led me to find leaks in the openssl_pkey_new() function.

There's two levels of leak detection - internal memory management &
valgrind. How to work with them:

Internal. Compile php with --enable-debug and run your tests. Leaks
will be reported by php.

External. Set USE_ZEND_ALLOC environment variable to 0. Set
ZEND_DONT_UNLOAD_MODULES to 1. Run it under valgrind, see what it reports.

You might want to do something like

USE_ZEND_ALLOC=0 ZEND_DONT_UNLOAD_MODULES=1
TEST_PHP_EXECUTABLE=./sapi/cli/php php ./run-tests.php -m ext/openssl/

It seems your API is good.
Isn't it just there aren't people who would like to
responsible?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Stas Malyshev — view source

unread

Hi!

You might want to do something like

USE_ZEND_ALLOC=0 ZEND_DONT_UNLOAD_MODULES=1
TEST_PHP_EXECUTABLE=./sapi/cli/php php ./run-tests.php -m ext/openssl/

Also I would advise adding tests that test failure conditions - right
now the test seem to only test "OK" conditions but not failures - like
passing random strings or other bad data.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by jason.gerfen@gmail.com — view source

unread

Hi!

You might want to do something like

USE_ZEND_ALLOC=0 ZEND_DONT_UNLOAD_MODULES=1
TEST_PHP_EXECUTABLE=./sapi/cli/php php ./run-tests.php -m ext/openssl/

I am glad I asked as I was not aware of this. Is there a comprehensive guide to this? I have scoured the php.net site and google but found very little.

Also I would advise adding tests that test failure conditions - right
now the test seem to only test "OK" conditions but not failures - like
passing random strings or other bad data.

No problem I can update this pretty quickly

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by jason.gerfen@gmail.com — view source

unread

Hi

2012/4/12 Stas Malyshev smalyshev@sugarcrm.com:

Hi!

There really doesn't seem to be much interest in this proposed patch. Should I continue development efforts on closing this feature request?

Can't say anything here - I don't use these APIs personally, but maybe
other people need them. No idea :)

On that note Mr. Malyshev has indicated (in addition to several other threads on the internals list) that no new features will be added in 5.3 or 5.4 branches.

Any idea on when new features might make it in to the core?

I do also have a few questions regarding standards adherence, and memory leak methods of detection.

I ask about the memory leak detection as passing test scripts through valgrind only led me to find leaks in the openssl_pkey_new() function.

There's two levels of leak detection - internal memory management &
valgrind. How to work with them:

Internal. Compile php with --enable-debug and run your tests. Leaks
will be reported by php.

External. Set USE_ZEND_ALLOC environment variable to 0. Set
ZEND_DONT_UNLOAD_MODULES to 1. Run it under valgrind, see what it reports.

You might want to do something like

Is this in the documentation? If so could you provide a link?

USE_ZEND_ALLOC=0 ZEND_DONT_UNLOAD_MODULES=1
TEST_PHP_EXECUTABLE=./sapi/cli/php php ./run-tests.php -m ext/openssl/

It seems your API is good.
Isn't it just there aren't people who would like to
responsible?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Stas Malyshev — view source

unread

Hi!

On that note Mr. Malyshev has indicated (in addition to several other
threads on the internals list) that no new features will be added in
5.3 or 5.4 branches.

5.3 is out of the question, I think, but for 5.4 small self-contained
additions - like adding a couple of functions here and there - may be fine.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by jason.gerfen@gmail.com — view source

unread

Hi!

On that note Mr. Malyshev has indicated (in addition to several other
threads on the internals list) that no new features will be added in
5.3 or 5.4 branches.

5.3 is out of the question, I think, but for 5.4 small self-contained
additions - like adding a couple of functions here and there - may be fine

Good to know, I appreciate your time

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

5.3 is out of the question, I think, but for 5.4 small self-contained additions - like adding a couple of functions here and there - may be fine.

5.3 is out of the question, I think, but for 5.4 small self-contained
additions - like adding a couple of functions here and there - may be fine.