----- On Nov 13, 2019, at 11:38 AM, Florian Walbroel walbr...@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
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
