On Friday 20 October 2006 14:41, Rémi Denis-Courmont wrote: > Hello,
Hi, > Some random comments: Thanks for those; see my response inlined below: > The proposed API adds a new IPv6-level socket option > (IPV6_ADDRESS_PREFERENCES). IMHO, it ought to > also specify that this can also be used as a > "non-sticky" option through sendmsg() ancilliarry > data, like other IPv6 options found in the advanced > IPv6 API. But this is specified (and somehow discouraged) in the Appendix: Doing per packet address selection would certainly be costly because running the algorithm isn't as cheap as setting a field in a packet. Also, note that there's already setsockopt only and ancillary only options (e.g. RFC3493 has IPV6_UNICAST_HOPS and IPV6_MULTICAST_HOPS for get/setsockopt, while RFC3542 has IPV6_HOPLIMIT only for ancillary.) > [...] > > The struct definition should probably be moved into > an appendix as an example implementation detail. I do not think that's an implementation detail. The addrinfo struct is part of the core of RFC3493, not of the appendix. > [...] > > The example getaddrinfo() is flawed. DO NOT let this > flow into an RFC... freeaddrinfo() must be applied > to the pointer returned by getaddrinfo(), i.e. the > pointer to the first struct addrinfo in the linked > list. Your code is essentially equivalent to > freeaddrinfo(NULL) ! Good catch, the initial addrinfo returned by getaddrinfo should be saved for later freeing. > Please specify the interaction between > AI_PREFERENCES and AI_PASSIVE - the current spec > seems to assume AI_PASSIVE is not set. Setting AI_PASSIVE means you want to bind to a socket without actually initiating communications on your own. In that case source address selection algorithm does not apply since there's no > More seriously, there are some design flaws in the > proposed getaddrinfo() extensions: > > For a start, there is a race condition if the > locally available addresses or the routing table > change between the getaddrinfo() invocation and > connect() calls, we may get unwanted results. Yes there might be race conditions; that is however not specific to the use of getaddrinfo() while expressing address selection preferences. > Maybe a solution would be to return ai_bindaddr and > ai_connaddr instead of just ai_addr. One goal of this API is to be preserve the existing basic IPv6 socket API. Besides, I don't understand how would ai_connaddr and ai_bindaddr solve the issue you describe. > Also, extending the byte size of an already existing > structure might pose some problem: imagine an > application initializes a "hint" struct addrinfo, > and passes it to a library API that would tweak it > depending on some configuration or whatever, then > the application would use the same hint as a > parameter to getaddrinfo(). Now what if the > application was built with the old-style struct > addrinfo without the ai_eflags, but the library uses > it? we risk a buffer overflow there. It should never be assumed (by application, library kernel...) that the ai_eflags field is present in a struct addrinfo if the struct doesn't have the AI_PREFERENCES flag set in its ai_flags field. Hence a buffer overflow shouldn't happen unless the library of you scenario is broken. > IMHO, it might be wiser to define extended > getaddrinfo, freeaddrinfo and struct addrinfo > variants (geteaddrinfo ?) - that woud allow to use > the ai_bindaddr and ai_connaddr thing at the same > time. IMHO it's nice to avoid introducing yet another name resolution primitive. Regarding ai_bindaddr and ai_connaddr, see my comment above. Thanks, --julien -------------------------------------------------------------------- IETF IPv6 working group mailing list ipv6@ietf.org Administrative Requests: https://www1.ietf.org/mailman/listinfo/ipv6 --------------------------------------------------------------------
