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 5CB9F4577A; Fri, 9 Aug 2024 17:13:51 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 41B1F42FBB; Fri, 9 Aug 2024 17:13:51 +0200 (CEST) Received: from mail-pf1-f175.google.com (mail-pf1-f175.google.com [209.85.210.175]) by mails.dpdk.org (Postfix) with ESMTP id 329FA42E9D for ; Fri, 9 Aug 2024 17:13:50 +0200 (CEST) Received: by mail-pf1-f175.google.com with SMTP id d2e1a72fcca58-710ce81bf7dso1335204b3a.0 for ; Fri, 09 Aug 2024 08:13:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1723216429; x=1723821229; 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=RJklsfPXbnH2t2Lf2+RXalALJaxg+vcxAHDrTTdVZUk=; b=SlYrI1EM1bjOszExxzb+TX6afxZ5F6pi6rVxHMU0JkCba+6ZNek3xZM0OqlmIJOEuT vFLCjNmmyNPCquyKjJT5I0X4tJCKqSX04k03XAonl9dXXKNmXY0bNGASz5AtM96HcDJT SO7d+nXG5l6inZs9VGkgCFQvKnsmgMD3Yl4Ws= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723216429; x=1723821229; 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=RJklsfPXbnH2t2Lf2+RXalALJaxg+vcxAHDrTTdVZUk=; b=PBP/eJ8yT5fcwjXUPMRV44njkLPXh+QMy9ZmghVJjGgzaM4YH5vsue4+Q37GGbHQKI oSacnjUPKPBGy8a6UP6Bz3iKG0K0YQN8kTXasO8omV0bkFMYu9Yo+ArnyBz1X7GNgcOw xp+vQ76iICyqIWGum0g8Uwxdv1m26D3Jem6V+SLyut1+kfgdSWhEALwoRCl+cAqExejg x1+9XBCsQBzkgiK2U5HiCfzcnqht7NuaKNv6ExaKD+1eC/1s45ayc3eFnY0aWrXa3YBm ZcYo4ZNDmGP7GGHCOOPGKpROCJp6zn4ggED2Tkes4RfQKhDJ+cRuORQomL0MQCmymmVs BGjg== X-Gm-Message-State: AOJu0YypSCHlZ016OKVCs2Llj5zCkkZLIMroUvpnamsuPN8jWUZthsxD GDgpcmrFUyBmPrD0sJ9+GeP5Tvzez8faQ/WRwLu0cN+HMjYkdhU8pQjY0GOEEuxtN2v3Yo3pRDu EzM7XsDOb7yKNZRExuFczwuUcJKmEhYaD6JQBIGdMPmRRvH3wX2w= X-Google-Smtp-Source: AGHT+IF3pdIJ2/fWzIbq2405H0nTN5yKnaQ7biD0DoTgt2f1LSSjL0rGTnoFcbgT4GGUnCEMoRfOJEKkmYMfV9ByK30= X-Received: by 2002:a17:90b:2383:b0:2cd:3e75:4e51 with SMTP id 98e67ed59e1d1-2d1e806a06fmr1979911a91.39.1723216429112; Fri, 09 Aug 2024 08:13:49 -0700 (PDT) MIME-Version: 1.0 References: <20240806121417.2567708-1-Luca.Vizzarro@arm.com> <20240806124642.2580828-1-luca.vizzarro@arm.com> <20240806124642.2580828-5-luca.vizzarro@arm.com> In-Reply-To: <20240806124642.2580828-5-luca.vizzarro@arm.com> From: Jeremy Spewock Date: Fri, 9 Aug 2024 11:13:37 -0400 Message-ID: Subject: Re: [PATCH v2 4/5] dts: add ability to start/stop testpmd ports To: Luca Vizzarro Cc: dev@dpdk.org, =?UTF-8?Q?Juraj_Linke=C5=A1?= , Honnappa Nagarahalli , Paul Szczepanek 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 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.tec= h/ On Tue, Aug 6, 2024 at 8:49=E2=80=AFAM 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 =3D ParamSpec("P") > +TestPmdShellMethod =3D 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. > + > > class TestPmdDevice: > """The data of a device that testpmd can recognize. > @@ -577,12 +581,51 @@ class TestPmdPortStats(TextParser): > tx_bps: int =3D field(metadata=3DTextParser.find_int(r"Tx-bps:\s+(\d= +)")) > > > +def requires_stopped_ports(func: TestPmdShellMethod) -> TestPmdShellMeth= od: > + """Decorator for :class:`TestPmdShell` commands methods that require= stopped ports. > + > + If the decorated method is called while the ports are started, then = these are stopped before > + continuing. > + """ > + > + @functools.wraps(func) > + def _wrapper(self: "TestPmdShell", *args: P.args, **kwargs: P.kwargs= ): > + if self.ports_started: > + self._logger.debug("Ports need to be stopped to continue") > + self.stop_all_ports() > + > + return func(self, *args, **kwargs) > + > + return _wrapper > + > + > +def requires_started_ports(func: TestPmdShellMethod) -> TestPmdShellMeth= od: > + """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. > + > class TestPmdShell(DPDKShell): > """Testpmd interactive shell. > > The testpmd shell users should never use > the :meth:`~.interactive_shell.InteractiveShell.send_command` method= directly, but rather > call specialized methods. If there isn't one that satisfies a need, = it should be added. > + > + Attributes: > + ports_started: Indicates whether the ports are started. > """ > > _app_params: TestPmdParams > @@ -619,6 +662,9 @@ def __init__( > name, > ) > > + self.ports_started =3D 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. > + @requires_started_ports > def start(self, verify: bool =3D True) -> None: > """Start packet forwarding with the current configuration. > -- > 2.34.1 >