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

Socket::IPAddress constructors #13382

Open
HertzDevil opened this issue Apr 24, 2023 · 9 comments · Fixed by #13422
Open

Socket::IPAddress constructors #13382

HertzDevil opened this issue Apr 24, 2023 · 9 comments · Fixed by #13422

Comments

@HertzDevil
Copy link
Contributor

The only ways to construct a Socket::IPAddress are by parsing a string or using the C representation that is not publicly documented:

Socket::IPAddress.new("#{192}.#{168}.#{1}.#{3}", 1234) # => Socket::IPAddress(192.168.1.3:1234)

addr = LibC::SockaddrIn.new(
  sin_family: LibC::AF_INET,
  sin_port: 1234.to_u16.byte_swap,
  sin_addr: LibC::InAddr.new(s_addr: 0x03_01_a8_c0),
)
Socket::IPAddress.from(pointerof(addr).as(LibC::Sockaddr*), sizeof(typeof(addr))) # => Socket::IPAddress(192.168.1.3:1234)

There should be simpler constructors to do this:

struct Socket::IPAddress
  def self.v4(x0, x1, x2, x3, *, port) : self
  end

  def self.v6(x0, x1, x2, x3, x4, x5, x6, x7, *, port) : self
  end

  def self.v4_mapped_v6(x0, x1, x2, x3, *, port) : self
  end
end
@straight-shoota
Copy link
Member

I'm wondering if the type restriction on the component parameters should actually be UInt8 and UInt16, respectively, instead of Int.
This would remove the need to validate the range. The caller would be responsible for converting to the expected type.
These constructors would probably often be used with literals where autocasting takes care of that implicitly.

@Sija
Copy link
Contributor

Sija commented May 3, 2023

@straight-shoota This would mean doing the same for every caller, sounds like avoidable PITA :)

@straight-shoota
Copy link
Member

Yeah, that might be too annoying for using it with arbitrary numbers.
However maybe we could have an overload for UInt8 to skip the validation and conversion when the types already match.

@HertzDevil
Copy link
Contributor Author

For #13191 it'll be nice to have StaticArray overloads as well because I am doing something like this currently:

# port is Int32 because that's what the existing constructor uses

if v4_fields = parse_v4_fields?(address)
  addr = v4(v4_fields[0], v4_fields[1], v4_fields[2], v4_fields[3], port: port.to_u16!)
elsif v6_fields = parse_v6_fields?(address)
  addr = v6(v6_fields[0], v6_fields[1], v6_fields[2], v6_fields[3], v6_fields[4], v6_fields[5], v6_fields[6], v6_fields[7], port: port.to_u16!)
else
  raise Error.new("Invalid IP address: #{address}")
end

@straight-shoota
Copy link
Member

Maybe static array should be supported for splatting: v4(*v4_fields, port: port.to_u16!) 😄

Alternatively, StaticArray#to_tuple could be a useful enhancement as the opposite of Tuple#to_static_array (#12930). In contrast to the latter, it should always be just require a cast for reinterpretation because the memory layout is identical.

@HertzDevil
Copy link
Contributor Author

It would be too easy to instantiate Tuple types with an extremely large number of generic arguments. I think #to_tuple gives people the wrong idea about when to or not to use StaticArray.

So I think only these overloads are necessary:

struct Socket::IPAddress
  def self.v4(x0 : Int, x1 : Int, x2 : Int, x3 : Int, *, port : Int) : self
  end

  def self.v4(fields : UInt8[4], *, port : UInt16) : self
  end

  # ditto for `.v6` and `.v4_mapped_v6`
end

@oprypin
Copy link
Member

oprypin commented May 16, 2023

I don't follow the argument- why did the parameters end up as Int and not UInt8? These are bytes. Why is there a run-time check when just specifying the correct type would ensure static safety?

@oprypin
Copy link
Member

oprypin commented May 16, 2023

There's also a possible argument against representing an IPv6 address as 8 integers that are clamped to 65536. As I understand, that grouping is mainly just for convenience in text representation, but in the end the address is also just bytes.

@straight-shoota
Copy link
Member

The idea was to have a convenience constructor that receives Int where the caller does not need to cast the type. And there's also an efficient constructor which receives UInt8 (in form of a static array), so you can avoid the runtime check. This constructor is less convenient to call, though. I think that can be acceptable.

However I'm wondering about concrete use cases for the convenience constructor with Int. I can't think of many, so maybe UInt8 would really be better?

Regarding the representation of IPv6 I suppose there are good reasons for both. The general concept is based on 16 bytes, but typical string representations use 8 numbers.
Accordingly, there are APIs which use one or the other (e.g. https://doc.rust-lang.org/std/net/struct.Ipv6Addr.html, https://pkg.go.dev/net#IP, https://docs.oracle.com/javase/8/docs/api/java/net/InetAddress.html#getByAddress-java.lang.String-byte:A-).

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

Successfully merging a pull request may close this issue.

4 participants