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 06717460EB; Wed, 22 Jan 2025 16:11:55 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EB94C402D5; Wed, 22 Jan 2025 16:11:54 +0100 (CET) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id A1150402BC for ; Wed, 22 Jan 2025 16:11:53 +0100 (CET) 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 8EEC21007; Wed, 22 Jan 2025 07:12:21 -0800 (PST) Received: from [10.1.32.19] (JR4XG4HTQC.cambridge.arm.com [10.1.32.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 0DF8E3F66E; Wed, 22 Jan 2025 07:11:51 -0800 (PST) Message-ID: Date: Wed, 22 Jan 2025 15:11:50 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v6 2/2] dts: add flow create/delete to testpmd shell Content-Language: en-GB To: Dean Marx , probb@iol.unh.edu, npratte@iol.unh.edu, yoan.picchi@foss.arm.com, Honnappa.Nagarahalli@arm.com, paul.szczepanek@arm.com Cc: dev@dpdk.org References: <20241204232246.30339-2-dmarx@iol.unh.edu> <20250121204135.28944-1-dmarx@iol.unh.edu> <20250121204135.28944-2-dmarx@iol.unh.edu> From: Luca Vizzarro In-Reply-To: <20250121204135.28944-2-dmarx@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 Hi Dean, it looks mostly good, just some nits. On 21/01/2025 20:41, Dean Marx wrote: > + def flow_create(self, flow_rule: FlowRule, port_id: int, verify: bool = True) -> int: > + """Creates a flow rule in the testpmd session. > + > + Args: > + flow_rule: :class:`FlowRule` object used for creating testpmd flow rule. > + port_id: Integer representing the port to use. > + verify: If :data:`True`, the output of the command is scanned > + to ensure the flow rule was created successfully. This line should be indented further. > + > + Raises: > + InteractiveCommandExecutionError: If flow rule is invalid. > + > + Returns: > + Id of created flow rule as an integer. There is no reason to specify the type when it's already annotated as part of the function signature. > + """ > + flow_output = self.send_command(f"flow create {port_id} {flow_rule}") > + 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}" > + ) With the check below here, we are already verifying the command execution... as you are effectively testing the same output. Therefore this verification above is redundant. I'd remove it, together with the verify argument. Finally, I'd specify in the description of the docstring that this function by returning the number of the created flow it's implicitly verifying its execution. > + match = re.search(r"#(\d+)", flow_output) > + if match is not None: > + match_str = match.group(1) > + flow_id = int(match_str) > + return flow_id > + else: > + self._logger.debug(f"Failed to create flow rule:\n{flow_output}") > + raise InteractiveCommandExecutionError(f"Failed to create flow rule:\n{flow_output}") > + > + def flow_delete(self, flow_id: int, port_id: int, verify: bool = True) -> None: > + """Deletes the specified flow rule from the testpmd session. > + > + Args: > + flow_id: :class:`FlowRule` id used for deleting testpmd flow rule. I guess it's not really a FlowRule id. Just Flow id. So: Id of the flow to remove. > + port_id: Integer representing the port to use. > + verify: If :data:`True`, the output of the command is scanned > + to ensure the flow rule was deleted successfully. indent