Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make TCP SetSocketOption template public #1373

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Make TCP SetSocketOption template public #1373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 0 additions & 25 deletions src/comm/Tcp.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

#include "squid.h"
#include "comm/Tcp.h"
#include "debug/Stream.h"

#if HAVE_NETINET_TCP_H
#include <netinet/tcp.h>
Expand All @@ -23,30 +22,6 @@
#endif
#include <type_traits>

/// setsockopt(2) wrapper
template <typename Option>
static bool
SetSocketOption(const int fd, const int level, const int optName, const Option &optValue)
{
static_assert(std::is_trivially_copyable<Option>::value, "setsockopt() expects POD-like options");
static_assert(!std::is_same<Option, bool>::value, "setsockopt() uses int to represent boolean options");
if (setsockopt(fd, level, optName, &optValue, sizeof(optValue)) < 0) {
const auto xerrno = errno;
debugs(5, DBG_IMPORTANT, "ERROR: setsockopt(2) failure: " << xstrerr(xerrno));
// TODO: Generalize to throw on errors when some callers need that.
return false;
}
return true;
}

/// setsockopt(2) wrapper for setting typical on/off options
static bool
SetBooleanSocketOption(const int fd, const int level, const int optName, const bool enable)
{
const int optValue = enable ? 1 :0;
return SetSocketOption(fd, level, optName, optValue);
}

void
Comm::ApplyTcpKeepAlive(int fd, const TcpKeepAlive &cfg)
{
Expand Down
26 changes: 26 additions & 0 deletions src/comm/Tcp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
#ifndef SQUID__SRC_COMM_TCP_H
#define SQUID__SRC_COMM_TCP_H

#include "debug/Stream.h"

namespace Comm
{

Expand All @@ -25,6 +27,30 @@ class TcpKeepAlive
/// apply configured TCP keep-alive settings to the given FD socket
void ApplyTcpKeepAlive(int fd, const TcpKeepAlive &);

/// setsockopt(2) wrapper
template <typename Option>
inline bool
SetSocketOption(const int fd, const int level, const int optName, const Option &optValue)
{
static_assert(std::is_trivially_copyable<Option>::value, "setsockopt() expects POD-like options");
static_assert(!std::is_same<Option, bool>::value, "setsockopt() uses int to represent boolean options");
if (setsockopt(fd, level, optName, reinterpret_cast<char *>(const_cast<Option *>(&optValue)), sizeof(optValue)) < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 similar1 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. ↩

const auto xerrno = errno;
debugs(5, DBG_IMPORTANT, "ERROR: setsockopt(2) failure: " << xstrerr(xerrno));
Copy link
Contributor

@rousskov rousskov Jun 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// TODO: Generalize to throw on errors when some callers need that.
return false;
}
return true;
}

/// setsockopt(2) wrapper for setting typical on/off options
inline bool
SetBooleanSocketOption(const int fd, const int level, const int optName, const bool enable)
{
const int optValue = enable ? 1 :0;
return SetSocketOption(fd, level, optName, optValue);
}

} // namespace Comm

#endif /* SQUID__SRC_COMM_TCP_H */