Skip to content

Implement total Show functions for SockAddr#441

Closed
karknu wants to merge 2 commits intohaskell:masterfrom
karknu:karknu/total_show
Closed

Implement total Show functions for SockAddr#441
karknu wants to merge 2 commits intohaskell:masterfrom
karknu:karknu/total_show

Conversation

@karknu
Copy link
Contributor

@karknu karknu commented Mar 13, 2020 •
edited
Loading

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
kazu-yamamoto
kazu-yamamoto requested changes Mar 16, 2020
View reviewed changes
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
@karknu
Test cases for getNameInfo and show SockAddr
868ae19
@karknu
Implement total Show functions for SockAddr
cabc788 
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 •
edited
Loading

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
Merge PR haskell#441
7040ac3
@kazu-yamamoto
Copy link
Collaborator

kazu-yamamoto commented Mar 17, 2020

@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
@eborden eborden mentioned this pull request May 17, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kazu-yamamoto kazu-yamamoto Awaiting requested review from kazu-yamamoto

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@karknu @vdukhovni @kazu-yamamoto @eborden