Re: [PATCH v2] tcp.c: leverage MSG_PEEK with offset kernel capability when available

Not a full review, but a couple of comments, mostly about stuff I also
had in pkt_selfie.c (review of v1):

On Thu, 18 Jan 2024 14:05:38 +1100
David Gibson <david(a)gibson.dropbear.id.au> wrote:

...
  On Sun, Jan 14, 2024 at 01:07:55PM -0500, Jon Maloy
wrote:
 
 [...]

 +
 +	s[0] = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
 +	s[1] = socket(AF_INET, SOCK_STREAM | SOCK_NONBLOCK, IPPROTO_TCP);
 +	if (s[0] < 0 || s[1] < 0) {
 +		perror("Temporary probe socket creation failed\n");
 +		goto out;
 +	}
 +	if (0 > bind(s[0], &a, sizeof(a))) {   
 
 Since the socket address is unspecified, why do you need to bind at
 all?  It might be clearer to explicitly set a to localhost + a
 specific port - because you're in a temporary namespace, you can rely
 on every port being available. 
There are two advantages of bind() without port, and then getsockname():
first, ip_unprivileged_port_start might have whatever value in our new
namespace (we don't touch it), and I wouldn't take for granted we'll
have CAP_SYS_ADMIN in it for all the possible start-up combinations.

Second, there's no need for a magic value.

...
 
 
+		perror("Temporary probe socket bind() failed\n");
 +		goto out;
 +	}
 +	if (0 > getsockname(s[0], &a, &((socklen_t) { sizeof(a) }))) {
 +		perror("Temporary probe socket getsockname() failed\n");
 +		goto out;
 +	}
 +	if (0 > listen(s[0], 0)) {
 +		perror("Temporary probe socket listen() failed\n");
 +		goto out;
 +	}
 +	if (0 <= connect(s[1], &a, sizeof(a)) || errno != EINPROGRESS) {
 +		perror("Temporary probe socket connect() failed\n");
 +		goto out;
 +	}   
 
 This is assuming that a will now contain the correct address to
 connect to.  Although it will have the right port, I think the address
 may still be unspecified for the listening socket. 
Hmm, why? From getsockname(2):

       getsockname()  returns  the  current address to which the socket
       sockfd is bound [...]

...
 
  [...]

 +/** tcp_probe_msg_peek_offset_cap() - Probe kernel for MSG_PEEK with offset support
 + */
 +static bool tcp_probe_msg_peek_offset_cap()   
 
 I believe we prefer the explicit foo(void) for declarations of
 functions with no parameters, rather than just foo(). 
Right, because foo() isn't a prototype, while foo(void) is. Perhaps at
some point we should enable -Wstrict-prototypes in CFLAGS (it's not in
-Wextra, I just realised).

Look:

$ cat prototypes.c
int a(void) { ; }
int b() { ; }
int main(char **argv) { a(); a(1); b(); b(1); }

$ gcc prototypes.c
prototypes.c: In function ‘main’:
prototypes.c:3:30: error: too many arguments to function ‘a’
    3 | int main(char **argv) { a(); a(1); b(); b(1); }
      |                              ^
prototypes.c:1:5: note: declared here
    1 | int a(void) { ; }
      |     ^

note that calling b() with any number and type of arguments is fine. And:

$ gcc -Wstrict-prototypes -Werror -Wfatal-errors prototypes.c
prototypes.c:2:5: error: function declaration isn’t a prototype
[-Werror=strict-prototypes]
    2 | int b() { ; }
      |     ^
compilation terminated due to -Wfatal-errors.
cc1: all warnings being treated as errors

...
  > [...] 
-- 
Stefano