Skip to content

Commit

Permalink
Simplify Address#getInfo() native implementation
Browse files Browse the repository at this point in the history 
Don't apply tricks and rely on OS behavior to resolve addresses. It is the
caller's responsibility to properly process socket addresses if more than
one exists.

This mimics the behavior of HTTPd's listen.c for apr_sockaddr_info_get().
  • Loading branch information
@michael-o
michael-o committed May 26, 2021
1 parent 075db78 commit 420cd1c
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 41 deletions.
42 changes: 1 addition & 41 deletions native/src/address.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,59 +23,19 @@ TCN_IMPLEMENT_CALL(jlong, Address, info)(TCN_STDARGS,
{
apr_pool_t *p = J2P(pool, apr_pool_t *);
TCN_ALLOC_CSTRING(hostname);
char *sp = NULL;
int scope_id = 0;
apr_sockaddr_t *sa = NULL;
apr_sockaddr_t *sl = NULL;
apr_int32_t f;


UNREFERENCED(o);
GET_S_FAMILY(f, family);
#if APR_HAVE_IPV6
if (hostname) {
/* XXX: This only works for real scope_id's
*/
if ((sp = strchr(J2S(hostname), '%'))) {
*sp++ = '\0';
scope_id = atoi(sp);
}
}
#endif
TCN_THROW_IF_ERR(apr_sockaddr_info_get(&sa,
J2S(hostname), f, (apr_port_t)port,
(apr_int32_t)flags, p), sa);
sl = sa;
/*
* apr_sockaddr_info_get may return several address so this is not
* go to work in some cases (but as least it works for Linux)
* XXX: with AP_ENABLE_V4_MAPPED it is going to work otherwise it won't.
*/
#if APR_HAVE_IPV6
if (hostname == NULL) {
/* Try all address using IPV6 one */
while (sl) {
if (sl->family == APR_INET6)
break; /* Done */
sl = sl->next;
}
/* If we don't find an IPv6 address, use the original one */
if (sl == NULL) {
sl = sa;
}
}
if (sp) {
/* Set the provided scope id
* APR lack the api for setting this directly so lets presume
* the sin6_scope_id is present everywhere
*/
sl->sa.sin6.sin6_scope_id = scope_id;
}
#endif

cleanup:
TCN_FREE_CSTRING(hostname);
return P2J(sl);
return P2J(sa);
}

TCN_IMPLEMENT_CALL(jstring, Address, getnameinfo)(TCN_STDARGS,
Expand Down
5 changes: 5 additions & 0 deletions xdocs/miscellaneous/changelog.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@
</p>
</section>
<section name="Changes in 1.2.29">
<changelog>
<update>
Simplify <code>Address.getInfo()<code> native implementation. (michaelo)
</update>
</changelog>
</section>
<section name="Changes in 1.2.28">
<changelog>
Expand Down

8 comments on commit 420cd1c

@mturk
Copy link
Contributor

@mturk mturk commented on 420cd1c May 26, 2021

Choose a reason for hiding this comment

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

You cannot break ABI just like that. Please revert the change

@michael-o
Copy link
Member Author

@michael-o michael-o commented on 420cd1c May 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Can you explain why this breaks ABI for you? The Javadoc of Address does not document anything about the code I have changed. Also no behavior is documented.

@mturk
Copy link
Contributor

@mturk mturk commented on 420cd1c May 26, 2021

Choose a reason for hiding this comment

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

Can you explain why you made a change? Have you tested it on Windows?
When you require a "caller (in our case Tomcat) to change responsibility" it means you are requiring to change the ABI.
You can create a new branch of Tomcat Native and require that.

@michael-o
Copy link
Member Author

@michael-o michael-o commented on 420cd1c May 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Can you explain why you made a change? Have you tested it on Windows?
When you require a "caller (in our case Tomcat) to change responsibility" it means you are requiring to change the ABI.
You can create a new branch of Tomcat Native and require that.

Please read: https://www.mail-archive.com/[email protected]/msg152629.html
Yes, I have compiled (wtih you CMSC distro) and tested multiple times on Windows deployments, Windows 10 and Windows Server with IPv4 and IPv6. On hardware and in a VMWare VM. I am also checked how this is implemented in APR and how different OS behavior is that they all prefer IPv6 with UNSPEC unless you configure differently. Moreover, Tomcat does not even follow the next pointer in contrast to HTTPd (listen.c). So this is somewhat random already.

@mturk
Copy link
Contributor

@mturk mturk commented on 420cd1c May 26, 2021

Choose a reason for hiding this comment

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

Please revert the change because it breaks bunch of current deployments. Like said, you can create a new branch that will not break current Tomcat ABI, and we can discuss it there.

@michael-o
Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted.

@mturk
Copy link
Contributor

@mturk mturk commented on 420cd1c May 26, 2021

Choose a reason for hiding this comment

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

Thanks

@michael-o
Copy link
Member Author

Choose a reason for hiding this comment

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

Please see #9

Please sign in to comment.