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 29B2845A39; Thu, 26 Sep 2024 16:54:57 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id AC61C4042E; Thu, 26 Sep 2024 16:54:56 +0200 (CEST) Received: from mail-pj1-f51.google.com (mail-pj1-f51.google.com [209.85.216.51]) by mails.dpdk.org (Postfix) with ESMTP id 1646F40279 for ; Thu, 26 Sep 2024 16:54:55 +0200 (CEST) Received: by mail-pj1-f51.google.com with SMTP id 98e67ed59e1d1-2e06f5d4bc7so995830a91.2 for ; Thu, 26 Sep 2024 07:54:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1727362494; x=1727967294; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=IfY2inzXF85KWPZg0xOQ6+umi/waJy141aNSRKxn9pA=; b=GUgAZEILEfYth8NBYUMijiquPkewNb767T3BdDkropPptPo2X9pYcqR+smJP0i2Rp4 uTGEkgGAPseMYoj4xux2ydevpTgL6AjSDA7dtvBAXf4i6Na2Cm++GJxty1s26B0UGrqO yD9rz+ZjoQwUCKKhi92cJcKf9eZDmBYePAmis= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1727362494; x=1727967294; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=IfY2inzXF85KWPZg0xOQ6+umi/waJy141aNSRKxn9pA=; b=PuDIySP36lRKV2dDF905fYMXPsq/xfQwtRs+4DvjI1UGl+9v/d7wvMeGRE2YjhyEZG YoCBlKqWh6tW2SqKxT5tRKstvu5lb0zLXkAV/9TrCrUScLippb5dhhgfN5kY1GmgWQ6G qqqpX7+/XJ28AOPZF40O+BUxKDCEr5woSmLoPb/UyVnBkeMsD7kxFeQ5AlALs6qqyvLJ T6hVa2c3uERznuOBWVhSEYgK7wqHnfKQiLhiOKhd3vETY35+7Y20byyQc1E6nKShnLHV bgLzZf2EZ5cX5l6nad8gWIGW2Q4BGz7y97xOK/QAu6Kz8PBxYHIqx40qmZN3zKX8+g26 PXEA== X-Forwarded-Encrypted: i=1; AJvYcCUfSIyiI0Nk2iWclhRFWRbt+gYvtj3CoRoe19ij1nLgWveA0rmuTz2rxS2f+tgt2LFV69c=@dpdk.org X-Gm-Message-State: AOJu0Yy5POiBRKtmvdLEzIPGdT1b5VKer+uSgyCgmliV+KZvzXS6fx4w RiwhrlmFG3O5uKdIkpmvKKhkG268ZavEgtlalwudwNAnquNePk/3v0t+muDDnIrNCCSsIK7TmNb Pgl46fPkA51f8zBxXAdHSjBn3dgLycbMwd26DZw== X-Google-Smtp-Source: AGHT+IG5qSj4R5sXRMma78HjGo/qq0rf2cS426ykhuFeB/rx1KrZ9RuGTEVd4hDULh4A3JoJeC6tVSr9vliLEzpU2z8= X-Received: by 2002:a17:90a:51c4:b0:2c9:9f50:3f9d with SMTP id 98e67ed59e1d1-2e06ae2cae8mr7503573a91.5.1727362493928; Thu, 26 Sep 2024 07:54:53 -0700 (PDT) MIME-Version: 1.0 References: <20240605175227.7003-1-jspewock@iol.unh.edu> <20240925173727.14111-1-jspewock@iol.unh.edu> <20240925173727.14111-2-jspewock@iol.unh.edu> <15a9640e-72cd-482d-9a3b-1e89c3e6d57c@pantheon.tech> In-Reply-To: <15a9640e-72cd-482d-9a3b-1e89c3e6d57c@pantheon.tech> From: Jeremy Spewock Date: Thu, 26 Sep 2024 10:54:42 -0400 Message-ID: Subject: Re: [PATCH v4 1/1] dts: Remove XML-RPC server for Scapy TG and instead use PythonShell To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: 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, dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 On Thu, Sep 26, 2024 at 5:12=E2=80=AFAM Juraj Linke=C5=A1 wrote: > > 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_she= ll.py b/dts/framework/remote_session/single_active_interactive_shell.py > > > @@ -93,9 +94,13 @@ def __init__( > > timeout: float =3D SETTINGS.timeout, > > app_params: Params =3D Params(), > > name: str | None =3D None, > > + **kwargs, > > ) -> None: > > """Create an SSH channel during initialization. > > > > + Additional key-word arguments can be passed through `kwargs` i= f needed for fulfilling other > > key-word -> keyword Ack. > > > + 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. I'm not sure why I put so many, haha. I'll look through and try to find the rest. > > > diff --git a/dts/framework/testbed_model/traffic_generator/scapy.py b/d= ts/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 remot= e TG node. > > +The traffic generator uses an interactive shell to run Scapy on the re= mote TG node. > > > > -The traffic generator uses the :mod:`xmlrpc.server` module to run an X= ML-RPC server > > -in an interactive remote Python SSH session. The communication with th= e 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 th= e 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). Good idea, I'll add this. > > > > +class ScapyTrafficGenerator(PythonShell, CapturingTrafficGenerator): > > + """Provides access to scapy functions on a traffic generator node. > > > + This class extends the base with remote execution of scapy functio= ns. All methods for > > + processing packets are implemented using an underlying > > + :class:`framework.remote_session.python_shell.PythonShell` which i= mports 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. Ack. > > > + the shell and methods that run locally to fulfill traffic generati= on needs. To help make a > > + clear distinction between the two, the names of the methods that w= rap 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. Ack. > > > > + Note that the order of inheritance is important for this class. In= order to instantiate this > > + class, the abstract methods of :class:`~.capturing_traffic_generat= or.CapturingTrafficGenerator` > > + must be implemented. Since some of these methods are implemented i= n the underlying interactive > > + shell, according to Python's Method Resolution Order (MRO), the in= teractive 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. Yes it is because of the close method in the traffic generator. I think closing the shell should be all we need to do really, there isn't anything else this traffic generator is really using on the host. > > > > + def __init__(self, tg_node: Node, config: ScapyTrafficGeneratorCon= fig, **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 she= ll 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 fo= r 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 s= uperclass'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 supp= ort in order to allow for > > + multiple-inheritance. > > I'm not sure why all and why they must support them. We can just say the I guess other leaf subclasses of traffic generators which don't inherit from two things wouldn't actually need them, you're right. I was just thinking that they had to be supported since the base class supports them to allow for the multiple inheritance, but thinking more on it now not all of them have to. > supported keyword arguments corresdpond to the parameters of > :meth:`PythonShell.__init__`. Sure, I like this wording. > > Also, multiple-inheritance -> multiple inheritance Clearly I need to do some reading about when I should actually be using hyphens :). > > > > diff --git a/dts/framework/testbed_model/traffic_generator/traffic_gene= rator.py b/dts/framework/testbed_model/traffic_generator/traffic_generator.= py > > index 4ce1148706..176d5e9065 100644 > > --- a/dts/framework/testbed_model/traffic_generator/traffic_generator.p= y > > +++ b/dts/framework/testbed_model/traffic_generator/traffic_generator.p= y > > @@ -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_summ= aries > > > > > > -class TrafficGenerator(ABC): > > +class TrafficGenerator(MultiInheritanceBaseClass, ABC): > > """The base traffic generator. > > > > Exposes the common public methods of all traffic generators and d= efines private methods > > - that must implement the traffic generation logic in subclasses. > > + that must implement the traffic generation logic in subclasses. Th= is class also extends from > > + :class:`framework.utils.MultiInheritanceBaseClass` to allow subcla= sses the ability to inherit > > + from multiple classes to fulfil the traffic generating functionali= ty without breaking > > + single-inheritance. > > Hyphen again. Ack. > > > """ > > > > _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` i= f needed for fulfilling other > > + constructors in the case of multiple-inheritance. > > + > > keyword, multiple inheritance Ack. > >