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 F0DAF454E3; Mon, 24 Jun 2024 22:54:17 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C9EBC40A76; Mon, 24 Jun 2024 22:54:17 +0200 (CEST) Received: from mail-pj1-f41.google.com (mail-pj1-f41.google.com [209.85.216.41]) by mails.dpdk.org (Postfix) with ESMTP id 027C4402E1 for ; Mon, 24 Jun 2024 22:54:17 +0200 (CEST) Received: by mail-pj1-f41.google.com with SMTP id 98e67ed59e1d1-2c86e3fb6e7so1223934a91.1 for ; Mon, 24 Jun 2024 13:54:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1719262456; x=1719867256; 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=MdWFVc83dtsUVCAs7DGy560a2dXgy7XpeEgSGYitxeg=; b=CCqc+qJmznmPj9B7c9luSJSCIjPg67XXRnl5sFwlB/2Raiqxy1Quisvz2qdeSnAH6h 3yy4pFdzzlMS5AfbwA0sJlUiL/AM4kKjMHik0eSPs0qu8XmSYBLLmMsTCkjyDOiHhiXO mPn8yahQACc/BQBF1Xr6wLTx5d5PRj8/qkAtM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719262456; x=1719867256; 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=MdWFVc83dtsUVCAs7DGy560a2dXgy7XpeEgSGYitxeg=; b=uyPOY89Zyw0F7yTclb7TxcdAV8FN5LnLCA8/wXigW+AbLygUVuzJ42dAORHehFWAx8 wj380Apc10u42AIyrHsgHIo6FG3eismpOFtnivb6sEAGYALGu2Wlf6Vb0DQVzLjQKjHz YFHs2848gnjcDugT3dAPHEmpInloCx8F2XAAhmCSfAsjhvDRZ422O509Xw9n+1yX0sb3 IInSUihKOsHFt56ZRMW7n+aBuqsB/XgE9IQ8Hi9VAhrA0qRlfq5Wreb9oWfgdXtQxSXe mdhfE4L3ADsv5WvT+OEdxa4UvQKwhlKqlVCLpdBq4F/lBDBvUYdASTFVbcBYyHAuZrTf +c6g== X-Forwarded-Encrypted: i=1; AJvYcCWamvyYNDvnjdn2PbVutf8YRNnTXEZKZAHZhI2iqq/mNhrx9yeEj1/wfxyx7OYj3Kudz2xTu+6+SB+QyVI= X-Gm-Message-State: AOJu0YzBKJdT0n4XOeULGvhYgQViYKLDEktUMltHtIuR+PMvg+6TVnYE WFiBPS0Ar2NCOOaHmtqIyROChA7EdA8bxBIWbwPaZIZNNscHog6BECRPiYfvy4SIRfY8iyUBK9v rKiAz5j6AZTPRvIoXBWs2kaO8CE5+ruy2eIk1EA== X-Google-Smtp-Source: AGHT+IEvTP/uQONHW0LBmty6x/u5ueEOxCfR5KOGYxPG6j/g53YHmcobIRwVIBdIfSTi0QNxgA4ZKpFKJ6OLg1D2uZ8= X-Received: by 2002:a17:90a:784a:b0:2c4:b515:46d4 with SMTP id 98e67ed59e1d1-2c861224475mr4000086a91.3.1719262455840; Mon, 24 Jun 2024 13:54:15 -0700 (PDT) MIME-Version: 1.0 References: <20240605175227.7003-1-jspewock@iol.unh.edu> <20240620231158.12008-1-jspewock@iol.unh.edu> <20240620231158.12008-3-jspewock@iol.unh.edu> In-Reply-To: From: Jeremy Spewock Date: Mon, 24 Jun 2024 16:54:04 -0400 Message-ID: Subject: Re: [PATCH v1 1/1] dts: Remove XML-RPC server for Scapy TG and instead use PythonShell To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: Honnappa.Nagarahalli@arm.com, paul.szczepanek@arm.com, yoan.picchi@foss.arm.com, npratte@iol.unh.edu, probb@iol.unh.edu, thomas@monjalon.net, wathsala.vithanage@arm.com, Luca.Vizzarro@arm.com, 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 Fri, Jun 21, 2024 at 10:14=E2=80=AFAM Juraj Linke=C5=A1 wrote: > > > > + #: Padding to add to the start of a line for python syntax complia= nce. > > + _padding: ClassVar[str] =3D " " * 4 > > We use padding in the context of packets so let's use something else > here, such as _python_indentation. I didn't think of that, good call. > > > + > > def __init__(self, tg_node: Node, config: ScapyTrafficGeneratorCo= nfig): > > """Extend the constructor with Scapy TG specifics. > > > > - The traffic generator first starts an XML-RPC on the remote `t= g_node`. > > - Then it populates the server with functions which use the Scap= y 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 :opt= ion:`--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)? Yeah, I can see how that would make sense. I'll update this. > > > + 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=3DTrue) > > > > This should probably mirror the superclass order from which the class is > subclassed - PythonShell first, then CapturingTrafficGenerator. Ack. > > 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 =3D False, timeout =3D 15, > start_on_init =3D True, params =3D "", **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 usef= ul. It could be useful, but it does increase the complexity a little bit. I could put it together and see how it looks. It definitely would be better to use super so we don't have to unnecessarily override the init method in the future, the only thing that would deter me slightly is the seemingly random `**kwargs` that don't get used inside the interactive shell class. Of course this ambiguity goes away if it's documented, so this shouldn't really be a problem. > > > assert ( > > self._tg_node.config.os =3D=3D OS.linux > > ), "Linux is the only supported OS for scapy traffic generati= on" > > > > - self.session =3D PythonShell( > > - self._tg_node, timeout=3D5, privileged=3DTrue, name=3D"Sca= pyXMLRPCServer" > > - ) > > + def start_application(self) -> None: > > + """Extends :meth:`framework.remote_session.interactive_shell.s= tart_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. That makes sense, I'll make the change. > > > + the shell for usage in later calls to the methods of this clas= s. > > + """ > > + 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. > That's not a bad idea, I could try and just use something from the scapy library and check the output. > Actually, we probably should add a smoke test that would do that - some > basic verification of traffic generators. This idea I like a lot! We've had the traffic generator itself implemented for a while now, a smoke test that ensures traffic can flow in general would definitely be beneficial. >