Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ext/sockets: inet family conversions internal changes. #17297

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

devnexen
Copy link
Member

No description provided.

@@ -13,7 +13,7 @@ extern zend_result php_string_to_if_index(const char *val, unsigned *out);

#ifdef HAVE_IPV6
/* Sets addr by hostname, or by ip in string form (AF_INET6) */
int php_set_inet6_addr(struct sockaddr_in6 *sin6, char *string, php_socket *php_sock) /* {{{ */
zend_result php_set_inet6_addr(struct sockaddr_in6 *sin6, char *string, php_socket *php_sock) /* {{{ */
Copy link
Member

Choose a reason for hiding this comment

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

You could use bool here instead of zend_result this would preserve the existing semantics

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 did that at first but was to make it consistent with the callers e.g. here.

Copy link
Member

Choose a reason for hiding this comment

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

How public are these APIs? Because it might make more sense to change the callers API to return a bool (which is static here).

Copy link
Member Author

Choose a reason for hiding this comment

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

fair points

}
/* }}} */
#endif

/* Sets addr by hostname, or by ip in string form (AF_INET) */
int php_set_inet_addr(struct sockaddr_in *sin, char *string, php_socket *php_sock) /* {{{ */
zend_result php_set_inet_addr(struct sockaddr_in *sin, char *string, php_socket *php_sock) /* {{{ */
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

}
/* }}} */

/* Sets addr by hostname or by ip in string form (AF_INET or AF_INET6,
* depending on the socket) */
int php_set_inet46_addr(php_sockaddr_storage *ss, socklen_t *ss_len, char *string, php_socket *php_sock) /* {{{ */
zend_result php_set_inet46_addr(php_sockaddr_storage *ss, socklen_t *ss_len, char *string, php_socket *php_sock) /* {{{ */
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

@devnexen devnexen force-pushed the socketaddr_conv_changes branch 2 times, most recently from 1690118 to 9beedba Compare December 29, 2024 17:49
@devnexen devnexen force-pushed the socketaddr_conv_changes branch from 9beedba to 99f91bc Compare December 29, 2024 18:49
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.

2 participants