Re: [PATCH v8 20/21] dts: scapy tg docstring update

[Jeremy Spewock <jspewock@iol.unh.edu>]

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

On Thu, Nov 23, 2023 at 10:14 AM Juraj Linkeš <juraj.linkes@pantheon.tech>
wrote:

> Format according to the Google format and PEP257, with slight
> deviations.
>
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> ---
>  .../testbed_model/traffic_generator/scapy.py  | 91 +++++++++++--------
>  1 file changed, 54 insertions(+), 37 deletions(-)
>
> diff --git a/dts/framework/testbed_model/traffic_generator/scapy.py
> b/dts/framework/testbed_model/traffic_generator/scapy.py
> index c88cf28369..30ea3914ee 100644
> --- a/dts/framework/testbed_model/traffic_generator/scapy.py
> +++ b/dts/framework/testbed_model/traffic_generator/scapy.py
> @@ -2,14 +2,15 @@
>  # Copyright(c) 2022 University of New Hampshire
>  # Copyright(c) 2023 PANTHEON.tech s.r.o.
>
> -"""Scapy traffic generator.
> +"""The Scapy traffic generator.
>
> -Traffic generator used for functional testing, implemented using the
> Scapy library.
> +A traffic generator used for functional testing, implemented with
> +`the Scapy library <https://scapy.readthedocs.io/en/latest/>`_.
>  The traffic generator uses an XML-RPC server to run Scapy on the remote
> TG node.
>
> -The XML-RPC server runs in an interactive remote SSH session running
> Python console,
> -where we start the server. The communication with the server is
> facilitated with
> -a local server proxy.
> +The traffic generator uses the :mod:`xmlrpc.server` module to run an
> XML-RPC server
> +in an interactive remote Python SSH session. The communication with the
> server is facilitated
> +with a local server proxy from the :mod:`xmlrpc.client` module.
>  """
>
>  import inspect
> @@ -69,20 +70,20 @@ def scapy_send_packets_and_capture(
>      recv_iface: str,
>      duration: float,
>  ) -> list[bytes]:
> -    """RPC function to send and capture packets.
> +    """The RPC function to send and capture packets.
>
> -    The function is meant to be executed on the remote TG node.
> +    The function is meant to be executed on the remote TG node via the
> server proxy.
>
>
Should this maybe be "This function is meant" instead? I'm not completely
sure if it should be, I feel like it might be able to go either way.


>      Args:
>          xmlrpc_packets: The packets to send. These need to be converted to
> -            xmlrpc.client.Binary before sending to the remote server.
> +            :class:`~xmlrpc.client.Binary` objects before sending to the
> remote server.
>          send_iface: The logical name of the egress interface.
>          recv_iface: The logical name of the ingress interface.
>          duration: Capture for this amount of time, in seconds.
>
>      Returns:
>          A list of bytes. Each item in the list represents one packet,
> which needs
> -            to be converted back upon transfer from the remote node.
> +        to be converted back upon transfer from the remote node.
>      """
>      scapy_packets = [scapy.all.Packet(packet.data) for packet in
> xmlrpc_packets]
>      sniffer = scapy.all.AsyncSniffer(
> @@ -96,19 +97,15 @@ def scapy_send_packets_and_capture(
>
>
>  def scapy_send_packets(xmlrpc_packets: list[xmlrpc.client.Binary],
> send_iface: str) -> None:
> -    """RPC function to send packets.
> +    """The RPC function to send packets.
>
> -    The function is meant to be executed on the remote TG node.
> -    It doesn't return anything, only sends packets.
> +    The function is meant to be executed on the remote TG node via the
> server proxy.
>

Same thing here. I don't think it matters that much since you refer to it
as being "the RPC function" for sending packets, but it feels like you are
referring instead to this specific function on this line.


> +    It only sends `xmlrpc_packets`, without capturing them.
>
>      Args:
>          xmlrpc_packets: The packets to send. These need to be converted to
> -            xmlrpc.client.Binary before sending to the remote server.
> +            :class:`~xmlrpc.client.Binary` objects before sending to the
> remote server.
>          send_iface: The logical name of the egress interface.
> -
> -    Returns:
> -        A list of bytes. Each item in the list represents one packet,
> which needs
> -            to be converted back upon transfer from the remote node.
>      """
>      scapy_packets = [scapy.all.Packet(packet.data) for packet in
> xmlrpc_packets]
>      scapy.all.sendp(scapy_packets, iface=send_iface, realtime=True,
> verbose=True)
> @@ -128,11 +125,19 @@ def scapy_send_packets(xmlrpc_packets:
> list[xmlrpc.client.Binary], send_iface: s
>
>
>  class QuittableXMLRPCServer(SimpleXMLRPCServer):
> -    """Basic XML-RPC server that may be extended
> -    by functions serializable by the marshal module.
> +    """Basic XML-RPC server.
> +
> +    The server may be augmented by functions serializable by the
> :mod:`marshal` module.
>      """
>
>      def __init__(self, *args, **kwargs):
> +        """Extend the XML-RPC server initialization.
> +
> +        Args:
> +            args: The positional arguments that will be passed to the
> superclass's constructor.
> +            kwargs: The keyword arguments that will be passed to the
> superclass's constructor.
> +                The `allow_none` argument will be set to :data:`True`.
> +        """
>          kwargs["allow_none"] = True
>          super().__init__(*args, **kwargs)
>          self.register_introspection_functions()
> @@ -140,13 +145,12 @@ def __init__(self, *args, **kwargs):
>          self.register_function(self.add_rpc_function)
>
>      def quit(self) -> None:
> +        """Quit the server."""
>          self._BaseServer__shutdown_request = True
>          return None
>
>      def add_rpc_function(self, name: str, function_bytes:
> xmlrpc.client.Binary) -> None:
> -        """Add a function to the server.
> -
> -        This is meant to be executed remotely.
> +        """Add a function to the server from the local server proxy.
>
>          Args:
>                name: The name of the function.
> @@ -157,6 +161,11 @@ def add_rpc_function(self, name: str, function_bytes:
> xmlrpc.client.Binary) -> N
>          self.register_function(function)
>
>      def serve_forever(self, poll_interval: float = 0.5) -> None:
> +        """Extend the superclass method with an additional print.
> +
> +        Once executed in the local server proxy, the print gives us a
> clear string to expect
> +        when starting the server. The print means the function was
> executed on the XML-RPC server.
> +        """
>          print("XMLRPC OK")
>          super().serve_forever(poll_interval)
>
> @@ -164,19 +173,12 @@ def serve_forever(self, poll_interval: float = 0.5)
> -> None:
>  class ScapyTrafficGenerator(CapturingTrafficGenerator):
>      """Provides access to scapy functions via an RPC interface.
>
> -    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.
> -
> -    Any packets sent to the remote server are first converted to bytes.
> -    They are received as xmlrpc.client.Binary objects on the server side.
> -    When the server sends the packets back, they are also received as
> -    xmlrpc.client.Binary object on the client side, are converted back to
> Scapy
> -    packets and only then returned from the methods.
> +    The class extends the base with remote execution of scapy functions.
>

Same thing here if the above end up getting changed.


>
> -    Arguments:
> -        tg_node: The node where the traffic generator resides.
> -        config: The user configuration of the traffic generator.
> +    Any packets sent to the remote server are first converted to bytes.
> They are received as
> +    :class:`~xmlrpc.client.Binary` objects on the server side. When the
> server sends the packets
> +    back, they are also received as :class:`~xmlrpc.client.Binary`
> objects on the client side, are
> +    converted back to :class:`~scapy.packet.Packet` objects and only then
> returned from the methods.
>
>      Attributes:
>          session: The exclusive interactive remote session created by the
> Scapy
> @@ -190,6 +192,22 @@ class
> ScapyTrafficGenerator(CapturingTrafficGenerator):
>      _config: ScapyTrafficGeneratorConfig
>
>      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.
> +
> +        Args:
> +            tg_node: The node where the traffic generator resides.
> +            config: The traffic generator's test run configuration.
> +        """
>          super().__init__(tg_node, config)
>
>          assert (
> @@ -231,10 +249,8 @@ def _start_xmlrpc_server_in_remote_python(self,
> listen_port: int) -> None:
>          # or class, so strip all lines containing only whitespace
>          src = "\n".join([line for line in src.splitlines() if not
> line.isspace() and line != ""])
>
> -        spacing = "\n" * 4
> -
>          # execute it in the python terminal
> -        self.session.send_command(spacing + src + spacing)
> +        self.session.send_command(src + "\n")
>          self.session.send_command(
>              f"server = QuittableXMLRPCServer(('0.0.0.0',
> {listen_port}));server.serve_forever()",
>              "XMLRPC OK",
> @@ -267,6 +283,7 @@ def _send_packets_and_capture(
>          return scapy_packets
>
>      def close(self) -> None:
> +        """Close the traffic generator."""
>          try:
>              self.rpc_server_proxy.quit()
>          except ConnectionRefusedError:
> --
> 2.34.1
>
>

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