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 612C7454E5; Wed, 26 Jun 2024 20:23:10 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4E97A4027B; Wed, 26 Jun 2024 20:23:10 +0200 (CEST) Received: from mail-pj1-f49.google.com (mail-pj1-f49.google.com [209.85.216.49]) by mails.dpdk.org (Postfix) with ESMTP id 21FDF4021E for ; Wed, 26 Jun 2024 20:23:09 +0200 (CEST) Received: by mail-pj1-f49.google.com with SMTP id 98e67ed59e1d1-2c5362c7c0bso5211947a91.1 for ; Wed, 26 Jun 2024 11:23:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1719426188; x=1720030988; 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=W+08RV4CTFFTPHlXCt+R+bEtwhI88KJhwzK6zj9Z4Us=; b=CeLc8yqq6v6vrBZ4yxvLyTbcgbFpWZ4f8nGzzHEyHg/2dyEG67h+bbPhWnt7UYcPRR bMyNYB508YQCNiIksudq6IUVM6c4U0Cz9wzUFQ/BLadb6bgpbEdbzBCEiJe7hyYM+e6F J0FTr5tbpVQaTSDspJB3neqETlj5ePmBg6ScY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719426188; x=1720030988; 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=W+08RV4CTFFTPHlXCt+R+bEtwhI88KJhwzK6zj9Z4Us=; b=k+BGJ6e7ych71Z8Bo1UthJ8aJCrBL2NRebpbGXBjD8K9u0GKxFooRq9AwuDxKqpZUR WJDtfj0yUHd/SlHMmVYehSQatmx5yAAVL6yb0erx9YCSUSHEd2AcnNFLZx7vHcmuSUpN GprOmdFfskqfdDF88G03p797Q1SG41giYzFX9WU4Zo6fuweyTGj3TBCKT4mkZzwypn4M vqGVDD3ZMhDoniT//XaVceIO+7j/+ev3+Jo5ViCtTcxoaW5nByyVxWFM63Xy9xeqp2gg BmNSX+1mvrMXVH4wS+6dQ4FxHJwCysKTn2QMGxa/cfFBtLA+d63+w5+LYk7y2M49/Q09 /fMQ== X-Forwarded-Encrypted: i=1; AJvYcCVOIY1DkBd20uMcHoJQS32O5dSgrWCiaCc66lGsR8z9rzPmb0gC6/jDjIyiEQh22pGayQaCpHQ2Ezj09lI= X-Gm-Message-State: AOJu0YxmENi/sfaB8ZoqJeaZaT/XCHNAb4MVrSp7Jun9p4yd6hCspv4S nLCE8hwJFEck9TwdevaqVEZm+HXUWc9CWcu1HYuYihz2mcP7c+JgToYSwNnqPBI0UDNwGzKPZ1v eZFPgMmYxKhvAQbCd0fmBE89TQRXP/hgOqq0BDg== X-Google-Smtp-Source: AGHT+IGLAr9w9Zohk2CeMJl24KhgM9wW5Xf966m6fm14C3m5Up5/D2Kjt+ijmi1iZDlLWOradoKMWVfvmezlOWQ79HM= X-Received: by 2002:a17:90a:a407:b0:2c3:7e3:6be0 with SMTP id 98e67ed59e1d1-2c861491190mr10320203a91.31.1719426188036; Wed, 26 Jun 2024 11:23:08 -0700 (PDT) MIME-Version: 1.0 References: <20240614150238.26374-1-dmarx@iol.unh.edu> <20240625153324.27257-1-dmarx@iol.unh.edu> <20240625153324.27257-2-dmarx@iol.unh.edu> In-Reply-To: <20240625153324.27257-2-dmarx@iol.unh.edu> From: Jeremy Spewock Date: Wed, 26 Jun 2024 14:22:56 -0400 Message-ID: Subject: Re: [PATCH v7 2/3] dts: add VLAN methods to testpmd shell To: Dean Marx Cc: Honnappa.Nagarahalli@arm.com, juraj.linkes@pantheon.tech, probb@iol.unh.edu, paul.szczepanek@arm.com, yoan.picchi@foss.arm.com, bruce.richardson@intel.com, luca.vizzarro@arm.com, dev@dpdk.org 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 The functionality all looks good to me, just a few documentation nit-picks and a suggestion for something that I think could shorten up one function. On Tue, Jun 25, 2024 at 11:34=E2=80=AFAM Dean Marx wrot= e: > > added the following methods to testpmd shell class: > vlan set filter on/off, rx vlan add/rm, > vlan set strip on/off, port stop/start all/port, > tx vlan set/reset, set promisc/verbose > > Signed-off-by: Dean Marx > --- > dts/framework/remote_session/testpmd_shell.py | 278 ++++++++++++++++++ > 1 file changed, 278 insertions(+) > > diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framewor= k/remote_session/testpmd_shell.py > index ec22f72221..d504e92a45 100644 > --- a/dts/framework/remote_session/testpmd_shell.py > +++ b/dts/framework/remote_session/testpmd_shell.py > @@ -806,6 +806,284 @@ def show_port_stats(self, port_id: int) -> TestPmdP= ortStats: > > return TestPmdPortStats.parse(output) > > + def rx_vlan_add(self, vlan: int, port: int, verify: bool =3D True): > + """Add specified vlan tag to the filter list on a port. > + > + Args: > + vlan: The vlan tag to add, should be within 1-1005, 1-4094 e= xtended. > + port: The port number to add the tag on, should be within 0-= 32. > + verify: If :data:`True`, the output of the command is scanne= d to verify that > + the vlan tag was added to the filter list on the specifi= ed port. If not, it is > + considered an error. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True= ` and the tag > + is not added. Just a little documentation nit-pick, I noticed this second line is indented in most other methods, we should probably keep this consistent. > + """ > + vlan_add_output =3D self.send_command(f"rx_vlan add {vlan} {port= }") > + if verify: > + if "VLAN-filtering disabled" in vlan_add_output or "Invalid = vlan_id" in vlan_add_output: > + self._logger.debug(f"Failed to add vlan tag {vlan} on po= rt {port}: \n{vlan_add_output}") > + raise InteractiveCommandExecutionError(f"Testpmd failed = to add vlan tag {vlan} on port {port}.") > + > + def rx_vlan_rm(self, vlan: int, port: int, verify: bool =3D True): > + """Remove specified vlan tag from filter list on a port. > + > + Args: > + vlan: The vlan tag to remove, should be within 1-4094. > + port: The port number to remove the tag from, should be with= in 0-32. > + verify: If :data:`True`, the output of the command is scanne= d to verify that > + the vlan tag was removed from the filter list on the spe= cified port. If not, it is > + considered an error. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True= ` and the tag > + is not removed. Same thing here. > + """ > + vlan_rm_output =3D self.send_command(f"rx_vlan rm {vlan} {port}"= ) > + if verify: > + if "VLAN-filtering disabled" in vlan_rm_output or "Invalid v= lan_id" in vlan_rm_output: > + self._logger.debug(f"Failed to remove vlan tag {vlan} on= port {port}: \n{vlan_rm_output}") > + raise InteractiveCommandExecutionError(f"Testpmd failed = to remove vlan tag {vlan} on port {port}.") > + > + def tx_vlan_set(self, port: int, vlan: int, verify: bool =3D True): > + """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. > + verify: If :data:`True`, the output of the command is scanne= d 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. This should probably also be indented. > + """ > + vlan_insert_output =3D self.send_command(f"tx_vlan set {port} {v= lan}") > + if verify: > + if ("Please stop port" in vlan_insert_output or "Invalid vla= n_id" in vlan_insert_output > + or "Invalid port" in vlan_insert_output): > + self._logger.debug(f"Failed to set vlan insertion tag {v= lan} on port {port}: \n{vlan_insert_output}") > + raise InteractiveCommandExecutionError(f"Testpmd failed = to set vlan insertion tag {vlan} on port {port}.") > + > + def set_promisc(self, port: int, on: bool, verify: bool =3D True): > + """Turns promiscuous mode on/off for the specified port > + > + Args: > + port: Port number to use, should be within 0-32. > + on: If :data:`True`, turn promisc mode on, otherwise turn of= f. > + verify: If :data:`True` an additional command will be sent t= o 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. This should probably also be indented. > + """ > + if on: > + promisc_output =3D self.send_command(f"set promisc {port} on= ") > + else: > + promisc_output =3D self.send_command(f"set promisc {port} of= f") You can inline this if-else-statement to shorten it up. Something like this= : promisc_output =3D self.send_command(f"set promisc {port} {'on' if on else 'off''}") > + if verify: > + if (on and "Promiscuous mode: enabled" not in > + self.send_command(f"show port info {port}")): > + 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}.") > + elif (not on and "Promiscuous mode: disabled" not in > + self.send_command(f"show port info {port}")): > + 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}.") > + I actually mentioned this on Nick's patch, but I think this could be shortened. I'll copy what I said there: The logger output and the error seem to be exactly the same here so we should avoid duplicating them. There are a few ways to go about combining these two cases in the conditional but I would probably just use an f-string to conditionally look for "enabled" vs "disabled". I wonder if this check would be easier using the dataclass for port info since it will be a boolean inside of the dataclass rather than just searching for a string. I think if you had a boolean for if promisc mode was on you could use an XOR of add and is_promisc_mode and it would have the same effect. Normally I avoid using the dataclass if the check is simple without it just because I think it is slightly faster that way, but if there is a good use-case for it (like there is here) then I think we might as well use it. I think using the testpmd.show_port_info method would make for a cleaner approach, so I slightly favor that one. > + > + def set_verbose(self, level: int, verify: bool =3D True): > + """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 t= o 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 =3D self.send_command(f"set verbose {level}") > + if verify: > + if "Change verbose level" not in verbose_output: > + self._logger.debug(f"Failed to set verbose level to {lev= el}: \n{verbose_output}") > + raise InteractiveCommandExecutionError(f"Testpmd failed = to set verbose level to {level}.") > + > def close(self) -> None: > """Overrides :meth:`~.interactive_shell.close`.""" > self.send_command("quit", "") > -- > 2.44.0 >