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

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

[Florian Walbroel]

[-- Attachment #1.1: Type: text/plain, Size: 6657 bytes --]

Hi,

I sent in this patch last month and did not hear about any updates since 
then.

There were a few formatting issues that have been addressed. Is there 
anything more that should be done?

Best regards,
Florian Walbroel

-------- Forwarded Message --------
Subject: 	[PATCH lttng-modules] Add UDP and ICMP packet header 
information to the tracepoint:
Date: 	Wed, 13 Nov 2019 17:38:14 +0100
From: 	Florian Walbroel <walbroel@silexica.com>
To: 	lttng-dev@lists.lttng.org



* UDP transport header
* ICMP transport header

(Correct indentation for switch/case)

Signed-off-by: Florian Walbroel <walbroel@silexica.com>
---
instrumentation/events/lttng-module/net.h | 166 ++++++++++++++++++++--
1 file changed, 153 insertions(+), 13 deletions(-)

diff --git a/instrumentation/events/lttng-module/net.h 
b/instrumentation/events/lttng-module/net.h
index bfa14fc..ad1892a 100644
--- a/instrumentation/events/lttng-module/net.h
+++ b/instrumentation/events/lttng-module/net.h
@@ -11,6 +11,8 @@
#include <linux/ip.h>
#include <linux/ipv6.h>
#include <linux/tcp.h>
+#include <linux/udp.h>
+#include <linux/icmp.h>
#include <linux/version.h>
#include <lttng-endian.h>
#include <net/sock.h>
@@ -85,6 +87,53 @@ static struct lttng_event_field tcpfields[] = {
},
};
+static struct lttng_event_field udpfields[] = {
+ [0] = {
+ .name = "source_port",
+ .type = __type_integer(uint16_t, 0, 0, 0,
+ __BIG_ENDIAN, 10, none),
+ },
+ [1] = {
+ .name = "dest_port",
+ .type = __type_integer(uint16_t, 0, 0, 0,
+ __BIG_ENDIAN, 10, none),
+ },
+ [2] = {
+ .name = "len",
+ .type = __type_integer(uint16_t, 0, 0, 0,
+ __BIG_ENDIAN, 10, none),
+ },
+ [3] = {
+ .name = "check",
+ .type = __type_integer(uint16_t, 0, 0, 0,
+ __BIG_ENDIAN, 10, none),
+ },
+};
+
+static struct lttng_event_field icmpfields[] = {
+ [0] = {
+ .name = "type",
+ .type = __type_integer(uint8_t, 0, 0, 0,
+ __BIG_ENDIAN, 10, none),
+ },
+ [1] = {
+ .name = "code",
+ .type = __type_integer(uint8_t, 0, 0, 0,
+ __BIG_ENDIAN, 10, none),
+ },
+ [2] = {
+ .name = "checksum",
+ .type = __type_integer(uint16_t, 0, 0, 0,
+ __BIG_ENDIAN, 10, none),
+ },
+ [3] = {
+ .name = "gateway",
+ .type = __type_integer(uint32_t, 0, 0, 0,
+ __BIG_ENDIAN, 10, none),
+ },
+};
+
+
static struct lttng_event_field transport_fields[] = {
[0] = {
.name = "unknown",
@@ -102,13 +151,57 @@ static struct lttng_event_field transport_fields[] = {
.u._struct.fields = tcpfields,
},
},
+ [2] = {
+ .name = "udp",
+ .type = {
+ .atype = atype_struct,
+ .u._struct.nr_fields = ARRAY_SIZE(udpfields),
+ .u._struct.fields = udpfields,
+ },
+ },
+ [3] = {
+ .name = "icmp",
+ .type = {
+ .atype = atype_struct,
+ .u._struct.nr_fields = ARRAY_SIZE(icmpfields),
+ .u._struct.fields = icmpfields,
+ },
+ },
};
enum transport_header_types {
TH_NONE = 0,
TH_TCP = 1,
+ TH_UDP = 2,
+ TH_ICMP = 3,
};
+static inline enum transport_header_types 
__get_transport_header_type_ip(struct sk_buff *skb)
+{
+ 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) {
+ 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(struct sk_buff *skb)
{
if (__has_network_hdr(skb)) {
@@ -123,13 +216,13 @@ static inline enum transport_header_types 
__get_transport_header_type(struct sk_
* header's data. This method works both for
* sent and received packets.
*/
- if ((skb->protocol == htons(ETH_P_IP) &&
- ip_hdr(skb)->protocol == IPPROTO_TCP) ||
- (skb->protocol == htons(ETH_P_IPV6) &&
- ipv6_hdr(skb)->nexthdr == IPPROTO_TCP))
- return TH_TCP;
+ if (skb->protocol == htons(ETH_P_IP)) {
+ return __get_transport_header_type_ip(skb);
+ } else if(skb->protocol == htons(ETH_P_IPV6)) {
+ return __get_transport_header_type_ipv6(skb);
+ }
}
- /* Fallthrough for other cases where header is not TCP. */
+ /* Fallthrough for other cases where header is not recognized. */
}
return TH_NONE;
}
@@ -137,16 +230,36 @@ static inline enum transport_header_types 
__get_transport_header_type(struct sk_
static struct lttng_enum_entry proto_transport_enum_entries[] = {
[0] = {
.start = { .value = 0, .signedness = 0, },
- .end = { .value = IPPROTO_TCP - 1, .signedness = 0, },
+ .end = { .value = IPPROTO_ICMP - 1, .signedness = 0, },
.string = "_unknown",
},
[1] = {
+ .start = { .value = IPPROTO_ICMP, .signedness = 0, },
+ .end = { .value = IPPROTO_ICMP, .signedness = 0, },
+ .string = "_icmp",
+ },
+ [2] = {
+ .start = { .value = IPPROTO_ICMP + 1, .signedness = 0, },
+ .end = { .value = IPPROTO_TCP - 1, .signedness = 0, },
+ .string = "_unknown",
+ },
+ [3] = {
.start = { .value = IPPROTO_TCP, .signedness = 0, },
.end = { .value = IPPROTO_TCP, .signedness = 0, },
.string = "_tcp",
},
- [2] = {
+ [4] = {
.start = { .value = IPPROTO_TCP + 1, .signedness = 0, },
+ .end = { .value = IPPROTO_UDP - 1, .signedness = 0, },
+ .string = "_unknown",
+ },
+ [5] = {
+ .start = { .value = IPPROTO_UDP, .signedness = 0, },
+ .end = { .value = IPPROTO_UDP, .signedness = 0, },
+ .string = "_udp",
+ },
+ [6] = {
+ .start = { .value = IPPROTO_UDP + 1, .signedness = 0, },
.end = { .value = 255, .signedness = 0, },
.string = "_unknown",
},
@@ -169,6 +282,16 @@ static struct lttng_enum_entry 
transport_enum_entries[] = {
.end = { .value = TH_TCP, .signedness = 0, },
.string = "_tcp",
},
+ [2] = {
+ .start = { .value = TH_UDP, .signedness = 0, },
+ .end = { .value = TH_UDP, .signedness = 0, },
+ .string = "_udp",
+ },
+ [3] = {
+ .start = { .value = TH_ICMP, .signedness = 0, },
+ .end = { .value = TH_ICMP, .signedness = 0, },
+ .string = "_icmp",
+ },
};
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;
+ }
+ default:
+ /*
+ * For any other transport header type,
+ * there is nothing to do.
+ */
+ break;
}
- /*
- * For any other transport header type,
- * there is nothing to do.
- */
}
)
)

