+    #: 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.

Reply via email to