On 24. 9. 2024 18:34, Jeremy Spewock wrote:
On Tue, Sep 24, 2024 at 6:55 AM Juraj Linkeš <juraj.lin...@pantheon.tech> wrote:
I like how this looks. I have a number of minor comments (mainly wording
and naming), but overall it looks very good.
On 19. 9. 2024 21:02, jspew...@iol.unh.edu wrote:
From: Jeremy Spewock <jspew...@iol.unh.edu>
Previously all scapy commands were handled using an XML-RPC server that
ran on the TGNode. This unnecessarily enforces a minimum Python version
of 3.10 on the server that is being used as a traffic generator and
complicates the implementation of scapy methods.
What is the TG's minimum Python version now?
https://bugs.dpdk.org/show_bug.cgi?id=1388 says this will become a moot
point, but we're still using Python on the remote node.
Right, there is still some dependency there. I'm not sure the exact
versions, but doing some looking around I believe one of the newest
things scapy tools we use in the framework is the AsyncSniffer and the
scapy documentation [1] says that it has been available since version
2.4.3, and then when I looked at that version of the scapy package [2]
it looks like it claims to support python 2.7 and python 3.4-7. Poking
around in the documentation/code from scapy version 2.4.3 it also
looks like the syntax is very similar, so I believe it would work, but
I'm not sure I have any hosts that I could run on which have python
3.4. Maybe that does make the dependency essentially a moot point
considering these are fairly old python versions.
[1] https://github.com/secdev/scapy/blob/master/doc/scapy/usage.rst
[2] https://pypi.org/project/scapy/2.4.3/
Great. We should still document that the TG needs Python. I've updated
the ticket. [0]
[0] https://bugs.dpdk.org/show_bug.cgi?id=1388
+ def _create_packet_list(self, packets: list[Packet]) -> None:
Maybe we could apply some of the ideas from the local/remote naming
scheme I talked about in the tg devbind script patch here. Whatever
happens on the TG could be prefixed with remote and whatever is
happening locally would be without the prefix (or maybe whatever is
happnening on the TG shouldn't be prefixed (or a different prefix -
shell)? Makes sense, but then we'd need a good prefix for what's
happening on the execution environment. Maybe this also needs to be in a
different patch, after it's been though through with everything else in
I'll still write something here that makes the distinction and that
other patch could reformat if the author thought something else was
better.
Great.
+ sniffer_commands = [
+ f"{self._sniffer_name} = AsyncSniffer(",
+ f"iface='{recv_port.logical_name}',",
+ "store=True,",
+ "started_callback=lambda *args: sendp(",
As far as I can tell, we're not using the args, so we can just use
"lambda: sendp()"
We don't use the argument, but there are positional arguments passed
into this function by scapy which is why we have to catch and ignore
them.
Makes sense now, thanks for the explanation. Maybe we could put
somewhere in here a little comment pointing this out?
+ (
+ f"{self._python_indentation}{self._send_packet_list_name},"
Is the indentation needed here?
It's not required, but I think it makes the logs more readable.
That's a worthy use, let's keep it. Maybe also add an explanatory
comment here?
