Skip to content

Conversation

bukka
Copy link
Member

@bukka bukka commented Jul 26, 2025

This tries to use strerror_r in networking and stream code. It required some extending of php_socket_strerror and php_socket_error_str which is not exactly beautiful but can't think about much nicer way atm.

@bukka bukka changed the base branch from master to PHP-8.3 July 26, 2025 15:26
bukka added a commit to bukka/php-src that referenced this pull request Jul 26, 2025
@bukka bukka force-pushed the stream_gh19248_strerror branch from 81f7547 to 469fc77 Compare July 26, 2025 15:26
@bukka
Copy link
Member Author

bukka commented Jul 26, 2025

It will need more testing. Need to figure out how to actually test the posix variant to make sure that it all works. Also need to figure out if we even have all those errors covered so there is still a bit of work.

@bukka bukka marked this pull request as draft July 26, 2025 15:29
@bukka bukka linked an issue Jul 26, 2025 that may be closed by this pull request
@bukka bukka force-pushed the stream_gh19248_strerror branch from 469fc77 to 0323cbf Compare August 12, 2025 18:19
bukka added a commit to bukka/php-src that referenced this pull request Aug 12, 2025
@bukka
Copy link
Member Author

bukka commented Aug 12, 2025

Ok so XSI variant (returning int) is used on FreeBSD and MacOS which is tested and showed an issue. I just fixed that.

Another thing is Windows. The fact that I replaced strerror with php_socket_strerror in some places means that Windows will now use php_win32_error_to_msg that uses FormatMessageW and results in a more explaining error. The side effect is that it breaks many tests because those are Windows specific messages but maybe it's not really a problem. But not sure if it's ok for stable version. Alternatively I could introduce a special variant for Windows using strerror_s . @cmb69 What do you think about this?

@cmb69
Copy link
Member

cmb69 commented Aug 12, 2025

I'm afraid that switching to php_win32_error_to_msg() is too much of a BC break – some code might actually "parse" the error messages, and there might be some tests relying on the messages. And IIRC, php_win32_error_to_msg() produces localized error messages, what might be even worse. So I think it would be better to go with strerror_s() for now, and only switch to php_win32_error_to_msg() for master.

@bukka bukka force-pushed the stream_gh19248_strerror branch from 0323cbf to 477c1d1 Compare August 31, 2025 21:26
bukka added a commit to bukka/php-src that referenced this pull request Aug 31, 2025
Or on Windows it is going to use either FormatMessageW or strerror_s
for compatibility with previous error messages.

It also needs to accomodate for GNU and BSD versions of strerror_r
returning different type.

Closes phpGH-19251
bukka added a commit to bukka/php-src that referenced this pull request Aug 31, 2025
Or on Windows it is going to use either FormatMessageW or strerror_s
for compatibility with previous error messages.

It also needs to accomodate for GNU and BSD versions of strerror_r
returning different type.

Closes phpGH-19251
@bukka bukka force-pushed the stream_gh19248_strerror branch from 477c1d1 to b3fdac9 Compare August 31, 2025 22:31
Or on Windows it is going to use either FormatMessageW or strerror_s
for compatibility with previous error messages.

It also needs to accomodate for GNU and BSD versions of strerror_r
returning different type.

Closes phpGH-19251
@bukka
Copy link
Member Author

bukka commented Sep 1, 2025

Ok I introduced a special php_socket_strerror_s variant that is used for the cases that were using strerror directly. This has special implementation on Windows to use strerror_s and for other platforms it is just a macro for php_socket_strerror. CI is green so hopefully all good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use strerror_r instead of strerror in stream and networking code
2 participants