Re: [PATCH v2 3/4] dtrace: add tcp provider

[Kris Van Hees <kris.van.hees@oracle.com>]

A few other observations:

On Tue, Jun 10, 2025 at 02:58:12PM +0100, Alan Maguire wrote:
<<skip>>
> diff --git a/libdtrace/tcp.d b/libdtrace/tcp.d
> index 54e310cb..d4beea87 100644
> --- a/libdtrace/tcp.d
> +++ b/libdtrace/tcp.d
> @@ -8,7 +8,6 @@
>  #pragma D depends_on module vmlinux
>  #pragma D depends_on library net.d
>  #pragma D depends_on provider ip
> -#pragma D depends_on provider tcp

Definitely should be put back (already mentioned earlier and replied to, so
will be fixed).

>  inline int TH_FIN =	0x01;
>  inline int TH_SYN =	0x02;
> @@ -60,7 +59,7 @@ typedef struct tcpinfo {
>  	uint32_t tcp_seq;		/* sequence number */
>  	uint32_t tcp_ack;		/* acknowledgment number */
>  	uint8_t tcp_offset;		/* data offset, in bytes */
> -	uint8_t tcp_flags;		/* flags */
> +	uint16_t tcp_flags;		/* flags */

Why is the type of tcp_flags being changed?  See the comment below where it
is being assigned.

>  	uint16_t tcp_window;		/* window size */
>  	uint16_t tcp_checksum;		/* checksum */
>  	uint16_t tcp_urgent;		/* urgent data pointer */
> @@ -111,13 +110,16 @@ translator tcpinfo_t < struct tcphdr *T > {
>  	tcp_seq = T ? ntohl(T->seq) : 0;
>  	tcp_ack = T ? ntohl(T->ack_seq) : 0;
>  	tcp_offset = T ? (*(uint8_t *)(T + 12) & 0xf0) >> 2 : 0;
> -	tcp_flags = T ? *(uint8_t *)(T + 13) : 0;
> +	tcp_flags = T ? *((uint8_t *)T + 13) : 0;

This clearly shows that it is meant to be uint8_t anyway?

>  	tcp_window = T ? ntohs(T->window) : 0;
>  	tcp_checksum = T ? ntohs(T->check) : 0;
>  	tcp_urgent = T ? ntohs(T->urg_ptr) : 0;
>  	tcp_hdr = (uintptr_t)T;
>  };
>  
> +inline int tcp_fullsock[struct tcp_sock *sk] =
> +	(((struct sock_common *)sk)->skc_state != TCP_STATE_SYN_RECEIVED &&
> +	 ((struct sock_common *)sk)->skc_state != TCP_STATE_TIME_WAIT);

This could do with a comment I think to explain what it is and how it will be
used?

>  /*
>   * In the main we simply translate from the "struct [tcp_]sock *" to
>   * a tcpsinfo_t *.  However there are a few exceptions:
> @@ -158,47 +160,45 @@ translator tcpsinfo_t < struct tcp_sock *T > {
>              ((uint32_t *)&((struct sock *)T)->__sk_common.skc_v6_daddr)[2] &&
>  	    ((uint32_t *)&((struct sock *)T)->__sk_common.skc_v6_rcv_saddr)[3])
>  	    : 0;
> -	tcps_lport = (T && ((struct inet_sock *)T)->inet_sport != 0) ?
> +	tcps_lport = T && ((struct inet_sock *)T)->inet_sport != 0 &&
> +	    tcp_fullsock[T] ?
>  	    ntohs(((struct inet_sock *)T)->inet_sport) :
>  	    (T && ((struct inet_sock *)T)->inet_sport == 0) ?
> -	    ntohs(((struct sock *)T)->__sk_common.skc_num) :
> +	    ((struct sock *)T)->__sk_common.skc_num :
>  	    arg4 != NULL ?
>  	    ntohs(arg7 == NET_PROBE_INBOUND ?
> -	    ((struct tcphdr *)arg4)->dest : ((struct tcphdr *)arg4)->source) :
> +		  ((struct tcphdr *)arg4)->dest :
> +		  ((struct tcphdr *)arg4)->source) :
>  	    0;
>  	tcps_rport = T && ((struct sock *)T)->__sk_common.skc_dport != 0 ?
>  	    ntohs(((struct sock *)T)->__sk_common.skc_dport) :
>  	    arg4 != NULL ?
>  	    ntohs(arg7 == NET_PROBE_INBOUND ?
> -            ((struct tcphdr *)arg4)->source : ((struct tcphdr *)arg4)->dest) :
> +		  ((struct tcphdr *)arg4)->source :
> +		  ((struct tcphdr *)arg4)->dest) :
>  	    0;
>  	tcps_laddr =
>  	    T && ((struct sock *)T)->__sk_common.skc_family == AF_INET ?
>  	    inet_ntoa(&((struct sock *)T)->__sk_common.skc_rcv_saddr) :
>  	    T && ((struct sock *)T)->__sk_common.skc_family == AF_INET6 ?
>  	    inet_ntoa6(&((struct sock *)T)->__sk_common.skc_v6_rcv_saddr) :
> -	    arg2 != NULL && (*(uint8_t *)arg2) >> 4 == 4 ?
> -	    inet_ntoa(arg7 == NET_PROBE_INBOUND ?
> -	    &((struct iphdr *)arg2)->daddr : &((struct iphdr *)arg2)->saddr) :
> -	    arg2 != NULL && *((uint8_t *)arg2) >> 4 == 6 ?
> -	    inet_ntoa6(arg7 == NET_PROBE_INBOUND ?
> -	    &((struct ipv6hdr *)arg2)->daddr :
> -	    &((struct ipv6hdr *)arg2)->saddr) :
> +	    arg2 != NULL && (*(uint8_t *)arg2 >> 4) == 4 ?
> +	    inet_ntoa(&((struct iphdr *)arg2)->daddr) :
> +	    arg2 != NULL && (*(uint8_t *)arg2 >> 4) == 6 ?
> +	    inet_ntoa6(&((struct ipv6hdr *)arg2)->daddr) :
>  	    "<unknown>";
>  	tcps_raddr =
>  	    T && ((struct sock *)T)->__sk_common.skc_family == AF_INET ?
>  	    inet_ntoa(&((struct sock *)T)->__sk_common.skc_daddr) :
>  	    T && ((struct sock *)T)->__sk_common.skc_family == AF_INET6 ?
>  	    inet_ntoa6(&((struct sock *)T)->__sk_common.skc_v6_daddr) :
> -	    arg2 != NULL && (*(uint8_t *)arg2) >> 4 == 4 ?
> -	    inet_ntoa(arg7 == NET_PROBE_INBOUND ?
> -	    &((struct iphdr *)arg2)->saddr : &((struct iphdr *)arg2)->daddr) :
> -	    arg2 != NULL && *((uint8_t *)arg2) >> 4 == 6 ?
> -	    inet_ntoa6(arg7 == NET_PROBE_INBOUND ?
> -	    &((struct ipv6hdr *)arg2)->saddr :
> -	    &((struct ipv6hdr *)arg2)->daddr) :
> -	    "<unknown>";
> -	tcps_state = arg6;
> +	    arg2 != NULL && (*(uint8_t *)arg2 >> 4) == 4 ?
> +	    inet_ntoa(&((struct iphdr *)arg2)->saddr) :
> +	    arg2 != NULL && (*(uint8_t *)arg2 >> 4) == 6 ?
> +	    inet_ntoa6(&((struct ipv6hdr *)arg2)->saddr) :
> +	    "<unknown";
> +	tcps_state = arg7 == NET_PROBE_STATE ? arg6 :
> +	    T ? ((struct sock *)T)->__sk_common.skc_state : 0;
>  	tcps_iss = T ?
>  	    T->snd_una - (uint32_t)T->bytes_acked : 0;
>  	tcps_suna = T ? T->snd_una : 0;
> @@ -229,3 +229,9 @@ translator tcpsinfo_t < struct tcp_sock *T > {
>  translator tcplsinfo_t < int I > {
>  	tcps_state = I;
>  };
> +
> +/* For tracepoint, the last state is in the sock state, next passed as arg6 */
> +#pragma D binding "1.6.3" translator
> +translator tcplsinfo_t < struct sock *S > {
> +	tcps_state = S ? S->__sk_common.skc_state : 0;
> +};
> -- 
> 2.39.3
>