On 10/26/22 16:04, Zebediah Figura wrote:
On 10/25/22 20:18, Paul Gofman wrote:
On 10/25/22 19:04, Zebediah Figura (@zfigura) wrote:
Zebediah Figura (@zfigura) commented about server/sock.c:
+static struct rb_tree bound_addresses_tree = { addr_compare };
+static int is_blocking_addr( struct sock *sock, const union unix_sockaddr *addr ) +{ + if (!is_tcp_socket( sock )) return 0;
+ if (sock->family == WS_AF_INET) + { + if (addr->addr.sa_family != AF_INET) return 0; + if (!addr->in.sin_port) return 0; + return 1; + } + if (addr->addr.sa_family != AF_INET6) return 0; + if (!addr->in6.sin6_port) return 0; + return 1;
I think it'd be clearer if you had an "else if (sock->family == WS_AF_INET6)" branch, even though it's more lines of code.
Also, do we really need those sa_family checks? It skips the address lookup, but that doesn't seem necessary, and it's a bit distracting.
The check is here because in theory an app may supply the wrong address type for socket. In this case, if I don't skip the lookup here and happen to find the actually supplied address already bound we will return the wrong error code (that should fail in native bind() with a different error). I don't think such specific and weird condition warrants any code complication, but it also looks weird to search for a non matching address type for socket, so maybe we could keep the check in place?
Oh, right, I can't think. I guess in that case the question is the opposite—can we just check sa_family here and *not* sock->family?
If I am not missing anything, this is another aspect. Checking just sa_family would avoid weird access for ipv4 as ipv6 and vice versa from addr, but still family not matching between socket and addr will result in a bind error later and we ideally don't want to return wrong error code here if that doesn't cost too much. I redid the function in the patch update and now I believe it is much more readable, do you think it is still too messy?