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

Ensure setting / getting the traffic class on an ipv4 only system wor… #7278

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

Projects
None yet
3 participants
@normanmaurer
Member

normanmaurer commented Oct 5, 2017

…ks when using the native transport.

Motivation:

We tried to set IPV6 opts on an ipv4 only system and so failed to set / get the traffic opts. This resulted in a test-error when trying to compile netty on ipv4 only systems.

Modifications:

Use the correct opts depending on if the system is ipv4 only or not.

Result:

Be able to build and use on ipv4 only systems.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 5, 2017

Member

@Scottmitch this was the cause of the build failure on docker (which is ipv4 only).

Member

normanmaurer commented Oct 5, 2017

@Scottmitch this was the cause of the build failure on docker (which is ipv4 only).

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer
Member

normanmaurer commented Oct 7, 2017

Show outdated Hide outdated transport-native-unix-common/src/main/c/netty_unix_socket.c Outdated
if (netty_unix_socket_getOption0(fd, IPPROTO_IPV6, IPV6_TCLASS, &optval, sizeof(optval)) == -1) {
if (errno == ENOPROTOOPT) {
if (netty_unix_socket_getOption(env, fd, IPPROTO_IP, IP_TOS, &optval, sizeof(optval)) == -1) {
return -1;

This comment has been minimized.

@pmarks-net

pmarks-net Oct 9, 2017

Shouldn't this call getOptionHandleError() before every return -1?

@pmarks-net

pmarks-net Oct 9, 2017

Shouldn't this call getOptionHandleError() before every return -1?

This comment has been minimized.

@normanmaurer

normanmaurer Oct 22, 2017

Member

@pmarks-net no... getOptionHandleError is only needed when calling netty_unix_socket_getOption0 (in which case we do it in this method, but not when using ´netty_unix_socket_getOption´ (no trailing 0).

@normanmaurer

normanmaurer Oct 22, 2017

Member

@pmarks-net no... getOptionHandleError is only needed when calling netty_unix_socket_getOption0 (in which case we do it in this method, but not when using ´netty_unix_socket_getOption´ (no trailing 0).

Ensure setting / getting the traffic class on an ipv4 only system wor…
…ks when using the native transport.

Motivation:

We tried to set IPV6 opts on an ipv4 only system and so failed to set / get the traffic opts. This resulted in a test-error when trying to compile netty on ipv4 only systems.

Modifications:

Use the correct opts depending on if the system is ipv4 only or not.

Result:

Be able to build and use on ipv4 only systems.
@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer
Member

normanmaurer commented Oct 22, 2017

@carl-mastrangelo

No other concerns from me, but I think you should @pmarks-net to weigh in.

@pmarks-net

Okay, now that I understand the difference between setOption and setOption0, this change looks good.

I am a bit concerned about the #ifdef __linux__, because there may exist other platforms where dualstack sockets need to set both, but if that's a bug then it was already present.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 24, 2017

Member

@pmarks-net like you said if this is a problem is existed before and we can address it in another PR once we are sure. Thanks for the review again!

Member

normanmaurer commented Oct 24, 2017

@pmarks-net like you said if this is a problem is existed before and we can address it in another PR once we are sure. Thanks for the review again!

@normanmaurer normanmaurer self-assigned this Oct 24, 2017

@normanmaurer normanmaurer added the defect label Oct 24, 2017

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 24, 2017

Member

Cherry-picked into 4.1 (d8e187f)

Member

normanmaurer commented Oct 24, 2017

Cherry-picked into 4.1 (d8e187f)

@normanmaurer normanmaurer deleted the native_traffic_class_ipv4 branch Oct 24, 2017

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