Re: [PATCH lttng-modules] Add UDP and ICMP packet header information to the tracepoint:

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

----- On Nov 13, 2019, at 11:38 AM, Florian Walbroel walbroel@silexica.com wrote:

> * UDP transport header
> * ICMP transport header
> 
> (Correct indentation for switch/case)

Hi Florian,

Please address the additional comments below,

[...]
> 
> +static inline enum transport_header_types __get_transport_header_type_ip(struct
> sk_buff *skb)
> +{
> +	switch(ip_hdr(skb)->protocol) {

this should be:

  switch (ip_hdr(skb)->protocol) {

> +	case IPPROTO_TCP:
> +		return TH_TCP;
> +	case IPPROTO_UDP:
> +		return TH_UDP;
> +	case IPPROTO_ICMP:
> +		return TH_ICMP;
> +	}
> +	return TH_NONE;
> +}
> +
> +static inline enum transport_header_types
> __get_transport_header_type_ipv6(struct sk_buff *skb)
> +{
> +	switch(ipv6_hdr(skb)->nexthdr) {

And this:

  switch (ipv6_hdr(skb)->nexthdr) {

> +	case IPPROTO_TCP:
> +		return TH_TCP;
> +	case IPPROTO_UDP:
> +		return TH_UDP;
> +	case IPPROTO_ICMP:
> +		return TH_ICMP;
> +	}
> +	return TH_NONE;
> +}
> +

[...]


> 
> static const struct lttng_enum_desc transport_header_type = {
> @@ -510,15 +633,32 @@ LTTNG_TRACEPOINT_EVENT_CLASS(net_dev_template,
> 					ctf_integer_type(unsigned char, th_type)
> 
> 					/* Copy the transport header. */
> -					if (th_type == TH_TCP) {
> +					switch (th_type) {
> +					case TH_TCP: {
> 						ctf_align(uint32_t)
> 						ctf_array_type(uint8_t, tcp_hdr(skb),
> 								sizeof(struct tcphdr))
> +						break;
> +					}
> +					case TH_UDP: {
> +						ctf_align(uint32_t)
> +						ctf_array_type(uint8_t, udp_hdr(skb),
> +								sizeof(struct udphdr))
> +						break;
> +					}
> +					case TH_ICMP: {
> +						ctf_align(uint32_t)
> +						ctf_array_type(uint8_t, icmp_hdr(skb),
> +								sizeof(struct udphdr))
> +						break;

Shouldn't this be "sizeof(struct icmphdr)" ?

By looking at their definitions they appear to be the same size by chance,
but it's good to have the right type here.

After a last respin of the patch, we should be good to merge.

Thanks,

Mathieu


> +					}
> +					default:
> +						/*
> +						* For any other transport header type,
> +						* there is nothing to do.
> +						*/
> +						break;
> 					}
> -					/*
> -					 * For any other transport header type,
> -					 * there is nothing to do.
> -					 */
> 				}
> 			)
> 		)

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com