Refactor subset of openssl module. #3578
Conversation
|
Windows builds failing with "'php_openssl_cipher_iv_length': definition of dllimport function not allowed", so I guess PHPAPI is not quite right. @weltling How are API functions correctly defined in extensions? |
devnexen
force-pushed
the
openssl_refactoring
branch
from
d00d05f to
f920648
Compare
|
@nikic extensions that exports should define their own export decl (not that it should matter to use PHPAPI tho), many do that by defining their own PHP_XXX_API macro in ext/xxx/php_xxx.h and then export them like that, it should be all that we need afair |
|
Comment on behalf of petk at php.net: Labelling |
devnexen
force-pushed
the
openssl_refactoring
branch
from
231352d to
a92947b
Compare
devnexen
force-pushed
the
openssl_refactoring
branch
from
506f6a4 to
432d3c7
Compare
devnexen
force-pushed
the
openssl_refactoring
branch
from
432d3c7 to
1e3e20a
Compare
|
@devnexen Seems ok from the quick (just one NIT). Could you pls also squash it. |
devnexen
force-pushed
the
openssl_refactoring
branch
from
1e3e20a to
ba3e500
Compare
devnexen
force-pushed
the
openssl_refactoring
branch
from
ba3e500 to
69b86a6
Compare
|
@bukka sorry to bother :-) but it s just to avoid further rebasing. Thanks. |
devnexen
force-pushed
the
openssl_refactoring
branch
2 times, most recently
from
260b4bb to
4148257
Compare
|
@devnexen Sorry for taking so long to do a better review but added few comments to the API. Will merge it as soon as it's addressed. |
devnexen
force-pushed
the
openssl_refactoring
branch
from
fc2f4ac to
64c6f62
Compare
devnexen
force-pushed
the
openssl_refactoring
branch
2 times, most recently
from
274bba1 to
2c3dd0c
Compare
ext/openssl/openssl.c
Outdated
| RETVAL_FALSE; | ||
| if (outbuf) | ||
| zend_string_release_ex(outbuf, 0); | ||
| outbuf = 0; |
There was a problem hiding this comment.
This should probably be outbuf = NULL? Same in a few cases below.
I'm also wondering why this code needed a change. Not that it's necessarily wrong, but I would expect that if outbuf needs to be freed here, then that have also been the case before this refactoring. Or am I missing something?
There was a problem hiding this comment.
By itself the change is not feature changing for sure but it gives the possiblity to uses the api outside of this module (I have further plans personally).
There was a problem hiding this comment.
Sorry, I didn't mean the overall change, but the added zend_string_release call here.
There was a problem hiding this comment.
When I launched the unit tests I got leak warnings. Not against changing to NULL for the rest.
Proposal to abstract a subset of the openssl module, to be able to use two ways encryption outside of this context.
devnexen
force-pushed
the
openssl_refactoring
branch
from
2c3dd0c to
857d8a1
Compare
|
Merged as 62c7432 ! Thanks! I can't close it as https://qa.php.net/pulls/# still seems to be not working for me (no PR's listed). Think it would be good idea to add committers to the project so everyone can handle PR in here which would be much simpler even when qa.php.net works. Also it would good that we could use assigning and official approvals. In the meantime please someone close this. |
7 participants
Proposal to abstract a subset of the openssl module,
to be able to use two ways encryption outside of this context.