From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id C91CB45A36; Thu, 26 Sep 2024 11:12:17 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5A222402AE; Thu, 26 Sep 2024 11:12:17 +0200 (CEST) Received: from mail-ej1-f54.google.com (mail-ej1-f54.google.com [209.85.218.54]) by mails.dpdk.org (Postfix) with ESMTP id E37024025D for ; Thu, 26 Sep 2024 11:12:15 +0200 (CEST) Received: by mail-ej1-f54.google.com with SMTP id a640c23a62f3a-a8a897bd4f1so104040266b.3 for ; Thu, 26 Sep 2024 02:12:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1727341934; x=1727946734; darn=dpdk.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=LNmbR9ixl4NpJOSsWuhovdEnxNnsW/jLHZiajpUX0IE=; b=Qv1BaY/koNASg5uVB8DTKc0xuiIPnLCKtodWaXWU9r2JxrGmQbrXPrqnhNIG1mE+wM pElfjs3/dMHMzRi2iLxNUgPDaAuJU8UbiaMxQcgXGeCByjq9qaHz0qm9zJvyrEtpMAfj Z9359tZUeQu2wtFYP4BGyG0fYGKBTb7yITktUbKM1gcxPxPYznEpw/fLP+yra1m8HCcf 9URQz8IoWi9zwvlBO6Xw2Ut/akUFQ0oLy2I9RQnwYlgEFS1TrdkICK2EGdPmDDd3+deC eZIwF/HetvqXkA7L3I0q2oU+VUREUrbsKTOCjuYL9gY4bJNd058zpLYP08HQr1dWh/eL /WUA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1727341934; x=1727946734; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=LNmbR9ixl4NpJOSsWuhovdEnxNnsW/jLHZiajpUX0IE=; b=W/QdNAz6ajn0iU1IAex3SB/Y7ry6W6eYe/+2g5NJ2ZFBTAg31Nl7ijp08TrhRHR1Vz Vb4LPm7LddGd/vh0phYUsAFviDnuWYjgC0s4y9RNmNmF0kwr1NZDiQJvDHImQelXjnDV LTcqZSiBCVKjwtSAkgLFYN/6FMiuSJPH40p+vgHnVzpKdWuypfQ2Tft6ownRx1z/Yyi6 oKIjHx/iL1irQINWzem/xXHEAUpKjzXNvv3ZKRyRSF5uJT2zbDKwGc3I1e1XW08nYaMy H/Z921CAAXLB/Z3VM7ESeQPppDePl2afNUcj5e+hedVlGAK3gvTh/oHdqBoUxKsfVn0v axbw== X-Gm-Message-State: AOJu0YydgaD2n+BT4xdJdAeHPprPCRflrq+vhxkU2ACZEcoV0LfE8tQU QLjLe+6Ded/blI6eTKmEqm8r6Dcyb8mb6sxVpvw9Jq56a4wJQgyuX1m83h50VWU= X-Google-Smtp-Source: AGHT+IFClS+SGZw8zTx321oY9Jas4aNk8wui6P9p4uDD/DLnZ19QV72MKPun+xm0FDkMkWcg/rKhFQ== X-Received: by 2002:a17:907:709:b0:a8d:f04:b19b with SMTP id a640c23a62f3a-a93a0341d2cmr485854466b.2.1727341934408; Thu, 26 Sep 2024 02:12:14 -0700 (PDT) Received: from [192.168.200.22] ([84.245.121.62]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-a93930cad0asm323635666b.118.2024.09.26.02.12.13 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 26 Sep 2024 02:12:14 -0700 (PDT) Message-ID: <15a9640e-72cd-482d-9a3b-1e89c3e6d57c@pantheon.tech> Date: Thu, 26 Sep 2024 11:12:12 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4 1/1] dts: Remove XML-RPC server for Scapy TG and instead use PythonShell To: jspewock@iol.unh.edu, thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, wathsala.vithanage@arm.com, alex.chapman@arm.com, Luca.Vizzarro@arm.com, probb@iol.unh.edu, yoan.picchi@foss.arm.com, paul.szczepanek@arm.com, npratte@iol.unh.edu Cc: dev@dpdk.org References: <20240605175227.7003-1-jspewock@iol.unh.edu> <20240925173727.14111-1-jspewock@iol.unh.edu> <20240925173727.14111-2-jspewock@iol.unh.edu> Content-Language: en-US From: =?UTF-8?Q?Juraj_Linke=C5=A1?= In-Reply-To: <20240925173727.14111-2-jspewock@iol.unh.edu> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org The subject line should start with a lowercase character and it's also over the character limit. This fits and basically says the same: use PythonShell for Scapy instead of XML-RPC Scapy TG didn't fit but I think the above still works. > diff --git a/dts/framework/remote_session/single_active_interactive_shell.py b/dts/framework/remote_session/single_active_interactive_shell.py > @@ -93,9 +94,13 @@ def __init__( > timeout: float = SETTINGS.timeout, > app_params: Params = Params(), > name: str | None = None, > + **kwargs, > ) -> None: > """Create an SSH channel during initialization. > > + Additional key-word arguments can be passed through `kwargs` if needed for fulfilling other key-word -> keyword > + constructors in the case of multiple-inheritance. I also didn't notice this hyphen, it shouldn't be there. These hyphens are in a lot of places, I don't if I caught them all. > diff --git a/dts/framework/testbed_model/traffic_generator/scapy.py b/dts/framework/testbed_model/traffic_generator/scapy.py > @@ -6,311 +6,150 @@ > > A traffic generator used for functional testing, implemented with > `the Scapy library `_. > -The traffic generator uses an XML-RPC server to run Scapy on the remote TG node. > +The traffic generator uses an interactive shell to run Scapy on the remote TG node. > > -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. > +The traffic generator extends :class:`framework.remote_session.python_shell.PythonShell` to > +implement the methods for handling packets by sending commands into the interactive shell. Since this only mentions PythonShell, people could understand that's the only thing the tg extends. We should either add a qualifier such as additionally extends or just explicitly say it also extends the capturing TG (which is also useful information as it tells us which kind of TG it is). > +class ScapyTrafficGenerator(PythonShell, CapturingTrafficGenerator): > + """Provides access to scapy functions on a traffic generator node. > + This class extends the base with remote execution of scapy functions. All methods for > + processing packets are implemented using an underlying > + :class:`framework.remote_session.python_shell.PythonShell` which imports the Scapy library. > + Because of the dual-inheritance, this class has both methods that wrap scapy commands sent into The hyphen doesn't sound right. And maybe double would be a better term. I understand dual to mean two of the same and that doesn't fit for me as well. > + the shell and methods that run locally to fulfill traffic generation needs. To help make a > + clear distinction between the two, the names of the methods that wrap the logic of the > + underlying shell should be prepended with "shell". I think there would be value in explicitly saying that the shell runs on the TG node. > + Note that the order of inheritance is important for this class. In order to instantiate this > + class, the abstract methods of :class:`~.capturing_traffic_generator.CapturingTrafficGenerator` > + must be implemented. Since some of these methods are implemented in the underlying interactive > + shell, according to Python's Method Resolution Order (MRO), the interactive shell must come > + first. I didn't notice this before. Is this because of the close() method? Do we need to add any special steps to close the TG? Closing the interactive session should be enough, but I wanted to check with you. > + def __init__(self, tg_node: Node, config: ScapyTrafficGeneratorConfig, **kwargs): > + """Extend the constructor with Scapy TG specifics. > > - def __init__(self, *args, **kwargs): > - """Extend the XML-RPC server initialization. > + Initializes both the traffic generator and the interactive shell used to handle Scapy > + functions. The interactive shell will be started on `tg_node`. The additional key-word > + arguments in `kwargs` are used to pass into the constructor for the interactive shell. > > 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`. > + tg_node: The node where the traffic generator resides. > + config: The traffic generator's test run configuration. > + kwargs: Keyword arguments all traffic generators must support in order to allow for > + multiple-inheritance. I'm not sure why all and why they must support them. We can just say the supported keyword arguments corresdpond to the parameters of :meth:`PythonShell.__init__`. Also, multiple-inheritance -> multiple inheritance > diff --git a/dts/framework/testbed_model/traffic_generator/traffic_generator.py b/dts/framework/testbed_model/traffic_generator/traffic_generator.py > index 4ce1148706..176d5e9065 100644 > --- a/dts/framework/testbed_model/traffic_generator/traffic_generator.py > +++ b/dts/framework/testbed_model/traffic_generator/traffic_generator.py > @@ -16,23 +16,29 @@ > from framework.logger import DTSLogger, get_dts_logger > from framework.testbed_model.node import Node > from framework.testbed_model.port import Port > -from framework.utils import get_packet_summaries > +from framework.utils import MultiInheritanceBaseClass, get_packet_summaries > > > -class TrafficGenerator(ABC): > +class TrafficGenerator(MultiInheritanceBaseClass, ABC): > """The base traffic generator. > > Exposes the common public methods of all traffic generators and defines private methods > - that must implement the traffic generation logic in subclasses. > + that must implement the traffic generation logic in subclasses. This class also extends from > + :class:`framework.utils.MultiInheritanceBaseClass` to allow subclasses the ability to inherit > + from multiple classes to fulfil the traffic generating functionality without breaking > + single-inheritance. Hyphen again. > """ > > _config: TrafficGeneratorConfig > _tg_node: Node > _logger: DTSLogger > > - def __init__(self, tg_node: Node, config: TrafficGeneratorConfig): > + def __init__(self, tg_node: Node, config: TrafficGeneratorConfig, **kwargs): > """Initialize the traffic generator. > > + Additional key-word arguments can be passed through `kwargs` if needed for fulfilling other > + constructors in the case of multiple-inheritance. > + keyword, multiple inheritance