Skip to content

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Oct 5, 2024

No description provided.

@Girgias
Copy link
Member Author

Girgias commented Oct 5, 2024

@cmb69 quick question, I see that suseconds_t is defined in win32/time.h for Windows, do I just need to include time.h at the beginning of the file for the definition to be found?

I'm guessing if I do <time.h> it will load the system header instead of the one from win32.

@cmb69
Copy link
Member

cmb69 commented Oct 5, 2024

@Girgias, you can use

#include "win32/time.h"

@Girgias Girgias force-pushed the streamfuncs-timeout branch from 84861ea to e595ede Compare October 5, 2024 21:10
@Girgias Girgias force-pushed the streamfuncs-timeout branch from e595ede to 822dc1a Compare October 6, 2024 23:21
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks reasonable.

Comment on lines +830 to +834
} else if (usec >= 1000000) {
php_error_docref(NULL, E_WARNING, "must be less than 1000000");
if (UNEXPECTED(EG(exception))) {
RETURN_THROWS();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this just be a zend_argument_value_error? We've shipped improved parameter validation all the time without an RFC and here the <0 case already throws.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if people rely on passing a large int or not for the microseconds

zend_argument_value_error(4, "must be greater than or equal to 0");
RETURN_THROWS();
} else if (usec < 0) {
zend_argument_value_error(5, "must be greater than or equal to 0");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could update the message to must be between 0 and 999999 to be consistent with stream_set_timeout.


/* prepare the timeout value for use */
struct timeval *tv_pointer;
if (timeout < 0.0 || timeout >= (double) PHP_TIMEOUT_ULL_MAX / 1000000.0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we no longer need the overflow check?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably do, actually.

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.

3 participants