Hi Tristan, I'm back on this topic (I had not forgotten it).
On Sat, Mar 09, 2024 at 07:02:34PM +0000, Tristan wrote: > > > On 09/03/2024 18:09, Tristan wrote: > > Hi Willy, > > > > On 09/03/2024 16:51, Willy Tarreau wrote: > > > Hi Tristan, > > > > > > On Sat, Mar 09, 2024 at 04:20:21PM +0000, Tristan wrote: > > > > To be honest, I don't think this is unfixable. It's just a matter of how > > > > much code change we think is acceptable for it. > > > > > > I don't mind about the amount of changes. "we've always done it like > > > this" > > > is never a valid excuse to keep code as it is, especially when it's wrong > > > and causes difficulties to add new features. Of course I prefer invasive > > > changes early in the dev cycle than late but we're still OK and I don't > > > expect that many changes for this anyway. > > > > Agreed on not shying away from amount of changes. > > > > I tried to follow that route, being generous in what I was willing to > > touch, but ran into the issue of protocols and socket families being a > > 1:1 binding. > > > > So adding a new socket family *requires* adding a new protocol too, > > which isn't too hard but spirals a bit out of control: > > - protocol_by_family is a misnomer, and it is actually protocols *by > > socket domain* (which is still AF_UNIX, even with family AF_CUST_ABNS2, > > and ends up overriding standard socket/abns protocol) > > - but you can't just change that, because then other things stop working > > as some places rely on socket domain != socket family > > Actually, following up on this, I'm more and more convinced that it actually > makes things somewhat worse to take this approach... > > At least I don't see a good way to make it work without looking very bad, > because in a few places we lookup a protocol based on a socket_storage > reference, so we have truly only the socket domain to work with if no > connection is established yet. > > While it sounds like shying away from deeper changes, it seems to me that a > bind/server option is truly the right way to go here. I addressed these shortcomings of protocol lookups in a branch here: https://github.com/haproxy/haproxy/tree/20240809-abnsz-4 There was an API bug dating from 2.3 that was making everything more complicated than necessary, the code was assigning the custom family to sock_domain (the socket() argument) instead of sock_family (the one we're supposed to store into the address itself). I fixed that and added the minimal needed logic to properly perform the lookups and also resolve custom families into their real ones, because previously we had to perform random protocol lookups to figure them (we didn't notice since other custom families don't map to real ones). It even simplified a few places where some values were hard-coded in certain calls, or some logic enforced late (e.g. log.c). I now created abns as a custom proto, separate from uxst, so that it has its own family. It was not strictly necessary but it will allow us to get rid of plenty of \0 tests at many places by just having functions dedicated to unix or abns. Even the addrcmp() function should become simpler and I thought I'd specialize them. I've just added a last patch to create abnsz (the zero-terminated abns, since William rightfully suggested that the codename abns2 could make one think it's version 2), but for now it's a perfect copy of the other one, I have not modified the address length checks. I remember that you had a working PoC so I guess you've already spotted all locations and what to change, so restarting from your patch set will be much faster now. Please let me know if you want to give it a try, or if you think you won't have time to look into it and you prefer me to try to merge your work into this, it's as you prefer. I feel like we're finally seeing the light at the end of this long tunnel! I'm done for today and probably for the week-end as well I think. Have a nice week-end! Wily
