-
Notifications
You must be signed in to change notification settings - Fork 30k
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
url,src: simplify ipv6 logic by using uv_inet_pton #38842
Conversation
src/node_url.cc
Outdated
@@ -78,7 +81,7 @@ class URLHost { | |||
union Value { | |||
std::string domain_or_opaque; | |||
uint32_t ipv4; | |||
uint16_t ipv6[8]; | |||
uint16_t ipv6[NS_IN6ADDRSZ / NS_INT16SZ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just 8
was a bit clearer here, to be honest…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think glibc is the gold standard for code readability here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed it back.🐣
memset(buf, 0, sizeof(buf)); | ||
memcpy(*ipv6, input, sizeof(const char) * length); | ||
|
||
int ret = uv_inet_pton(AF_INET6, *ipv6, buf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory I'm +1 but we need to make sure that uv_inet_pton
matches what the url standard expects here. The reason for the more complicated implementation is that it's what the standard spec specifically calls out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- It passes wpt;
uv_inet_ntop
andinet_ntop
breaksURLHost::ToString()
because it does not match web standard...:x.x.x.x
. So I do not modify logic by using*_ntop
.
PR-URL: #38842 Reviewed-By: Anna Henningsen <anna@addaleax.net>
Landed in c109a6c |
PR-URL: #38842 Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: #38842 Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: #38842 Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: #38842 Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: nodejs#38842 Reviewed-By: Anna Henningsen <anna@addaleax.net>
No description provided.