On Wed Feb 15, 2023 at 10:26 AM CST, Lassi Pulkkinen wrote:
> Allow IPv6 without port> Allow service name in address string
Is this really required? Now the `service` parameter is redundant.
In any case, none of the expectations of a valid address string format
(whether it is hostname/IPv4/IPv6; has port/service) nor which
service/port has priority is documented. All this should be documented
so that they aren't just implementation details.
>> Signed-off-by: Lassi Pulkkinen <lassi@pulk.fi>> ---> net/dial/resolve.ha | 57 +++++++++++++++++++--------------------------> 1 file changed, 24 insertions(+), 33 deletions(-)>> diff --git a/net/dial/resolve.ha b/net/dial/resolve.ha> index 9fa4e799..9361f5dc 100644> --- a/net/dial/resolve.ha> +++ b/net/dial/resolve.ha> @@ -18,60 +18,51 @@ export fn resolve(> addr: str,> service: str,> ) (([]ip::addr, u16) | error) = {> - let port = if (strings::hasprefix(addr, '[')) {> - // [::1]:80> - yield match (strings::index(addr, "]:")) {> + if (strings::hasprefix(addr, '[')) {> + match (strings::index(addr, "]")) {> case let i: size =>> - const sub = strings::sub(addr, i + 2, strings::end);> + const sub = strings::sub(addr, i + 1, strings::end);> addr = strings::sub(addr, 1, i);> - yield match (strconv::stou16(sub)) {> - case let u: u16 =>> - yield u;> - case =>> + // [::1]> + if (len(sub) == 0) yield;> + // [::1]:80> + if (!strings::hasprefix(sub, ":")) {> return invalid_address;> };> + service = strings::sub(sub, 1, strings::end);
Shouldn't `service` parameter have priority over what comes with the
address?
> case void =>> return invalid_address;> };> } else {> - yield match (strings::index(addr, ':')) {> - case void =>> - yield match (strconv::stou16(service)) {> - case let u: u16 =>> - yield u;> - case =>> - yield 0u16;> - };> + match (strings::index(addr, ':')) {> case let i: size =>> - const sub = strings::sub(addr, i + 1, strings::end);> + service = strings::sub(addr, i + 1, strings::end);> addr = strings::sub(addr, 0, i);> - yield match (strconv::stou16(sub)) {> - case let u: u16 =>> - yield u;> - case =>> - return invalid_address;> - };> + case void => void;> };> };> > - if (service == "unknown" && port == 0) {> - return unknown_service;> + const port = match (strconv::stou16(service)) {> + case let p: u16 =>> + yield p;> + case =>> + if (service == "unknown") {> + return unknown_service;> + };> + yield match (lookup_service(proto, service)) {> + case let p: u16 =>> + yield p;> + case void =>> + return unknown_service;> + };> };> > let addrs = resolve_addr(addr)?;> - if (port == 0) match (lookup_service(proto, service)) {> - case let p: u16 =>> - port = p;> - case void => void;> - };> > // TODO:> // - Consult /etc/services> // - Fetch the SRV record> > - if (port == 0) {> - return unknown_service;> - };> if (len(addrs) == 0) {> return dns::name_error;> };> -- > 2.39.1
--
> Now the `service` parameter is redundant.
No it isn't. It's used to provide a default, whereas `addr` would
typically be user-provided.
> In any case, none of the expectations of a valid address string format> (whether it is hostname/IPv4/IPv6; has port/service) nor which> service/port has priority is documented. All this should be documented> so that they aren't just implementation details.
This is documented on the `dial` function; that should indeed be
updated to reflect these changes. (Conspicuously, it leaves the
port-less case of IPv6 addresses unmentioned.)
> Shouldn't `service` parameter have priority over what comes with the> address?
As I already implied, no, nor did it ever work this way. This is also
documented on `dial`.
Slight correction: the service parameter is also meant to be used for
SRV resolution, which I forgot about because it hasn't been implemented
yet. A service name provided in the address probably shouldn't affect
that, making the case for allowing it a bit questionable. Up to the
maintainers, I guess.
On Wed Feb 15, 2023 at 3:45 PM CST, Lassi Pulkkinen wrote:
> > Now the `service` parameter is redundant.>> No it isn't. It's used to provide a default, whereas `addr` would> typically be user-provided.>> > In any case, none of the expectations of a valid address string format> > (whether it is hostname/IPv4/IPv6; has port/service) nor which> > service/port has priority is documented. All this should be documented> > so that they aren't just implementation details.>> This is documented on the `dial` function; that should indeed be> updated to reflect these changes. (Conspicuously, it leaves the> port-less case of IPv6 addresses unmentioned.)
Oh, I see. Then net::dial:dial should be referenced in the
documentation. net::dial::resolve is a public, standalone function, it
should be properly documented.
>> > Shouldn't `service` parameter have priority over what comes with the> > address?>> As I already implied, no, nor did it ever work this way. This is also> documented on `dial`.
> Oh, I see. Then net::dial:dial should be referenced in the> documentation. net::dial::resolve is a public, standalone function, it> should be properly documented.
Agreed. I'll wait for code review before updating the docs.