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

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

> +    #: Padding to add to the start of a line for python syntax compliance.
> +    _padding: ClassVar[str] = " " * 4

We use padding in the context of packets so let's use something else 
here, such as _python_indentation.

> +
>       def __init__(self, tg_node: Node, config: ScapyTrafficGeneratorConfig):
>           """Extend the constructor with Scapy TG specifics.
>   
> -        The traffic generator first starts an XML-RPC on the remote `tg_node`.
> -        Then it populates the server with functions which use the Scapy library
> -        to send/receive traffic:
> -
> -            * :func:`scapy_send_packets_and_capture`
> -            * :func:`scapy_send_packets`
> -
> -        To enable verbose logging from the xmlrpc client, use the :option:`--verbose`
> -        command line argument or the :envvar:`DTS_VERBOSE` environment variable.
> +        Initializes both a traffic generator and an interactive shell to handle Scapy functions.

Should these be with the definite article (instead of a and an)?

> +        The interactive shell will be started on `tg_node`.
>   
>           Args:
>               tg_node: The node where the traffic generator resides.
>               config: The traffic generator's test run configuration.
>           """
> -        super().__init__(tg_node, config)
> +        CapturingTrafficGenerator.__init__(self, tg_node, config)
> +        PythonShell.__init__(self, tg_node, privileged=True)
>   

This should probably mirror the superclass order from which the class is 
subclassed - PythonShell first, then CapturingTrafficGenerator.	

I'm also thinking of using super() here, but it's a bit convoluted. We'd 
need to do something like this:

class Common:
     def __init__(self, *args, **kwargs):
         super().__init__()

class TrafficGenerator(Common):
     def __init__(self, tg_node, config, **kwargs):
         super().__init__(tg_node, **kwargs)

class InteractiveShell(Common):
     def __init__(self, node, privileged = False, timeout = 15, 
start_on_init = True, params = "", **kwargs):
         super().__init__(node, **kwargs)

This works for both mro's. The common class basically makes the two 
subclasses eligible for multiple interitance while still working with 
normal inheritance (e.g. when making an instance of regular PythonShell) 
and the super() calls in the subclasses define what they have in common.

Since we only have this one use case, it may be overkill to use super() 
instead of calling both __init__()s directly. But with super(), we 
should be able to use any traffic generator with any interactive shell. 
I don't know whether we'll need that (we can use T-Rex without an 
interactive shell, but maybe using one is superior), but it could be useful.

>           assert (
>               self._tg_node.config.os == OS.linux
>           ), "Linux is the only supported OS for scapy traffic generation"
>   
> -        self.session = PythonShell(
> -            self._tg_node, timeout=5, privileged=True, name="ScapyXMLRPCServer"
> -        )
> +    def start_application(self) -> None:
> +        """Extends :meth:`framework.remote_session.interactive_shell.start_application`.
>   
> -        # import libs in remote python console
> -        for import_statement in SCAPY_RPC_SERVER_IMPORTS:
> -            self.session.send_command(import_statement)
> +        Adds a command that imports everything from the scapy library immediately after starting

This sound as if we were adding the command to some sort of internal 
list for processing later. We can probably just say "Import everything 
... in the remote shell.". I think it would be valuable to explicitly 
say where the import happens.

> +        the shell for usage in later calls to the methods of this class.
> +        """
> +        super().start_application()
> +        self.send_command("from scapy.all import *")
>   

It's possible we should check that the import was successful or do some 
other check that Scapy is indeed present and throw an exception if it isn't.

Actually, we probably should add a smoke test that would do that - some 
basic verification of traffic generators.