On Sun, 8 May 2022 at 09:57, Willy Tarreau <w...@1wt.eu> wrote:
>
> On Sun, May 01, 2022 at 03:33:17PM +0100, David CARLIER wrote:
> > Hi here a little patch to set idle time for SO_KEEPALIVE socket option.
>
> Now merged, thanks.
>
> David, one comment though, your commit messages keep missing a lot of
> crucial information for reviewers and debuggers, and I had to spend time
> documenting myself on keep-alive on MacOS to figure the differences and/or
> what the impacts or limitations of the patch could be. I finally found and
> put that information into the commit message, but it would be much easier
> if you could put it yourself after your development since you actually
> have access to the information I had to seek, and know the reasoning
> behind your choice.
>
Thanks, fair point :-) I ll take it in account even tough I was
certain for this one it would not break anything.
...
> The new choice was explained again. Finally, two weeks later I got a
> report of breakage when using external code linked at run time, the
> keyword registration didn't work anymore due to a mistake in this last
> patch. Fortunately, the design decision was explained and I could
> restart from this, figure my mistake and make sure that the 3rd attempt
> at a fix this time addressed all 3 identified use cases:
Do you have a reasonable numbers of macOs users or is it a rare occurrence ?
>
> commit 65d9f83794e00e136335348de531167f31d2f39b
> Author: Willy Tarreau <w...@1wt.eu>
> Date: Tue Apr 26 19:35:38 2022 +0200
>
> BUILD: compiler: properly distinguish weak and global symbols
>
> While weak symbols were finally fixed with commit fb1b6f5bc ("BUILD:
> compiler: use a more portable set of asm(".weak") statements"), it
> was an error to think that initcall symbols were also weak. They must
> not be and they're only global. The reason is that any externally
> linked code loaded as a .so would drop its weak symbols when being
> loaded, hence its initcalls that may contain various function
> registration calls.
>
> The ambiguity came from the fact that we initially reused the initcall's
> HA_GLOBL macro for OSX then generalized it, then turned it to a choice
> between .globl and .weak based on the OS, while in fact we needed a
> macro to define weak symbols.
>
> Let's rename the macro to HA_WEAK() to make it clear it's only for weak
> symbols, and redefine HA_GLOBL() that initcall needs.
>
> This will need to be backported wherever the commit above is backported
> (at least 2.5 for now).
>
I did not notice really those mem stats until now, good to know !
> For sure it's not perfect, and anything can always be improved. But
> that's not the point, the point here is to put info about the intent and
> the approach so that these ones can be preserved or corrected later. And
> that's valid for any patch, even a one-liner.
>
> The impact of your patch is that tcp-keep-alive will start to work on
> MacOS and if someone starts to rely on it and someone else later reverts
> your patch because it causes a build issue on a specific platform and the
> person thinks "oh we don't need this one there, we already have something
> else", by just reverting it they could break the other user's deployment.
>
> That's why it's important to put enough explanation to ensure the patch
> remains firmly stuck to the code base and cannot vanish without an extremly
> good argument.
>
Alrighty :-)
> Hoping this helps!
>
> Cheers
> Willy
