Maintenance: use SetSocketOption() template

[yadij]

Commit db60fdb added
SetSocketOption() and SetBooleanSocketOption() to remove code
duplication around calls to setsockopt(2) but did not convert
most of the relevant syscalls.

Implement the waiting "TODO: Move SetSocketOption() and friends
for wider reuse.") and cleanup code around all relevant
setsockopt(2) calls. That cleanup has implicated addition of a
description parameter to the template to ease debugs() output,
some API and documentation changes to QoS setting functions, and
some polish of #ifdef precompiler directives.

[rousskov]

This is a partial review to facilitate progress. I will come back to this PR when it passes CI tests, the review concerns are addressed, and the backlog is empty.

[rousskov]

Proposed SetSocketOption() interface forces most callers to create SBuf objects for every call. Those objects are only used for debugging (rare errors). Creating such objects on so many SetSocketOption() calls is too expensive. Please refactor to eliminate these call costs.

Solution candidates include, without limitation:

1. Do not report additional details: Pros: Less work and caller noise. Cons: Callers cannot provide (secondary) error detail info. Callers cannot be distinguished.
2. Accept a c-string instead of SBuf. Pros: Detailing flexibility. Cons: Some callers cannot provide (secondary) error detail info. A lot of SetSocketOption(..., FOO, "FOO") code duplication. Some callers cannot be distinguished (without annoying or automated policing of description arguments for global uniqueness).
3. Automatically print (certain) setsockopt() option names and values: Pros: Simpler callers. Cons: More work to provide proper printing wrappers for common names and values. Errors cannot be detailed for a few callers (unless or until we provide proper printing wrappers for their more "exotic" parameters). Some callers cannot be distinguished.
4. Accept a SourceLocation argument instead of SBuf; callers supply Here(). Pros: Automatically pinpoints the exact caller. Cons: Grokking error details requires access to source code.

Personally, I would probably investigate how difficult it would be to cover most callers with "fully automated" option 3 and default to "keep as is" option 1 if option 3 requires too much work or cannot provide enough coverage (without heroic efforts).

The above options and their annotations are not meant to be exhaustive and precise. If you find at least some of them helpful, great! Otherwise, please do not waste time on their discussion -- use/document/defend your own decision logic instead.

[yadij]

If you think this is worth optimizing then IMO using a lambda to generate the SBuf as-needed for passing to debugs() would probably be best. Do you see any issues with that?

[rousskov]

If you think this is worth optimizing

I think that the identified performance regression in reviewed PR code is significant and must be addressed. I would not call that work "optimizing", but we do not need to agree on that terminology.

using a lambda to generate the SBuf as-needed for passing to debugs() would probably be best. Do you see any issues with that?

I have experimented with using lambdas to detail debugs() messages. We may eventually start doing that in certain contexts. However, lambdas are probably an overkill for the simple low-level callers in question, and writing them well (e.g., avoiding performance penalties and code duplication) will not be easy; other sketched solutions are probably better in this context.

[rousskov]

This wrapper is not specific to TCP sockets. There is already one call that uses SOL_SOCKET/SO_KEEPALIVE parameters that are, technically/by-themselves, not specific to TCP, and existing setsockopt() callers not yet converted in this PR are going to use it for non-TCP sockets (e.g., AF_UNIX sockets in src/ipc.cc). Please move these function declarations to src/comm/SockOpt.h or a similar¹ dedicated header instead of Tcp.h.

The requested move will also solve another problem: The PR currently forced all Tcp.h users to pull in debug/Stream.h that some of those users may not care about. We are forced to inline some of the implementation code, and we inline all of it for simplicity sake, including debugs() code that is not really appropriate for other header files. Placing these wrappers into a dedicated source file solves this problem nicely while keeping the implementation as simple as it is now.

After the move requested above, inline SetBooleanSocketOption(). There will be no need for the corresponding .cc file then. If you do not inline, then the guts of SetSocketOption() should not be inlined either -- the callers do not care about them and we only need specific Option type for the static asserts (that would still be inlined). Those guts should then be moved into a non-inlined SetSocketOption_() or SetSocketOptionImplementation() function. I recommend inlining everything (at least for now) because this code is simple and should not be needed in other header files.

I have already flagged this problem at #1373 (comment).

Footnotes

1. Eventually, these wrappers will probably be joined by their getsockopt() counterparts. Thus, we should not call the new header with a name specific to the currently covered "set" operation. ↩

[rousskov]

A hard-coded DBG_IMPORTANT level is not appropriate for some of the new SetSocketOption() callers (at least). For example, there are callers that use SetSocketOption() for environment feature probing -- they are expected to fail all the time in certain environments; their failure is not important (and official code does not report it today).

I see several solution candidates:

1. Move the debugging level into a SetSocketOption() wrapper parameter.
2. Replace DBG_IMPORTANT with 2 and adjust a few callers that really need DBG_IMPORTANT messages to add those messages. Some callers will probably lose their DBG_IMPORTANT messages because they are not really that important.
3. Throw instead, as suggested by the TODO comment below this debugs() line. That would solve all related problems nicely, but it requires more work in other places.

If you make debugging level a parameter, please make it a required one (despite a few existing caller changes):

• defaulting to DBG_IMPORANT would hide this somewhat important and surprising side effect from caller code readers
• defaulting to level 2 or higher may eventually result in callers that should report an important error but forget to do so (neither the copy-pasting developer nor a reviewer will have enough visual clues to spot the problem)
• defaulting to any level may make future cache_log_message control (if any) more harsh/controversial, defeating one of the the primary purposes of that directive -- avoiding costly arguments regarding the "best" message level.
• having no default parameter may allow future wrapper versions to throw on errors by default -- the explicit parameter will then be used to disable that new default throwing behavior

We can even move both the debugging section and the debugging level into wrapper parameters. In that case, in may be a good idea to make them the first two parameters. I am concerned that he API will be a bit verbose and callers may look a bit odd (because these wrappers are not a debugging function). Your call.

I have already discussed this problem at #1373 (comment)

[rousskov]

This PR (#2061) contains code and API decisions that have already been reviewed and flagged as problematic in PR #1373. The existence of a conflicting and reviewed PR has not been disclosed.

Please do not open multiple PRs doing very similar things without explicitly disclosing their relationship and addressing past review concerns.

Please close your PR #1373 if this (year-2025) PR supersedes that (year-2023) PR.