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 4B94945920; Fri, 6 Sep 2024 18:42:00 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 14F0A42EB0; Fri, 6 Sep 2024 18:42:00 +0200 (CEST) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id 4624542E82 for ; Fri, 6 Sep 2024 18:41:59 +0200 (CEST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id DF75CFEC; Fri, 6 Sep 2024 09:42:25 -0700 (PDT) Received: from [10.57.17.223] (unknown [10.57.17.223]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id CDCAE3F73B; Fri, 6 Sep 2024 09:41:57 -0700 (PDT) Message-ID: Date: Fri, 6 Sep 2024 17:41:56 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 4/5] dts: add ability to start/stop testpmd ports Content-Language: en-GB To: Jeremy Spewock Cc: dev@dpdk.org, =?UTF-8?Q?Juraj_Linke=C5=A1?= , Honnappa Nagarahalli , Paul Szczepanek References: <20240806121417.2567708-1-Luca.Vizzarro@arm.com> <20240806124642.2580828-1-luca.vizzarro@arm.com> <20240806124642.2580828-5-luca.vizzarro@arm.com> From: Luca Vizzarro In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 09/08/2024 16:13, Jeremy Spewock wrote: > I tried to address this very same problem in the scatter expansion > patch series but, admittedly, I like your approach better. There is > one thing that Juraj and I discussed on that thread however that is > probably worth noting here regarding verification of the stopping and > starting ports. The main idea behind that being if the user calls a > testpmd method and specifically specifies they don't want to verify > the outcome of that method, but then we still verify whether we > stopped the ports or not, that could cause confusion. The compromise > we ended up settling for was just to make a parameter to the decorator > that allows you to statically decide which decorated methods should > verify the ports stopping and which shouldn't. It was mainly because > of the ability to specify "verify" as a kwarg or an arg and the types > being messy. The discussion we had about it starts here if you were > interested in reading it: > http://inbox.dpdk.org/dev/dff89e16-0173-4069-878c-d1c34df1ae13@pantheon.tech/ When it comes to starting and stopping ports, we may need to ensure that the operations were done correctly. Surely we don't want to start forwarding thinking that the ports are initialised, when they could be not... I guess the point here is that it may be important to save the correct state. Not sure, I guess verify could always be added later if needed. > On Tue, Aug 6, 2024 at 8:49 AM Luca Vizzarro wrote: >> >> Add testpmd commands to start and stop all the ports, so that they can >> be configured. Because there is a distinction of commands that require >> the ports to be stopped and started, also add decorators for commands >> that require a specific state, removing this logic from the test >> writer's duty. >> >> Signed-off-by: Luca Vizzarro >> Reviewed-by: Paul Szczepanek >> --- > >> +P = ParamSpec("P") >> +TestPmdShellMethod = Callable[Concatenate["TestPmdShell", P], Any] > > Agggh, I spent so much time trying to figure out how to do exactly > this when I was working on the scatter series but I couldn't get a > nice typehint while specifying that I wanted a TestPmdShell first. I > had no idea that Concatenate was a type but I will definitely keep > that one in mind. > Glad I could help! :D >> +def requires_started_ports(func: TestPmdShellMethod) -> TestPmdShellMethod: >> + """Decorator for :class:`TestPmdShell` commands methods that require started ports. >> + >> + If the decorated method is called while the ports are stopped, then these are started before >> + continuing. >> + """ >> + >> + @functools.wraps(func) >> + def _wrapper(self: "TestPmdShell", *args: P.args, **kwargs: P.kwargs): >> + if not self.ports_started: >> + self._logger.debug("Ports need to be started to continue") >> + self.start_all_ports() >> + >> + return func(self, *args, **kwargs) >> + >> + return _wrapper >> + > > I'm not sure how much it is necessary since it is pretty clear what > these decorators are trying to do with the parameter, but since these > are public methods I think not having an "Args:" section for the func > might trip up the doc generation. We have several methods that have one-liner docstrings... so not really sure. I was hoping to have the doc generation merged as testing what's correct and what's not is currently complicated. >> >> + self.ports_started = not self._app_params.disable_device_start >> + > > It might be useful to add this attribute as a stub in the class just > to be clear on the typing. It also helps me understand things at more > of a glance personally. You make a fair point here.