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 E00BB45A1C; Tue, 24 Sep 2024 18:34:52 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7ECF04028E; Tue, 24 Sep 2024 18:34:52 +0200 (CEST) Received: from mail-pj1-f45.google.com (mail-pj1-f45.google.com [209.85.216.45]) by mails.dpdk.org (Postfix) with ESMTP id 53A1E40274 for ; Tue, 24 Sep 2024 18:34:51 +0200 (CEST) Received: by mail-pj1-f45.google.com with SMTP id 98e67ed59e1d1-2daaa9706a9so4686220a91.1 for ; Tue, 24 Sep 2024 09:34:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1727195690; x=1727800490; 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=KZASqZQi2hF5R91KxedFnhIraAbYou6oDpUporAVrqE=; b=PBNF2/dUYzi/7A5mMU11HryKHbNuOo789tXaD/Q/BDgp3X3yfE/53vL3jkjPohdpdI dJlB5xEjin449NaY6GFuc2q9+wGRkvGPl3bhz7GlgrjrfDcuY2pr/prRugtzwwOrmJaZ fRveG5vFn/oJFvlaJpowAsegrtREbZITN2Qv4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1727195690; x=1727800490; 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=KZASqZQi2hF5R91KxedFnhIraAbYou6oDpUporAVrqE=; b=WzYQZSYn7ZiOgfDw2/apZj8N9gnvFPTftx9CS3ulZwpULMuMpyWyzK4kkufKDBunnt C901fXg7Xzh10kYrYqy/XbIwxO5AKT/ddypbdbGGdjZP6jpjxXvpoIMDQ+REaTg3suBe up9cTnrR2RECGOnuT9ok7sJWfHRrTqVCusWGen7TD7rqvHmMjuILCUfiBcPrt6M/MXmr gncMKMYQLpIz9VeMYnI/d/KqQuC7ROHeP+jbw2MwWbkAU6J62+6t4GpUHLU0M8iwpUvu IuHlxhRQh5KoRBIM1ckXrs7nyb1jtUw9mcTROqtTGYdrPK/YN3/QvJ7ZEV76/4K+FojE EiWA== X-Forwarded-Encrypted: i=1; AJvYcCV4WDxOlm/bEbmpFd2HlYKgK+GccFVNeatbHUA/rsoh3GMP/5B6za3m+BcS9dQ/1ZGUFJc=@dpdk.org X-Gm-Message-State: AOJu0Yy0MpuxuNMPZ6mGS+PKaKdNtAOTj6mIvqo+1H6DY1t0ucQSs5ZN 5mzVG/Tp3n8coig+0uztHmftZ0Kisd91LG8rWYtAl1YpAMCPd5wgRy98p/KIlA3D5jESBjEsrfe Bjs5eWw0Je1VD+a0VzcWOca5GlxRapCpjBorafw== X-Google-Smtp-Source: AGHT+IH+0QLVsEyu3BxujA6I8+ivmQRu4FvDwWpY4gHBaAeu5CB580wcpAOPV4ykmQ5ixOxd/71sWkFJ93KGE7u+q4w= X-Received: by 2002:a17:90b:314d:b0:2da:5acc:4e9c with SMTP id 98e67ed59e1d1-2dd7f6b6e91mr18298014a91.29.1727195690314; Tue, 24 Sep 2024 09:34:50 -0700 (PDT) MIME-Version: 1.0 References: <20240605175227.7003-1-jspewock@iol.unh.edu> <20240919190229.22095-1-jspewock@iol.unh.edu> <20240919190229.22095-2-jspewock@iol.unh.edu> <0e66d03d-652b-481a-96ac-7c18337a1e07@pantheon.tech> In-Reply-To: <0e66d03d-652b-481a-96ac-7c18337a1e07@pantheon.tech> From: Jeremy Spewock Date: Tue, 24 Sep 2024 12:34:38 -0400 Message-ID: Subject: Re: [PATCH v3 1/1] dts: Remove XML-RPC server for Scapy TG and instead use PythonShell To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: alex.chapman@arm.com, npratte@iol.unh.edu, Luca.Vizzarro@arm.com, probb@iol.unh.edu, paul.szczepanek@arm.com, thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, wathsala.vithanage@arm.com, yoan.picchi@foss.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 Tue, Sep 24, 2024 at 6:55=E2=80=AFAM Juraj Linke=C5=A1 wrote: > > I like how this looks. I have a number of minor comments (mainly wording > and naming), but overall it looks very good. > > On 19. 9. 2024 21:02, jspewock@iol.unh.edu wrote: > > From: Jeremy Spewock > > > > Previously all scapy commands were handled using an XML-RPC server that > > ran on the TGNode. This unnecessarily enforces a minimum Python version > > of 3.10 on the server that is being used as a traffic generator and > > complicates the implementation of scapy methods. > > What is the TG's minimum Python version now? > https://bugs.dpdk.org/show_bug.cgi?id=3D1388 says this will become a moot > point, but we're still using Python on the remote node. Right, there is still some dependency there. I'm not sure the exact versions, but doing some looking around I believe one of the newest things scapy tools we use in the framework is the AsyncSniffer and the scapy documentation [1] says that it has been available since version 2.4.3, and then when I looked at that version of the scapy package [2] it looks like it claims to support python 2.7 and python 3.4-7. Poking around in the documentation/code from scapy version 2.4.3 it also looks like the syntax is very similar, so I believe it would work, but I'm not sure I have any hosts that I could run on which have python 3.4. Maybe that does make the dependency essentially a moot point considering these are fairly old python versions. [1] https://github.com/secdev/scapy/blob/master/doc/scapy/usage.rst [2] https://pypi.org/project/scapy/2.4.3/ > > > > 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= s needed to fulfill other > > Typo: is -> if > > And key-word should be just keyword. > Ack. > Also, should we add super().__init__(), seeing as we also added it to > TrafficGenerator? > We probably should so that it doesn't matter which order you specify the two in and they still work, this is probably something that I missed in the rebase. > > > diff --git a/dts/framework/testbed_model/traffic_generator/scapy.py b/d= ts/framework/testbed_model/traffic_generator/scapy.py > > > +class ScapyTrafficGenerator(PythonShell, CapturingTrafficGenerator): > > > + def __init__(self, tg_node: Node, config: ScapyTrafficGeneratorCon= fig, **kwargs): > > The usage of privileged=3DTrue when creating the instance confused me for > a bit, because it's not this class's argument, but rather > SingleActiveInteractiveShell's. I thing we should document the arguments > of SingleActiveInteractiveShell in the Keyword Args section. We probably > need just a link to SingleActiveInteractiveShell. Sure, I can add some kind of reference. I agree that it's confusing, I wish there was a simple way we could unpack the parameters of the interactive shell into kwargs, but when I was searching I couldn't really find anything great. Other than doing something like making TypedDict classes for interactive shell parameters, but I'm not sure that's super sleek either. > > > > + def _create_packet_list(self, packets: list[Packet]) -> None: > > Maybe we could apply some of the ideas from the local/remote naming > scheme I talked about in the tg devbind script patch here. Whatever > happens on the TG could be prefixed with remote and whatever is > happening locally would be without the prefix (or maybe whatever is > happnening on the TG shouldn't be prefixed (or a different prefix - > shell)? Makes sense, but then we'd need a good prefix for what's > happening on the execution environment. Maybe this also needs to be in a > different patch, after it's been though through with everything else in I'll still write something here that makes the distinction and that other patch could reformat if the author thought something else was better. > mind.). That would make this _create_remote_packet_list, but we're just > setting a variable (the passed packets have already been built), so > maybe _set_remote_packet_list? > > Or maybe all of the remote methods could start with remote, making it > _remote_set_packet_list (or _shell_set_packet_list? Doesn't sound bad.). > Not sure which is better, maybe after renaming more of these it's going > to be clearer. This is a good idea. I like remote more initially, but I'll try it out on some of the methods and see if shell fits better. Regardless, I think one of those two would be a good option as well. > > Whatever we go with, the naming scheme should be explained in the > class's dosctring. Ack. > > > > + def _create_sniffer( > > + self, packets_to_send: list[Packet], send_port: Port, recv_por= t: Port, filter_config: str > > + ) -> None: > > + """Create an asynchronous sniffer in the shell. > > + > > + A list of packets to send is added to the sniffer inside of a = callback function so that > > + they are immediately sent at the time sniffing is started. > > This is still a bit confusing (where are the packets added and what is > inside the function?); We may not need the Scapy implementation details. > We could just say the packets are sent immediately at the time sniffing > is started. Or maybe: > A list of packets is passed to the sniffer's callback function so that > they are immediately sent at the time sniffing is started. Sure, I think this wording makes sense. That's a good point that this doc-string really doesn't need that much information about the implementation and it could be confusing. > > It's a private method, so maybe the implementation detail could be > valuable, even though it's not fully clear why the implication is true - > you still need some knowledge of how the sniffer works to put everything > together. I think it helps especially because the send method just creates a sniffer, then it starts and stops sniffing so I wanted to add more context as to how that actually leads to packets being sent. I think your wording above though gives enough information to understand that in a more elegant way. > > > > + sniffer_commands =3D [ > > + f"{self._sniffer_name} =3D AsyncSniffer(", > > + f"iface=3D'{recv_port.logical_name}',", > > + "store=3DTrue,", > > + "started_callback=3Dlambda *args: sendp(", > > As far as I can tell, we're not using the args, so we can just use > "lambda: sendp()" We don't use the argument, but there are positional arguments passed into this function by scapy which is why we have to catch and ignore them. > > > + ( > > + f"{self._python_indentation}{self._send_packet_list_na= me}," > > Is the indentation needed here? It's not required, but I think it makes the logs more readable. > > > > @@ -335,32 +204,19 @@ def _send_packets_and_capture( > > I think we can improve the order of methods in the class. I'd put > _send_packets() and _send_packets_and_capture() after the public > methods. These two methods are the most important ones (implementing the > abstract methods). The other methods should come after that in the order > they're used in _send_packets() and _send_packets_and_capture(). Sure, I can do that. > > > self, > > packets: list[Packet], > > send_port: Port, > > - receive_port: Port, > > + recv_port: Port, > > filter_config: PacketFilteringConfig, > > duration: float, > > - capture_name: str =3D _get_default_capture_name(), > > ) -> list[Packet]: > > > + """Implementation for sending packets and capturing any receiv= ed traffic. > > + > > + This method first creates an asynchronous sniffer that holds t= he packets to send, then > > + starts and stops and starts said sniffer. > > This looks like a typo: starts and stops and starts. > Yup, good catch. > Maybe we could add that we collect the received packets from the sniffer > once we've stopped it. > Sure, that could be useful information. > > > diff --git a/dts/framework/testbed_model/traffic_generator/traffic_gene= rator.py b/dts/framework/testbed_model/traffic_generator/traffic_generator.= py > > > @@ -16,23 +16,29 @@ > > > +class TrafficGenerator(MultiInheritanceBaseClass, ABC): > > > + 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. > > + > > The wording in this sentence is slightly different that the one in > SingleActiveInteractiveShell. Let's unify them. Ack. >