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 850264570F; Thu, 1 Aug 2024 11:24:15 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 51ECF42EB4; Thu, 1 Aug 2024 11:24:15 +0200 (CEST) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id D8A4E402C8 for ; Thu, 1 Aug 2024 11:24:13 +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 DA505DA7; Thu, 1 Aug 2024 02:24:38 -0700 (PDT) Received: from [10.1.38.17] (JR4XG4HTQC.cambridge.arm.com [10.1.38.17]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E30DC3F5A1; Thu, 1 Aug 2024 02:24:11 -0700 (PDT) Message-ID: <15f1aeb5-3123-4c87-90b7-46b43323a6db@arm.com> Date: Thu, 1 Aug 2024 10:24:10 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] dts: add flow rule dataclass to testpmd shell Content-Language: en-GB To: Dean Marx , probb@iol.unh.edu, npratte@iol.unh.edu, jspewock@iol.unh.edu, yoan.picchi@foss.arm.com, Honnappa.Nagarahalli@arm.com, paul.szczepanek@arm.com, juraj.linkes@pantheon.tech Cc: dev@dpdk.org References: <20240726141548.688-1-dmarx@iol.unh.edu> From: Luca Vizzarro In-Reply-To: <20240726141548.688-1-dmarx@iol.unh.edu> 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 Hi Dean, thank you for your work! Some minor comments. On 26/07/2024 15:15, Dean Marx wrote: > add dataclass for passing in flow rule creation arguments, as well as a > __str__ method for converting to a sendable testpmd command. Add > flow_create method to TestPmdShell class for initializing flow rules. > > Signed-off-by: Dean Marx > --- > dts/framework/remote_session/testpmd_shell.py | 58 ++++++++++++++++++- > 1 file changed, 57 insertions(+), 1 deletion(-) > > diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py > index eda6eb320f..d6c111da0a 100644 > --- a/dts/framework/remote_session/testpmd_shell.py > +++ b/dts/framework/remote_session/testpmd_shell.py > @@ -19,7 +19,7 @@ > from dataclasses import dataclass, field > from enum import Flag, auto > from pathlib import PurePath > -from typing import ClassVar > +from typing import ClassVar, Optional > > from typing_extensions import Self, Unpack > > @@ -577,6 +577,43 @@ class TestPmdPortStats(TextParser): > tx_bps: int = field(metadata=TextParser.find_int(r"Tx-bps:\s+(\d+)")) > > > +@dataclass > +class flow_func: Class names should be UpperCamelCase, also should be a suitable name for what it's describing. I believe FlowRule should work. > + """Dataclass for setting flow rule parameters.""" > + > + #: > + port_id: int > + #: > + ingress: bool > + #: > + pattern: str > + #: > + actions: str > + > + #: > + group_id: Optional[int] = None > + #: > + priority_level: Optional[int] = None > + #: > + user_id: Optional[int] = None Optional[..] is an outdated notation. `int | None` is preferred instead. See PEP 604[1]. > + > + def __str__(self) -> str: > + """Returns the string representation of a flow_func instance. > + > + In this case, a properly formatted flow create command that can be sent to testpmd. > + """ > + ret = [] > + ret.append(f"flow create {self.port_id} ") > + ret.append(f"group {self.group_id} " if self.group_id is not None else "") > + ret.append(f"priority {self.priority_level} " if self.priority_level is not None else "") > + ret.append("ingress " if self.ingress else "egress ") > + ret.append(f"user_id {self.user_id} " if self.user_id is not None else "") > + ret.append(f"pattern {self.pattern} ") > + ret.append(" / end actions ") > + ret.append(f"{self.actions} / end") > + return "".join(ret) Using a list with inline conditional appending is not particularly readable. A regular string with conditional appending should do: ret = f"flow create {self.port_id} " if self.group_id is not None:     ret += f"group {self.group_id} " ... Also the latest three append lines can all be in one, if you like the separation you can just do a multi-line string: ret += (     f"pattern {self.pattern} / end "     f"actions {self.actions} / end" ) # or actually this may be just fine: ret += f"pattern {self.pattern} / end " ret += f"actions {self.actions} / end" I guess the way it's split is more of a game changer. If you really want to use a list (in a way that is similar to what I've described here) then I'd take advantage of it... by omitting leading and trailing whitespaces and then use the join to add them in between: " ".join(ret) > + > + > class TestPmdShell(DPDKShell): > """Testpmd interactive shell. > > @@ -804,6 +841,25 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats: > > return TestPmdPortStats.parse(output) > > + def flow_create(self, cmd: str, verify: bool = True) -> None: Not a comment, but a discussion point. Normally we'd want a function to be read as an action such as: create_flow But I understand this is basically mirroring the command format... I wonder which one would be the best. I am personally inclined in verb first. Maybe others can give their opinion. > + """Creates a flow rule in the testpmd session. > + > + Args: > + cmd: String from flow_func instance to send as a flow rule. > + verify: If :data:`True`, the output of the command is scanned > + to ensure the flow rule was created successfully. > + > + Raises: > + InteractiveCommandExecutionError: If flow rule is invalid. > + """ > + flow_output = self.send_command(cmd) > + if verify: > + if "created" not in flow_output: > + self._logger.debug(f"Failed to create flow rule:\n{flow_output}") > + raise InteractiveCommandExecutionError( > + f"Failed to create flow rule:\n{flow_output}" > + ) > + > def _close(self) -> None: > """Overrides :meth:`~.interactive_shell.close`.""" > self.stop() [1] https://peps.python.org/pep-0604/