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 1795B459FE; Tue, 24 Sep 2024 15:56:56 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A5E7D40274; Tue, 24 Sep 2024 15:56:55 +0200 (CEST) Received: from mail-ej1-f44.google.com (mail-ej1-f44.google.com [209.85.218.44]) by mails.dpdk.org (Postfix) with ESMTP id DE4C7400D5 for ; Tue, 24 Sep 2024 15:56:53 +0200 (CEST) Received: by mail-ej1-f44.google.com with SMTP id a640c23a62f3a-a8d4979b843so705366566b.3 for ; Tue, 24 Sep 2024 06:56:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1727186213; x=1727791013; darn=dpdk.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=168DS7vSfoDlaVFLn/JDRLz5Bk/LAbP2KHQ/nUAMZJs=; b=HhWyU9K88UKS3iBHo4nDffxTyV6IdXunZl6/GZyrBhKGlZA5dauO+M/dUvG5y6GLc0 PK3O3HNci2/w0P0m7Hb53ajUJkW52Ror7UZxInT3MeHCwf1PJWoFnxeXHU8xlq1lUAYo O8fzJjk6xhMCPPCGzgxURAEeJ7h1OWle7oMQfBGvep3mh+SSCsIlA9op0C4rACN2tY/q 6gkTSviioSx2sT/oaM9i0QsjMD/l5g8/nbqIS1bv7fOXaYtAJ0kDVoOfliZb9w+B0J6g gkDUqGIHfPSdHDwnZNi1kREjAmnkErKFgJc4EOxjn0PHT0Rbt+S67etgSI8KBgOgtRmt 3lDg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1727186213; x=1727791013; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=168DS7vSfoDlaVFLn/JDRLz5Bk/LAbP2KHQ/nUAMZJs=; b=EekgXgP+RgaMBRUkj8jQaWbopqC7WVAMRGP0ZYbgXvdKiZkGNStIKF/5OVXmNaOqPw +ZhUeiTYmg8rJwxqPpvLIM1znAFtaS4ud0joppINAaSeR0gczWpTZxeJNkZct4Y2qDlO cAHR5JIImR0V8WgkGr0hSGeSiiP4g+wiCsoHsC6ycRQgzu0cWMe7beYZb7rcBmh2186w gHbwfK3OmUA9vMFRpBMjaIPcyv3KaTMfaM19z2og5oH37/fHPCV9P6fEKQlQpQzZEAan eGRHJF4lgKvGVWgISpC/Ux8XWQcNj1fFq/ypYxPg/8i43zP7Cp/Fxgvr+lAGnVbeNvhC iScA== X-Gm-Message-State: AOJu0YygWTokRhtt+ZtRQ9T4x5E8MVRQDwSlj7DJrSnKifHWpfC3pJUw HybFBs7y6/Gu+o93q6wDcp7a8MkxFXp6/twE3dZ9XOKzLEEM1+pPbHO6iRKXHps= X-Google-Smtp-Source: AGHT+IGmBk5Ljeo2iQrFrIrct/gzcun7afamVCDJFjCadfEOytYGM8Xeq6mgh4W3PBc8kQWOuok12w== X-Received: by 2002:a17:906:7949:b0:a86:b38d:d703 with SMTP id a640c23a62f3a-a90d57793e9mr1412414166b.50.1727186213244; Tue, 24 Sep 2024 06:56:53 -0700 (PDT) Received: from [192.168.200.22] ([84.245.121.62]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-a93930cae4fsm87340266b.137.2024.09.24.06.56.51 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 24 Sep 2024 06:56:52 -0700 (PDT) Message-ID: <52b5faca-799c-4de3-a40e-0422e1cf972b@pantheon.tech> Date: Tue, 24 Sep 2024 15:56:50 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] dts: add VLAN methods to testpmd shell To: Dean Marx , probb@iol.unh.edu, npratte@iol.unh.edu, jspewock@iol.unh.edu, luca.vizzarro@arm.com, yoan.picchi@foss.arm.com, Honnappa.Nagarahalli@arm.com, paul.szczepanek@arm.com Cc: dev@dpdk.org References: <20240911173801.16538-1-dmarx@iol.unh.edu> <20240918194127.11556-1-dmarx@iol.unh.edu> Content-Language: en-US From: =?UTF-8?Q?Juraj_Linke=C5=A1?= In-Reply-To: <20240918194127.11556-1-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 We should bring up the exact names of method used in this patch and talk through them in the call. On 18. 9. 2024 21:41, Dean Marx wrote: > added the following methods to testpmd shell class: Capitalize please. > vlan set filter on/off, rx vlan add/rm, > vlan set strip on/off, tx vlan set/reset, > set promisc/verbose > > Fixes: 61d5bc9bf974 ("dts: add port info command to testpmd shell") Is the fix related to the rest of the changes? It looks like it is. The relation to the other changes should be explained in the commit message as well as what the fix fixes (or in which cases the original implementation didn't work properly). > > Signed-off-by: Dean Marx > --- > dts/framework/remote_session/testpmd_shell.py | 175 +++++++++++++++++- > 1 file changed, 174 insertions(+), 1 deletion(-) > > diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py > index 8c228af39f..5c5e681841 100644 > --- a/dts/framework/remote_session/testpmd_shell.py > +++ b/dts/framework/remote_session/testpmd_shell.py > @@ -102,7 +102,7 @@ def make_parser(cls) -> ParserFn: > r"strip (?Pon|off), " > r"filter (?Pon|off), " > r"extend (?Pon|off), " > - r"qinq strip (?Pon|off)$", > + r"qinq strip (?Pon|off)", > re.MULTILINE, > named=True, > ), > @@ -982,6 +982,179 @@ def set_port_mtu_all(self, mtu: int, verify: bool = True) -> None: > for port in self.ports: > self.set_port_mtu(port.id, mtu, verify) > > + def vlan_filter_set(self, port: int, on: bool, verify: bool = True) -> None: The names should follow the same naming scheme. I think the scheme says it doesn't necessarily have to follow testpmd commands, so I'd say let's name it the same way as the mtu methods - set_vlan_filter. > + """Set vlan filter on. Out of curiosity, what is vlan filter? > + > + Args: > + port: The port number to enable VLAN filter on, should be within 0-32. Where is this 0-32 coming from? And why 32 and not 31? > + on: Sets filter on if :data:`True`, otherwise turns off. I'd use third person and add the port: Enable the filter on `port` if :data:`True`, otherwise disable it. > + verify: If :data:`True`, the output of the command and show port info > + is scanned to verify that vlan filtering was enabled successfully. > + If not, it is considered an error. This is not 100% clear whether this means if False or if not enabled successfully. Looks like this should be updated everywhere. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter > + fails to update. > + """ > + filter_cmd_output = self.send_command(f"vlan set filter {'on' if on else 'off'} {port}") > + if verify: > + vlan_settings = self.show_port_info(port_id=port).vlan_offload > + if on ^ (vlan_settings is not None and VLANOffloadFlag.FILTER in vlan_settings): This is an awesome condition. > + self._logger.debug(f"Failed to set filter on port {port}: \n{filter_cmd_output}") > + raise InteractiveCommandExecutionError( > + f"Testpmd failed to set VLAN filter on port {port}." > + ) We should say whether we're enabling or disabling the filter in both the log and error. Does filter_cmd_output contain this? What about the other commands (every one of them except verbose_output)? > + > + def rx_vlan(self, vlan: int, port: int, add: bool, verify: bool = True) -> None: We have set as the prefix for boolean configs and configs that only set the value, here we probabaly want add_del_rx_vlan. We really should talk about these names in the call. > + """Add specified vlan tag to the filter list on a port. The command doesn't mention the filter. Does this mean the filter needs to be enabled before we can config vlans on the port? > + > + Args: > + vlan: The vlan tag to add, should be within 1-1005, 1-4094 extended. The method mentions extended vlans only in this place. It's not clear when can we use extended vlans (where is it configured or enforced). > + port: The port number to add the tag on, should be within 0-32. > + add: Adds the tag if :data:`True`, otherwise removes tag. removes the tag > + verify: If :data:`True`, the output of the command is scanned to verify that > + the vlan tag was added to the filter list on the specified port. If not, it is > + considered an error.> + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True` and the tag > + is not added. I guess this would also be raised if removal is unsuccessful. > + """ > + rx_output = self.send_command(f"rx_vlan {'add' if add else 'rm'} {vlan} {port}") As Patrick mentioned, the other method has cmd in the name of the variable, it would be useful here as well. > + if verify: > + if ( > + "VLAN-filtering disabled" in rx_output > + or "Invalid vlan_id" in rx_output > + or "Bad arguments" in rx_output > + ): > + self._logger.debug( > + f"Failed to {'add' if add else 'remove'} tag {vlan} port {port}: \n{rx_output}" > + ) > + raise InteractiveCommandExecutionError( > + f"Testpmd failed to {'add' if add else 'remove'} tag {vlan} on port {port}." > + ) > + > + def vlan_strip_set(self, port: int, on: bool, verify: bool = True) -> None: > + """Enable vlan stripping on the specified port. This should say enable or disable. > + > + Args: > + port: The port number to use, should be within 0-32. > + on: If :data:`True`, will turn strip on, otherwise will turn off. Let's not be lazy: turn vlan stripping on > + verify: If :data:`True`, the output of the command and show port info > + is scanned to verify that vlan stripping was enabled on the specified port. > + If not, it is considered an error. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True` and stripping > + fails to update. > + """ > + strip_output = self.send_command(f"vlan set strip {'on' if on else 'off'} {port}") Also add cmd to the var name. > + if verify: > + vlan_settings = self.show_port_info(port_id=port).vlan_offload > + if on ^ (vlan_settings is not None and VLANOffloadFlag.STRIP in vlan_settings): > + self._logger.debug( > + f"Failed to set strip {'on' if on else 'off'} port {port}: \n{strip_output}" > + ) > + raise InteractiveCommandExecutionError( > + f"Testpmd failed to set strip {'on' if on else 'off'} port {port}." > + ) > + > + def tx_vlan_set(self, port: int, vlan: int, verify: bool = True) -> None: > + """Set hardware insertion of vlan tags in packets sent on a port. > + > + Args: > + port: The port number to use, should be within 0-32. > + vlan: The vlan tag to insert, should be within 1-4094. This says 1-4094 right away. Both docstrings should say the same thing. > + verify: If :data:`True`, the output of the command is scanned to verify that > + vlan insertion was enabled on the specified port. If not, it is > + considered an error. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True` and the insertion > + tag is not set. > + """ > + vlan_insert_output = self.send_command(f"tx_vlan set {port} {vlan}") Also add cmd to the var name. > + if verify: > + if ( > + "Please stop port" in vlan_insert_output > + or "Invalid vlan_id" in vlan_insert_output > + or "Invalid port" in vlan_insert_output > + ): > + self._logger.debug( > + f"Failed to set vlan tag {vlan} on port {port}:\n{vlan_insert_output}" > + ) > + raise InteractiveCommandExecutionError( > + f"Testpmd failed to set vlan insertion tag {vlan} on port {port}." > + ) > + > + def tx_vlan_reset(self, port: int, verify: bool = True) -> None: I think this could be merged with the previous method. We only need to add the "on" parameter. Now that I think about it, we should rename on -> enable, as we'll have a clear, unified name regardless of what the actual command uses (on, off, set, reset, etc.). > + """Disable hardware insertion of vlan tags in packets sent on a port. > + > + Args: > + port: The port number to use, should be within 0-32. > + verify: If :data:`True`, the output of the command is scanned to verify that > + vlan insertion was disabled on the specified port. If not, it is > + considered an error. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True` and the insertion > + tag is not reset. > + """ > + vlan_insert_output = self.send_command(f"tx_vlan reset {port}") > + if verify: > + if "Please stop port" in vlan_insert_output or "Invalid port" in vlan_insert_output: > + self._logger.debug( > + f"Failed to reset vlan insertion on port {port}: \n{vlan_insert_output}" > + ) > + raise InteractiveCommandExecutionError( > + f"Testpmd failed to reset vlan insertion on port {port}." > + ) > + > + def set_promisc(self, port: int, on: bool, verify: bool = True) -> None: > + """Turns promiscuous mode on/off for the specified port. In line with previous comments, this should say Enable/disable promiscuous mode on the specified port. > + > + Args: > + port: Port number to use, should be within 0-32. > + on: If :data:`True`, turn promisc mode on, otherwise turn off. > + verify: If :data:`True` an additional command will be sent to verify that promisc mode > + is properly set. Defaults to :data:`True`. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True` and promisc mode > + is not correctly set. > + """ promisc -> promiscuous in the whole docstring > + promisc_output = self.send_command(f"set promisc {port} {'on' if on else 'off'}") > + if verify: > + stats = self.show_port_info(port_id=port) > + if on ^ stats.is_promiscuous_mode_enabled: > + self._logger.debug(f"Failed to set promisc mode on port {port}: \n{promisc_output}") > + raise InteractiveCommandExecutionError( > + f"Testpmd failed to set promisc mode on port {port}." > + ) And in these two: promisc -> promiscuous. > + > + def set_verbose(self, level: int, verify: bool = True) -> None: > + """Set debug verbosity level. > + > + Args: > + level: 0 - silent except for error > + 1 - fully verbose except for Tx packets > + 2 - fully verbose except for Rx packets > + >2 - fully verbose > + verify: If :data:`True` the command output will be scanned to verify that verbose level > + is properly set. Defaults to :data:`True`. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True` and verbose level > + is not correctly set. > + """ > + verbose_output = self.send_command(f"set verbose {level}") Also add cmd to the var name. > + if verify: > + if "Change verbose level" not in verbose_output: > + self._logger.debug(f"Failed to set verbose level to {level}: \n{verbose_output}") > + raise InteractiveCommandExecutionError( > + f"Testpmd failed to set verbose level to {level}." > + ) > + > def _close(self) -> None: > """Overrides :meth:`~.interactive_shell.close`.""" > self.stop()