Fix formatting of non-loopback IPv6 addresses

[mdarse]

Hi,

Currently the ip_address_to_string function formats all IPv6 addresses (except loopback) to the form 8193::3512::0::0::0::0::0::1 (which is invalid) instead of one of the following representations:

• 2001:db8:0:0:0:0:0:1
• 2001:0db8:0000:0000:0000:0000:0000:0001
• 2001:db8::1 (IETF’s recommended canonical format).

I encountered the issue using Mist while binding to a specific interface. It uses ip_address_to_string to print Listening on http://[0::0::0::0::0::0::0::0]:8080 in the console on startup, which is obviously not a valid URL in this instance.

This implement RFC 5952 canonical format. Previous loopback short-hand is the default and not a special case anymore.

Please tell me if the formatting logic would be better placed in an internal module or if it’s fine as-is in the root module.

[rawhat]

Hey, very cool! Thank you for looking into this. I appreciate the effort.

It looks like erlang has inet:ntoa that seems to behave somewhat similarly to what you have here. I'm not super familiar enough with the canonical formatting to say whether it would work here.

If it does, it seems like maybe it would be nice to just replace all my (admittedly, pretty dumb) custom formatting and just use that?

Let me know what you think!

[mdarse]

Damn, you’re right. I just checked inet:ntoa and it does what we need. I found that strange to not have this kind of network stuff in Erlang 😅.

It needs a bit of massage since the return value is not always a Charlist, from what I understand of the implementation that’s when an IP field is out of his legal range, which is possible with Ints of IpAddress. Currently ip_address_to_string returns a String, not a Result(String).

Is it ok to crash if the fields are out of range? Or should we either:

• Make ip_address_to_string return a Result (breaks backward compatibility)
• Move this check upstream (where IpAddresses are constructed) and make the type opaque.

Edit:

• Or use a sentinel value like the unicast unspecified address (::) as a fallback, not the prettiest though.

[rawhat]

Hmm... I need to think about that. Making it opaque kinda sucks since you couldn't pattern match on it, which maybe isn't really an issue? I have no idea how people are using it right now 😅

I'd be okay panicking if an improper IP is passed for binding since it's only at initialization, and I'm already doing that for SSL arguments.

So it's a bit of an annoying situation. I do feel like allowing people to construct invalid IPs is wrong, but the loss of pattern matching takes away a lot. I don't mind the to_string returning a Result really, but it would be nice to avoid that.

Edit: Is this a good use of phantom types? 🤔 Like the function requires an IpAddress(Valid)? Would still be a breaking change, but might be nicer.

[rawhat]

I don't have a good answer for the invalid case, but that was already an issue with what was there. Seems totally reasonable to just use what you have here.

Thank you!