On Wed, Sep 30, 2026 at 12:33:04AM +0900, Yeonggi Kim wrote: > From: Yeonggi Kim <[email protected]> > Hi Amaury, Frederic, > Following your comment on github issue #3503, here is a series which > sets the source address of the three packets emitted without connection > (Retry, stateless reset and Version Negotiation), as qc_snd_buf() > already does for connections using the listener socket. As you > suggested, each send function gets an extra source address parameter > next to the destination one, and each one is fixed in its own patch. > 1/5 MINOR: prerequisite, adds quic_sock_sendto(), a sendto()-like > function with an optional source address relying on the existing > cmsg_set_saddr(), and two inline helpers. The wildcard-only rule > of qc_may_use_saddr() (EINVAL on FreeBSD for a specific bind) is > moved into one of them so that it is shared. > 2/5 BUG/MEDIUM: Retry > 3/5 BUG/MINOR: stateless reset > 4/5 BUG/MINOR: Version Negotiation > 5/5 REGTESTS: new reg-tests/quic/retry_wildcard.vtc
Hi, I finally got the time to review your patches. I am fine with the general changes. Here is my comments about it. 1. When adding a source address as parameter, please put it before the destination in the function argument list. This ordering is used in most of our QUIC code, so it would be better to use the same one for clarity purpose. Also, it could be useful to rename "addr" argument to "dst" in send_retry()/send_version_negotiation() to remove any ambiguity. 2. As your commit messages noted, your serie introduces secondary evolutions on Retry/VN/Reset packets emission besides just setting an explicit source address. Indeed, your new helper quic_sock_sendto() retries emission on EINTR. IMO, this should be placed in a separate patch, as it can be considered as a separate bug fix. What should be done here is to introduce quic_sock_sendto() in the first patch without the while around sendmsg(). Then add a final patch after the whole serie to provide send retry for Retry/VN/Reset packets. 3. Similar to EINTR loop, new helper quic_sock_sendto() also specifies MSG_NOSIGNAL and MSG_DONTWAIT, which can also be considered as a separate change. The second one may be useful to ensure emission is never blocked, although it's not possible to retry on EAGAIN as with a dedicated connection socket. It may be worth to write a comment to highlight this. Regarding Linux manual, MSG_NOSIGNAL seems to be useful only to TCP sockets so I'm not sure this one is interesting. I'll have to look for more info here. 4. Finally, I have an idea to organize better the code and remove the need for the newly provided helper quic_dgram_reply_src(). First, write a new patch so that send_retry(), send_version_negotiation() and send_stateless_reset() both take the listener instance instead of the plain FD. Apply a similar change then to quic_sock_sendto(). BTW, it may be useful to rename it to quic_lstnr_sock_send() to ensure it is never used by mistake as the default send function. When these changes are applied, send_retry() and friends should be called with the datagram source address systematically. The final check quic_lstnr_may_set_src() can then be used instead in quic_sock_sendto() (or quic_lstnr_sock_send() as my alternative naming). I personally find this approach clearer as it removes the extra quic_dgram_reply_src() helper (I prefer to avoid having to define yet another one line function whose purpose is not immediately clear). Also, the decision to reuse or not the source address is now centralize in quic_sock module which is a cleaner method. This proposed refactoring is optional, as it may be unclear or does not suit you. In this case we could apply your patches first and I'll probably try to refactor it later on top. Please let me know what you think about my comments, Thanks, -- Amaury Denoyelle
