Re: [PATCH v1] dts: add time delay to async sniffer callback function

[Patrick Robb <probb@iol.unh.edu>]

[-- Attachment #1: Type: text/plain, Size: 2368 bytes --]

On Wed, Nov 6, 2024 at 3:17 PM Paul Szczepanek <paul.szczepanek@arm.com>
wrote:

>
> On 30/10/2024 17:08, Nicholas Pratte wrote:
> > There exists a bug within i40e NICs in which the async sniffer does not
> > catch send packets as a result of the callback function sending packets
> > too quickly before the NICs are ready to start capturing.
> >
> > There could be a multitude of reasons why this happens on these NICs, but
> > for the time being, inserting a one second delay in the callback function
> > will suffice.
>
> I can confirm the issue exists but we should explore a more definitive
> solution than adding a wait. Ideally instead of relying on the callback
> to send packets we should verify readines elsewhere in our sniffer and
> send packets when ready in our framework and not as part of the scapy
> sniffer constructor.
>

From looking at the documentation, it is the case that the standard way of
verifying readiness for the asyncsniffer is via the started_callback arg in
the asyncsniffer constructor. You can see some similar discussion here:
https://github.com/secdev/scapy/issues/3208

 So, if this is standard, it is probably best to remain within this
framework. I have been messing with this series tonight and although I
still can't tell why started_callback isn't calling on true sniffer
readiness, I think Nick's time.sleep calls are okay.

I will say, the modification of duration in this series is odd to me. It
looks like the _shell_start_and_stop_sniffing function arg has no default,
and no value is passed in in the call coming from send_packets_and_capture.
My preference would be to provide a default to the duration arg (say, 1)
and remove the arbitrary "duration + 1" in this series.

I also believe the comments about i40e should be removed. We understand
that this series is adding a delay to support sniffer readiness, but we
don't know why this behavior was originally seen on an i40e NIC, and
whether it's isolated to that driver.

Perhaps there would be a way to loop polling of scapy for sniffer
readiness, but I don't see how this would be better or different than the
asyncsniffer callback arg (which is essentially the same according to
docs). So, in my view the best thing to do is for me to fix up the commit
per my comments (and any others anyone has) and apply this.

[-- Attachment #2: Type: text/html, Size: 2945 bytes --]