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 8567C45A16; Tue, 24 Sep 2024 12:55:34 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4572940295; Tue, 24 Sep 2024 12:55:34 +0200 (CEST) Received: from mail-ej1-f52.google.com (mail-ej1-f52.google.com [209.85.218.52]) by mails.dpdk.org (Postfix) with ESMTP id 5692F4028E for ; Tue, 24 Sep 2024 12:55:31 +0200 (CEST) Received: by mail-ej1-f52.google.com with SMTP id a640c23a62f3a-a8a7596b7dfso210707166b.0 for ; Tue, 24 Sep 2024 03:55:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1727175331; x=1727780131; darn=dpdk.org; h=content-transfer-encoding:in-reply-to:content-language:references :cc:to:subject:from:user-agent:mime-version:date:message-id:from:to :cc:subject:date:message-id:reply-to; bh=ahbJn1BPzpky45c3kvmuVMCTzz3BIrJONLKkkeNX1UU=; b=tJOoNztA81gssuLARoWKCFtJlZMTLvgxLXlsS6bK6SOM/vVXVK3oh6PfuX1rBBNlOU DE45bKWHJ/rs+/9cmb35lkgAzNV4W/YJcu0L/6dKOyx8jY60oXnNTs73JwuUIQa6fVjz m1DSc+zLODu7PBEF5OJqJ3PocBtzn0TgU88mJPyOH3mMyN/LsXAiactmG3G4coxHnMH8 AWuFAWoPUpvcNkkJcwk24swWZC2UWkdZ9MePbRc4sf4no1d0LFYcYFegACLrNL2e9Sic ggN42qKAb/3AprLfNhdZYCZaE0JS7PjgoEFatTszXWBuPtXnZtTKoZOc3azY/VemtdpR e9jQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1727175331; x=1727780131; h=content-transfer-encoding:in-reply-to:content-language:references :cc:to:subject:from:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=ahbJn1BPzpky45c3kvmuVMCTzz3BIrJONLKkkeNX1UU=; b=QJ81SOXsfWUGQO7cuNCJt2rmmbrj+yFTt++RBb734iqw/yV+G3WoTOVSyWa221AlJe j4EpoVNUZ168U19+c8SK+APA+82ri+7sQ5HLNQFQxibLcCfAd8svYkBn2lWvP/G4rb1Q diPa7D6b2R1+y4wWTiQkerpYZvKNG7xljUxIiStcHTBKPeuutI71q6iRZvv/TFWT6d8+ HYcS3NHHywu+jvR6dtW1UM0LD7ijsit+8487+rT1MwiKz2eYamB6mnz37b1GCVl384E/ aQBcvmDxORMSQHev7yQg0uvrLRw4yPgCSRubxb6oBNH22Uj2jClu0L+70l8gBP5Wv3Hu FRug== X-Gm-Message-State: AOJu0Ywjq/XBBdWwrcZpYlvcCoOTRxIV/No5Dfps6jfITHhNy66OpJxQ WrX1u+v4+S/Vvk+jPL1XuG3G9aIaBpBW/b0DQv8BJjLOuQHqWP02v1ejKPLazrs= X-Google-Smtp-Source: AGHT+IEat0vMEkBEnD2a/9nEaAqGQM4Onmu0c+8ovOufsgtJYWFFfWn/TOX/P4WSRB6rIK5SR/q7nQ== X-Received: by 2002:a17:907:36cc:b0:a8a:754a:e1c1 with SMTP id a640c23a62f3a-a92c4810d6amr266264666b.8.1727175330875; Tue, 24 Sep 2024 03:55:30 -0700 (PDT) Received: from [10.12.0.236] ([81.89.53.154]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-a9392f34809sm70765366b.37.2024.09.24.03.55.29 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 24 Sep 2024 03:55:30 -0700 (PDT) Message-ID: <0e66d03d-652b-481a-96ac-7c18337a1e07@pantheon.tech> Date: Tue, 24 Sep 2024 12:55:29 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Subject: Re: [PATCH v3 1/1] dts: Remove XML-RPC server for Scapy TG and instead use PythonShell To: jspewock@iol.unh.edu, 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 Cc: dev@dpdk.org References: <20240605175227.7003-1-jspewock@iol.unh.edu> <20240919190229.22095-1-jspewock@iol.unh.edu> <20240919190229.22095-2-jspewock@iol.unh.edu> Content-Language: en-US In-Reply-To: <20240919190229.22095-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 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=1388 says this will become a moot point, but we're still using Python on the remote node. > 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` is needed to fulfill other Typo: is -> if And key-word should be just keyword. Also, should we add super().__init__(), seeing as we also added it to TrafficGenerator? > diff --git a/dts/framework/testbed_model/traffic_generator/scapy.py b/dts/framework/testbed_model/traffic_generator/scapy.py > +class ScapyTrafficGenerator(PythonShell, CapturingTrafficGenerator): > + def __init__(self, tg_node: Node, config: ScapyTrafficGeneratorConfig, **kwargs): The usage of privileged=True 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. > + 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 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. Whatever we go with, the naming scheme should be explained in the class's dosctring. > + def _create_sniffer( > + self, packets_to_send: list[Packet], send_port: Port, recv_port: 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. 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. > + sniffer_commands = [ > + f"{self._sniffer_name} = AsyncSniffer(", > + f"iface='{recv_port.logical_name}',", > + "store=True,", > + "started_callback=lambda *args: sendp(", As far as I can tell, we're not using the args, so we can just use "lambda: sendp()" > + ( > + f"{self._python_indentation}{self._send_packet_list_name}," Is the indentation needed here? > @@ -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(). > self, > packets: list[Packet], > send_port: Port, > - receive_port: Port, > + recv_port: Port, > filter_config: PacketFilteringConfig, > duration: float, > - capture_name: str = _get_default_capture_name(), > ) -> list[Packet]: > + """Implementation for sending packets and capturing any received traffic. > + > + This method first creates an asynchronous sniffer that holds the packets to send, then > + starts and stops and starts said sniffer. This looks like a typo: starts and stops and starts. Maybe we could add that we collect the received packets from the sniffer once we've stopped it. > diff --git a/dts/framework/testbed_model/traffic_generator/traffic_generator.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` if 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.