Re: [PATCH v3 1/1] dts: Remove XML-RPC server for Scapy TG and instead use PythonShell

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

On 24. 9. 2024 18:34, Jeremy Spewock wrote:
> On Tue, Sep 24, 2024 at 6:55 AM Juraj Linkeš <juraj.linkes@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, jspewock@iol.unh.edu wrote:
>>> From: Jeremy Spewock <jspewock@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?