Create SocketAddress from packed byte representation - #1692
Conversation
…ion. Motivation: A user should be able to create SocketAddress from packed bytes representation. Modifications: I added a new SocketAddress initializer which takes the IP address in ByteBuffer form. I have also added tests that ensure the initializer works properly. Result: We have a new way to initialize a SocketAddress from a byteBuffer.
|
Can one of the admins verify this patch? |
10 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
Lukasa
left a comment
Lukasa
left a comment
There was a problem hiding this comment.
Thanks for this patch!
As a quick note, we don't want to construct this from a packed SOCKS-formatted data structure, we'd like to construct it from just the raw address bytes themselves. The SOCKS formatting is necessary for the other use-case, but is not necessary for NIO more generically.
Motivation: We should be able to create SocketAddress from just the raw address bytes and not from packed SOCKS-formatted data structure. Modifications: Remove SocketAddress construction from packed SOCKS-formatted data structure. Result: Construct SocketAddress from just the raw address bytes.
Lukasa
left a comment
Lukasa
left a comment
There was a problem hiding this comment.
This looks really good! I have two minor notes and then I think this should be good to go.
Please also run the scripts/generate_linux_tests.rb script to add the extra hooks for testing on Linux.
| /// - ipAddress: The IP address, in ByteBuffer form. | ||
| /// - port: The target port. | ||
| /// - returns: the `SocketAddress` corresponding to this string and port combination. | ||
| /// - throws: may throw `SocketAddressError.failedToParseIPByteBuffer` if the IP address cannot be parsed or `SocketAddressError.unsupported` if the ATYP is wrong. |
There was a problem hiding this comment.
We aren't using an ATYP here and can never throw unsupported. Can we update this throws message?
| /// - port: The target port. | ||
| /// - returns: the `SocketAddress` corresponding to this string and port combination. | ||
| /// - throws: may throw `SocketAddressError.failedToParseIPByteBuffer` if the IP address cannot be parsed or `SocketAddressError.unsupported` if the ATYP is wrong. | ||
| public init(ipAddress: ByteBuffer, port: Int) throws { |
There was a problem hiding this comment.
One minor nit: can we change the argument to packedIpAddress? This will give us a slightly more descriptive name, and it'll avoid a type-based overload and replace it with a label-based overload.
Motivation: Make the code more consistent and fix some typos. Modifications: Renamed ipAddress into packedIpAddress. Added extra hooks for testing on Linux. Result: Some typos and refactoring changes.
| /// Unable to parse a given IP string | ||
| case failedToParseIPString(String) | ||
| /// Unable to parse a given IP ByteBuffer | ||
| case failedToParseIPByteBuffer(ByteBuffer) |
There was a problem hiding this comment.
I'm afraid we can't add new cases to enumerations: doing so is a Semver major change. Can we add a new struct to represent this error instead?
There was a problem hiding this comment.
I'm having trouble implementing the struct.
Could you enlighten me on this point ?
This is what i did :
public enum SocketAddressError: Error {
/// The host is unknown (could not be resolved).
case unknown(host: String, port: Int)
/// The requested `SocketAddress` is not supported.
case unsupported
/// The requested UDS path is too long.
case unixDomainSocketPathTooLong
/// Unable to parse a given IP string
case failedToParseIPString(String)
/// Unable to parse a given IP ByteBuffer
struct failedToParseIPByteBuffer: Error {
let address: ByteBuffer
}
}
// ...
throw SocketAddressError.failedToParseIPByteBuffer(address: packedIpAddress)There was a problem hiding this comment.
extension SocketAddressError {
public struct FailedToParseIPByteBuffer: Error, Hashable {
public var address: ByteBuffer
public init(address: ByteBuffer) {
self.address = address
}
}
}
throw SocketAddressError.FailedToParseIPByteBuffer(address: packedIpAddress)Motivation: Remove failedToParseIPByteBuffer from SocketAddressError enum as it forces Semver major change. Modifications: Add a new struct to represent the failedToParseIPByteBuffer error. Result: failedToParseIPByteBuffer error extracted into a struct.
|
@Lukasa Does the code look good to you or do you want me to make other changes ? |
|
@swift-nio-bot test this please |
Motivation: A new init was added to `SocketAddress` in apple#1692, but the casing of `packedIpAddress` is incorrect, it should be `packedIPAddress`. This hasn't been released yet so let's fix it while we still can! Modifications: - s/packedIpAddress/packedIPAddress Result: More consistent API
Motivation: A new init was added to `SocketAddress` in #1692, but the casing of `packedIpAddress` is incorrect, it should be `packedIPAddress`. This hasn't been released yet so let's fix it while we still can! Modifications: - s/packedIpAddress/packedIPAddress Result: More consistent API
3 participants
Closes #1649
Motivation:
A user should be able to create SocketAddress from packed bytes representation.
Modifications:
I added a new SocketAddress initializer which takes the IP address in ByteBuffer form.
I have also added tests that ensure the initializer works properly.
ByteBuffer inputs should be in the SOCKS representation:
As mentioned in #1648.
ATYP address type of following address:
Address field of following length:
Implemented address types:
I don't know if I followed the correct method and if there are scenarios that I haven't covered (for example if we want to initialize the IP address as a packed byte representation but the port as an Integer).
I don't have much knowledge on the subject and I would like to have your opinion on this PR !
Result:
We have a new way to initialize a SocketAddress from a byteBuffer.