-- 
2.17.1


[-- Attachment #1.2: Type: text/html, Size: 10585 bytes --]

[-- Attachment #2: Type: text/plain, Size: 156 bytes --]

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[Mathieu Desnoyers]

[-- Attachment #1.1: Type: text/plain, Size: 8074 bytes --]

----- On Dec 17, 2019, at 3:56 AM, Florian Walbroel <walbroel@silexica.com> wrote: 

> Hi,

> I sent in this patch last month and did not hear about any updates since then.

> There were a few formatting issues that have been addressed. Is there anything
> more that should be done?
Hi, 

I was expecting review from Geneviève and Julien on this patch, but never heard back from 
them. 

Geneviève, Julien, Michael, can you review this patch please ? 

Thanks, 

Mathieu 

> Best regards,
> Florian Walbroel

> -------- Forwarded Message -------- Subject: 	[PATCH lttng-modules] Add UDP and
> ICMP packet header information to the tracepoint:
> Date: 	Wed, 13 Nov 2019 17:38:14 +0100
> From: 	Florian Walbroel [ mailto:walbroel@silexica.com | <walbroel@silexica.com>
> ]

> To: 	[ mailto:lttng-dev@lists.lttng.org | lttng-dev@lists.lttng.org ]

> * UDP transport header
> * ICMP transport header

> (Correct indentation for switch/case)

> Signed-off-by: Florian Walbroel [ mailto:walbroel@silexica.com |
> <walbroel@silexica.com> ]
> ---
> instrumentation/events/lttng-module/net.h | 166 ++++++++++++++++++++--
> 1 file changed, 153 insertions(+), 13 deletions(-)

> diff --git a/instrumentation/events/lttng-module/net.h
> b/instrumentation/events/lttng-module/net.h
> index bfa14fc..ad1892a 100644
> --- a/instrumentation/events/lttng-module/net.h
> +++ b/instrumentation/events/lttng-module/net.h
> @@ -11,6 +11,8 @@
> #include <linux/ip.h>
> #include <linux/ipv6.h>
> #include <linux/tcp.h>
> +#include <linux/udp.h>
> +#include <linux/icmp.h>
> #include <linux/version.h>
> #include <lttng-endian.h>
> #include <net/sock.h>
> @@ -85,6 +87,53 @@ static struct lttng_event_field tcpfields[] = {
> },
> };
> +static struct lttng_event_field udpfields[] = {
> + [0] = {
> + .name = "source_port",
> + .type = __type_integer(uint16_t, 0, 0, 0,
> + __BIG_ENDIAN, 10, none),
> + },
> + [1] = {
> + .name = "dest_port",
> + .type = __type_integer(uint16_t, 0, 0, 0,
> + __BIG_ENDIAN, 10, none),
> + },
> + [2] = {
> + .name = "len",
> + .type = __type_integer(uint16_t, 0, 0, 0,
> + __BIG_ENDIAN, 10, none),
> + },
> + [3] = {
> + .name = "check",
> + .type = __type_integer(uint16_t, 0, 0, 0,
> + __BIG_ENDIAN, 10, none),
> + },
> +};
> +
> +static struct lttng_event_field icmpfields[] = {
> + [0] = {
> + .name = "type",
> + .type = __type_integer(uint8_t, 0, 0, 0,
> + __BIG_ENDIAN, 10, none),
> + },
> + [1] = {
> + .name = "code",
> + .type = __type_integer(uint8_t, 0, 0, 0,
> + __BIG_ENDIAN, 10, none),
> + },
> + [2] = {
> + .name = "checksum",
> + .type = __type_integer(uint16_t, 0, 0, 0,
> + __BIG_ENDIAN, 10, none),
> + },
> + [3] = {
> + .name = "gateway",
> + .type = __type_integer(uint32_t, 0, 0, 0,
> + __BIG_ENDIAN, 10, none),
> + },
> +};
> +
> +
> static struct lttng_event_field transport_fields[] = {
> [0] = {
> .name = "unknown",
> @@ -102,13 +151,57 @@ static struct lttng_event_field transport_fields[] = {
> .u._struct.fields = tcpfields,
> },
> },
> + [2] = {
> + .name = "udp",
> + .type = {
> + .atype = atype_struct,
> + .u._struct.nr_fields = ARRAY_SIZE(udpfields),
> + .u._struct.fields = udpfields,
> + },
> + },
> + [3] = {
> + .name = "icmp",
> + .type = {
> + .atype = atype_struct,
> + .u._struct.nr_fields = ARRAY_SIZE(icmpfields),
> + .u._struct.fields = icmpfields,
> + },
> + },
> };
> enum transport_header_types {
> TH_NONE = 0,
> TH_TCP = 1,
> + TH_UDP = 2,
> + TH_ICMP = 3,
> };
> +static inline enum transport_header_types __get_transport_header_type_ip(struct
> sk_buff *skb)
> +{
> + 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) {
> + 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(struct
> sk_buff *skb)
> {
> if (__has_network_hdr(skb)) {
> @@ -123,13 +216,13 @@ static inline enum transport_header_types
> __get_transport_header_type(struct sk_
> * header's data. This method works both for
> * sent and received packets.
> */
> - if ((skb->protocol == htons(ETH_P_IP) &&
> - ip_hdr(skb)->protocol == IPPROTO_TCP) ||
> - (skb->protocol == htons(ETH_P_IPV6) &&
> - ipv6_hdr(skb)->nexthdr == IPPROTO_TCP))
> - return TH_TCP;
> + if (skb->protocol == htons(ETH_P_IP)) {
> + return __get_transport_header_type_ip(skb);
> + } else if(skb->protocol == htons(ETH_P_IPV6)) {
> + return __get_transport_header_type_ipv6(skb);
> + }
> }
> - /* Fallthrough for other cases where header is not TCP. */
> + /* Fallthrough for other cases where header is not recognized. */
> }
> return TH_NONE;
> }
> @@ -137,16 +230,36 @@ static inline enum transport_header_types
> __get_transport_header_type(struct sk_
> static struct lttng_enum_entry proto_transport_enum_entries[] = {
> [0] = {
> .start = { .value = 0, .signedness = 0, },
> - .end = { .value = IPPROTO_TCP - 1, .signedness = 0, },
> + .end = { .value = IPPROTO_ICMP - 1, .signedness = 0, },
> .string = "_unknown",
> },
> [1] = {
> + .start = { .value = IPPROTO_ICMP, .signedness = 0, },
> + .end = { .value = IPPROTO_ICMP, .signedness = 0, },
> + .string = "_icmp",
> + },
> + [2] = {
> + .start = { .value = IPPROTO_ICMP + 1, .signedness = 0, },
> + .end = { .value = IPPROTO_TCP - 1, .signedness = 0, },
> + .string = "_unknown",
> + },
> + [3] = {
> .start = { .value = IPPROTO_TCP, .signedness = 0, },
> .end = { .value = IPPROTO_TCP, .signedness = 0, },
> .string = "_tcp",
> },
> - [2] = {
> + [4] = {
> .start = { .value = IPPROTO_TCP + 1, .signedness = 0, },
> + .end = { .value = IPPROTO_UDP - 1, .signedness = 0, },
> + .string = "_unknown",
> + },
> + [5] = {
> + .start = { .value = IPPROTO_UDP, .signedness = 0, },
> + .end = { .value = IPPROTO_UDP, .signedness = 0, },
> + .string = "_udp",
> + },
> + [6] = {
> + .start = { .value = IPPROTO_UDP + 1, .signedness = 0, },
> .end = { .value = 255, .signedness = 0, },
> .string = "_unknown",
> },
> @@ -169,6 +282,16 @@ static struct lttng_enum_entry transport_enum_entries[] = {
> .end = { .value = TH_TCP, .signedness = 0, },
> .string = "_tcp",
> },
> + [2] = {
> + .start = { .value = TH_UDP, .signedness = 0, },
> + .end = { .value = TH_UDP, .signedness = 0, },
> + .string = "_udp",
> + },
> + [3] = {
> + .start = { .value = TH_ICMP, .signedness = 0, },
> + .end = { .value = TH_ICMP, .signedness = 0, },
> + .string = "_icmp",
> + },
> };
> 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;
> + }
> + default:
> + /*
> + * For any other transport header type,
> + * there is nothing to do.
> + */
> + break;
> }
> - /*
> - * For any other transport header type,
> - * there is nothing to do.
> - */
> }
> )
> )
> --
> 2.17.1

> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[-- Attachment #1.2: Type: text/html, Size: 12139 bytes --]

[-- Attachment #2: Type: text/plain, Size: 156 bytes --]

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[Michael Jeanson]

On 2019-12-17 9:39 a.m., Mathieu Desnoyers wrote:
> I was expecting review from Geneviève and Julien on this patch, but
> never heard back from
> them.
> 
> Geneviève, Julien, Michael, can you review this patch please ?
> 
> Thanks,
> 
> Mathieu

It builds on all the latest tags of the stable kernel branches we
support and the patch itself looks good to me but Geneviève's or
Julien's feedback would be appreciated.

Michael
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[Florian Walbroel]

Hi,

is there any update regarding the patches?

BR,

Florian

On 17.12.19 18:20, Michael Jeanson wrote:
> On 2019-12-17 9:39 a.m., Mathieu Desnoyers wrote:
>> I was expecting review from Geneviève and Julien on this patch, but
>> never heard back from
>> them.
>>
>> Geneviève, Julien, Michael, can you review this patch please ?
>>
>> Thanks,
>>
>> Mathieu
> It builds on all the latest tags of the stable kernel branches we
> support and the patch itself looks good to me but Geneviève's or
> Julien's feedback would be appreciated.
>
> Michael
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[Mathieu Desnoyers]

----- 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