G'Day Evgenii,



On Fri, Feb 8, 2019 at 10:29 AM Evgenii Seliavka <evg.selyavka@...> wrote:
>
> Hello,
>
> I slightly changed tools/tcpaccept.py  and enhance the script with the possibility to track only connection which very dropped because of listen queue overflow. Just want to know community opinion is this changes worth of the PR on upstream/master, because bcc repo already contains tcpdrop.py? Please find the patch in attachment. Tested this on kernel-4.14.74 and kernel-4.18.0.

As in CONTRIBUTING_SCRIPTS.md, we're after tools that do one thing and
do it well, so I'd generally avoid adding features that didn't belong
in tools. In this case, it might actually belong because you're
looking at what we could call "accept failures".

But I'm struggling to read your diff. Can you please regenerate with
-u, and also there's a lot of churn:

-        event.task, event.ip,
-        inet_ntop(AF_INET6, event.daddr).encode(),
-        inet_ntop(AF_INET6, event.saddr).encode(),
-        event.lport))
+
+    fields = [event.pid,
+              event.task,
+              event.ip,
+              inet_ntop(AF_INET6, event.daddr).encode(),
+              inet_ntop(AF_INET6, event.saddr).encode(),
+              event.lport,
+              ]

Changing many things at once makes it harder to understand the change
you want us to look at.

Brendan



>
>
> --
> Best Regards,
> Seliavka Evgenii

Hi Brendan,

Thank you for your response. Please find regenerated patch in attachment. Besides that i tried to minimize scope of the changes.

On Fri, Feb 8, 2019 at 11:19 AM Brendan Gregg <brendan.d.gregg@...> wrote:

G'Day Evgenii,



On Fri, Feb 8, 2019 at 10:29 AM Evgenii Seliavka <evg.selyavka@...> wrote:
>
> Hello,
>
> I slightly changed tools/tcpaccept.py  and enhance the script with the possibility to track only connection which very dropped because of listen queue overflow. Just want to know community opinion is this changes worth of the PR on upstream/master, because bcc repo already contains tcpdrop.py? Please find the patch in attachment. Tested this on kernel-4.14.74 and kernel-4.18.0.

As in CONTRIBUTING_SCRIPTS.md, we're after tools that do one thing and
do it well, so I'd generally avoid adding features that didn't belong
in tools. In this case, it might actually belong because you're
looking at what we could call "accept failures".

But I'm struggling to read your diff. Can you please regenerate with
-u, and also there's a lot of churn:

-        event.task, event.ip,
-        inet_ntop(AF_INET6, event.daddr).encode(),
-        inet_ntop(AF_INET6, event.saddr).encode(),
-        event.lport))
+
+    fields = [event.pid,
+              event.task,
+              event.ip,
+              inet_ntop(AF_INET6, event.daddr).encode(),
+              inet_ntop(AF_INET6, event.saddr).encode(),
+              event.lport,
+              ]

Changing many things at once makes it harder to understand the change
you want us to look at.

Brendan



>
>
> --
> Best Regards,
> Seliavka Evgenii

--

Best Regards,
Seliavka Evgenii