Add support for IPv6 scoped addresses (RFC4007)

[foxxx0]

In order to translate interface names in such scoped addresses the
required LibC binding to if_nametoindex() has been added.
This method obviously only works for interfaces (devices) that are
actually present on the system.

The binding for the reverse operation if_indextoname() has also been
added, although its usage is a bit more cumbersome due to LibC::Char*
buffer handling. The necessary buffer length has been placed into the
constant LibC::IF_NAMESIZE, which appears to be 16u8 on unix-like
systems and 257u16 on windows.
This could potentially be reworked via a preprocessor block at
compile-time as indicated by some folks over on discord, I currently do
not know how to achieve that though.

Scoped identifiers are only valid for link-local (fe80::) addresses, e.g. fe80::1%eth0

References:

• https://datatracker.ietf.org/doc/html/rfc4007

Fixes #15264 .

[straight-shoota]

Could you please open an issue as a feature request first? We ask this to separate high-level discussion of the feature (in the issue discussion) from details of this specific PR (PR discussion). Ref: https://github.com/crystal-lang/crystal/blob/master/CONTRIBUTING.md#using-the-issue-tracker

I'm marking this PR as draft for now.

[foxxx0]

ah well, shame on me.

I've just created #15264 now to discuss the feature itself.

[foxxx0]

Alright, here we go.

Not sure why, but something is not happy in the workflow runs in my fork.

• AArch64 is always skipped immediately
• x86_64-windows-dlls and x86_64-windows-libs fail building libiconv

Apart from that the remaining workflows appear to be happy with it.

Let me know if you'd like more comments in the added code or want something changed.

[foxxx0]

Thanks for the detailed review and constructive feedback/suggestions.
I've updated the branch/PR accordingly.

Following the contributing guidelines I have done so with separate commits but I'd be happy to rebase+squash/fixup if desired for a "cleaner" history before merging into master.

[straight-shoota]

Don't worry about squash/rebase. We do that when merging.
Commit history in the PR is best left as is.

[foxxx0]

Thanks for the explanation and dilligence in the reviews, that goes to everyone who has taken part here, much appreciated.

I've pushed two more commits, one to switch the macro to use LibC.has_method? and another one to switch zone_id to an Int32 and allow generic Int as function parameters where applicable.

[straight-shoota]

I'm not happy about the Int restrictions. IMO they're wrong. We should not change the existing type restrictions, but we should avoid introducing new ones.

Int includes all integer types, so it's allowed to call the method with Int64::MAX, for example. But it's clear that such a value would never fit. With an Int restriction, there's no compile-time validation. It'll only raise at runtime.
With an Int32 restrictions, however, we still get implicit autocasting for all compatible types. But if the source type is bigger than Int32, we need to cast explicitly at the call site. This can help identify issues at compile time.

IMO input and output types should always be identical, so Int32 both for the parameter type and the return type.

[foxxx0]

That is definitely a good catch. I've adjusted zone_id to Int32 everywhere now.

[HertzDevil]

Windows CI is failing, because Win32 presumably does not set errno at all, and the ENOENT you're seeing probably came from another C runtime function call:

If the if_nametoindex function fails and returns zero, it is not possible to determine an error code.

[foxxx0]

Windows CI is failing, because Win32 presumably does not set errno at all, and the ENOENT you're seeing probably came from another C runtime function call:

If the if_nametoindex function fails and returns zero, it is not possible to determine an error code.

Unfortunately I can't seem to find any useful errno documentation for windows w.r.t. if_indextoname() / if_nametoindex().
Errors from if_nametoindex() are already handled regardless of errno via checking zone_id.positive?.
I have now adjusted the error handling for if_indextoname(), since a NULL pointer always indicates an error, regardless of what errno may or may not be present. For platforms with errno support, which include the following to my knowledge:

• linux
• bsd
• darwin
• solaris
  the Errno.value is inspected and appended to the exception message unless it is Errno::NONE.
  The corresponding spec is adjusted accordingly.

[foxxx0]

Are we good to merge now?

(I'd really like this to be part of the 1.17.0 release 🙂 )

[RX14]

Sorry for the post-merge comment, this is a fantastic PR I will get a bunch of use out of.

However, isn't it actually #to_s which i more important to document the representation of? #to_s is expected to be used in application code, whereas #inspect is for debugging, and happens to use #to_s here.

Additionally, would it make sense for #inspect to show the interface by name, since performance is a non-issue? With fallback to zone number if #inspect fails.

[straight-shoota]

Improving documentation sounds good for a follow-up 👌

I'm not sure about name vs. id. #to_s is supposed to be a user-friendly format, so that would be an argument to show the name instead of the id there...

[foxxx0]

Modifying the #to_s output would be a breaking change. There are likely lots of things/applications/libs that occasionally convert an IPv4/IPv6 address to a string and handing them off to whatever. Each of these implementations would then potentially need adjusting if there was suddenly a %<zone_id> suffix as part of the string.

I do think a separate #to_s_rfc4007? method might be useful to get that exact format. With an optional boolean to chose between numerical zone_id and link_local_interface.

[Sija]

I do think a separate #to_s_rfc4007? method might be useful to get that exact format. With an optional boolean to chose between numerical zone_id and link_local_interface.

Sounds like a great idea. I'd suggest using a dedicated named argument in to_s instead though.

[RX14]

Correct me if I'm wrong, but the current state of this PR looks like to_s is modified as well as inspect.

I'd prefer for inspect to by default include the zone, by default with name.

[straight-shoota]

Yes, this PR adds the zone ID to the output of both, #to_s and #inspect (the latter delegates to the former).
We missed adding a spec for that to make it explicit.
Considering https://github.com/crystal-lang/crystal/pull/15263/files#r2103365809 this might have been an unintended change and we should revert that.

I'd prefer for inspect to always include the zone.

Agreed. The information should be there. But should it be ID or name?
Maybe ID because it's sufficient and more succinct?

[foxxx0]

After taking another look, the #to_s always returned the address accompanied by the port. If one wanted to obtain just the address, there is an #address method, which hasn't been modified by this PR, i.e. it doesn't include any %<zone_id>, neither numerical nor string.

It is indeed true, that we've now implicitly changed the #to_s behavior with a potentially breaking formatting change. This was definitely an oversight as there is also no spec for it.

I can follow up with another PR for adding an optional bool to #to_s that controls whether or not to include the zone_id.

Should we default to always include it or omit it in #to_s to keep previous behavior and only always include it in #inspect?
Furthermore, when including it, do we use numerical ID or interface name?
I'd be fine attempting #link_local_interface and fallback to using numerical @zone_id if it failed. Thoughts?

[foxxx0]

One more thing worth noting:
Since zone_id can only ever be set (i.e. .positive?) on link-local ipv6 addresses, the format change doesn't really affect anything.
If at all, it actually might fix certain implementations since link-local ipv6 addresses might not even work properly without the zone_id.

But I still think we can make improvements in both the comments/docs, default behavior and obviously also add some specs for that.

[RX14]

Given the effect is only for link-local addresses, I think including the zone ID by default makes sense for both to_s and inspect. However, looking up the interface name has a performance impact so I believe it should only be on by default for inspect.