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

Implement total Show functions for SockAddr #441

Closed
wants to merge 2 commits into from

Conversation

karknu
Copy link
Contributor

@karknu karknu commented Mar 13, 2020

The previous implementation of show SockAddr used unsafePerformIO and getNameInfo. getNameInfo depends on show SockAddr in case of a failure which causes exceptions within exceptions within exceptions...

This is not just a theoretical problem. It is possible to get getnameinfo to fail on OSX using a multicast address with a reserved scope. This is because OSX attempts to map the second 16 bit word of an IPv6 address to a device name.
Example from C's getnameinfo:

0xff01:0x000a:: -> ff01::%bridge0
0xff01:0x0010:: -> ff01::%utun0
0xff01:0x0011:: -> getnameinfo: Device not configured

The showHostAddress and showHostAddress6 are based on showIPv4 and
showIPv6 in the iproute package.

@kazu-yamamoto kazu-yamamoto self-requested a review March 16, 2020 03:40
Copy link
Collaborator

@kazu-yamamoto kazu-yamamoto left a comment

Choose a reason for hiding this comment

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

I agree to throw away getNameInfo for this purpose.

  1. Would you start with a test case of multicast address which fails on macOS?
  2. Then, add your fix code. Unfortunately, @vdukhovni improved the code of iproute recently. So, please check it out on github.

You can use push -f for this PR as you like.

karknu added 2 commits March 16, 2020 10:28
The previous implementation of show used unsafePerformIO and
getNameInfo. getNameInfo depends on show SockAddr incase of a failure.

The showHostAddress and showHostAddress6 are based on showIPv4 and
showIPv6 in the iproute package.
@karknu karknu force-pushed the karknu/total_show branch from 5dcd18c to cabc788 Compare March 16, 2020 09:44
@karknu
Copy link
Contributor Author

karknu commented Mar 16, 2020

1. Would you start with a test case of multicast address which fails on macOS?

@kazu-yamamoto I've added test cases for getNameInfo and show SockAddr. The test case works for multicast IPv6 address with reserved scope will fail on OSX with

    works for multicast IPv6 address with reserved scope FAILED [1]

Failures:

  tests/Network/SocketSpec.hs:267:9: 
  1) Network.Socket, show SocketAddr, works for multicast IPv6 address with reserved scope
       uncaught exception: IOException of type NoSuchThing
spec: encountered an exception while trying to report an exception.
One possible reason for this is that we failed while trying to encode an error message. Check that your locale is configured properly.
Test suite spec: FAIL

without my patch.

2. Then, add your fix code. Unfortunately, @vdukhovni improved the code of `iproute` recently. So, please check it out on github.

From what I can see @vdukhovni's changes did not affect the show functions for IPv4 and IPv6 addresses.

@karknu karknu requested a review from kazu-yamamoto March 16, 2020 11:32
@vdukhovni
Copy link

vdukhovni commented Mar 16, 2020

From what I can see @vdukhovni's changes did not affect the show functions for IPv4 and IPv6 addresses.

That's correct, I only changed the various getters and setters for the binary forms.
My next pull request implements an optimized ByteString Builder for IPv4 and IPv6, but that too does not affect the String Show instances. The bestgap function from that code can speed up the String conversion for IPv6 by about a factor of 1.5, but it is not clear this is worth making a change for.

https://github.com/kazu-yamamoto/iproute/pull/46/files#diff-f237be36f8b3d58098397869af5e00cdR229-R271

kazu-yamamoto added a commit to kazu-yamamoto/network that referenced this pull request Mar 17, 2020
@kazu-yamamoto
Copy link
Collaborator

@karknu @vdukhovni Thank you for clarification. This PR now looks great.

Rebased and merged. Thank you for your contribution!

It would be nice if we implemented getAddrInfo and getNameInfo purely in Haskell someday!

@karknu karknu deleted the karknu/total_show branch March 17, 2020 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants