Make TCP SetSocketOption template public

[yadij]

No description provided.

[rousskov]

Thank you for generalizing our setsockopt() wrappers.

[rousskov]

This wrapper is not specific to TCP sockets. There is already one call that uses SOL_SOCKET/SO_KEEPALIVE parameters that are, technically, not specific to TCP, and, more importantly, some soon-to-be callers from #1359 are going to use it for non-TCP sockets (e.g., AF_UNIX). Please move these functions 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.

---

We can also declare these wrappers to be portability/compatiblity-related and move them from src/ into include/ or compat/. Doing so would address a dozen or so use cases outside of src/, but would probably require removing debugging, requiring existing caller changes. I do not insist on (or even recommend) moving these wrappers outside of src/; most of those "outside" callers should be removed or moved into src/ anyway... However, I would not object, in principle, to such a move. Your call.

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]

We can leave DBG_IMPORTANT hard-coded like this for now because all the existing callers use it, but some #1359 callers are not compatible with this debugging level. I recommend moving the debugging level into a wrapper parameter in this PR, so that #1359 can use these wrappers without waiting for another PR that solves the debug level problem and without mixing debugging level API changes with lots of "mechanical" caller changes (that #1359 is currently focusing on).

We can make debugging level the last parameter. If you do that, 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.

The last alternative is to actually throw, as suggested by the TODO comment below. That would solve all these problems nicely, but requires more work in other places.

In summary, I am OK with all of the options itemized below:

1. Do nothing in this PR; another PR will have to solve this problem.
2. Move the debugging level to the last required wrapper parameter.
3. Move both the debugging section and level to (first?) wrapper parameters.
4. Throw (instead of using debugs()) to report an error.