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
If push comes to shove, one
can always add an extra field somewhere, or an extra arg in get_addr_len,
even if it'd be a little ugly.
Actually you're totally right and you make me realize that there's an
addrcmp field in the address families, and it's inconsistent to have
get_addr_len() being family-agnostic. So most likely we should first
change get_addr_len() to be per-family and appear in the proto_fam
struct. The few places where get_addr_len() is used should instead
rely on the protocol family for this. And due to this new address
having a variable length, we should support passing a pointer to the
address (like the current get_addr_len() does) in addition to the
protocol pointer.
I tried to do that yes, changing sock_addrlen to be a function pointer.
But it's actually less convenient, surprisingly, since then you end up
with constructs like:
listener->rx/tx->ss->proto->fam->get_addrlen(listener->rx/tx->ss)
(paraphrasing from memory; point is that it becomes somewhat unpleasant
to look at)
Still might end up having to do that given how tricky things become
otherwise...
Doing so would cleanly unlock all the problem. The new abns2 family
would just bring its own get_addr_len() variant that uses strlen().
Agreed on that OOP-ish approach being in principle a bit nicer though
(sorry for the swear word ;-) )
Tristan
