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 38749455CE; Tue, 9 Jul 2024 23:22:49 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 14278427E1; Tue, 9 Jul 2024 23:22:49 +0200 (CEST) Received: from mail-pf1-f178.google.com (mail-pf1-f178.google.com [209.85.210.178]) by mails.dpdk.org (Postfix) with ESMTP id C2FD3427DF for ; Tue, 9 Jul 2024 23:22:46 +0200 (CEST) Received: by mail-pf1-f178.google.com with SMTP id d2e1a72fcca58-70b4267ccfcso1440959b3a.3 for ; Tue, 09 Jul 2024 14:22:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1720560166; x=1721164966; 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=YvZoNtuWDf2ODZwPnDQx9+QwPYj0I2157H48f9q9XIc=; b=f/fFKxnWmoMEkZ2ikzhKyoMhpkoMnKrR0uPOcF/cuviPF859mOEs6sddyTkcb2lq3X 7QIrNkn8tVhrlpV12RQlSzR49SMJqFs7VEb6jR3vb2fBSIjPOBZslLlEAgmHCtFi5ozg hKY+DLaPj/I2C2qx/VUdVciQlt+sf8rzV5vWw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1720560166; x=1721164966; 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=YvZoNtuWDf2ODZwPnDQx9+QwPYj0I2157H48f9q9XIc=; b=bDUWR5lBxQY04UbzeMy+vJijxqXEWaxK9wPutVZsTiGD4Yt4b/TIB0O6p0VALF0JQo kUBVQvvtKAY4eKCcoXCNRu/98PHVurRQwK3wxXUStC0xC7RYj6RpviYvQC1WyWmYaow4 F74NudPt+ZwwTLFM4e8PLBcxYtTaBQT1ZOxhTh8NOARBnMrLC2vbZX+TUXm9Eatcb/zf KG2BSnRlghYBoNc/mWANEjzNJ/wjRxzaK+1Q0eM+E+1AnuJRJFkbcsPrrWf9aFJxAChA I4yA0eXXTktjt8py+O4mVXMtQKCxX9RjCRJw6a/wzBuzuu0QsnXWto3mWnGXDMq+4Cmg ZWfQ== X-Forwarded-Encrypted: i=1; AJvYcCVTUkj9uLGO0SEvcjWXu7JS/9tAmiRDWIEWjeOMCLNGheG3Si9+qReAwqKaWCkxDKZ2QamSX1DIP9v7yYo= X-Gm-Message-State: AOJu0YzXqVRxZ2WWhWJ5wfAp0TqT2BpcH00XArbpz5qNOtgeoUIWZBSd mvtDNM2Op1P+GTYhLrsx3oHwGZeqUgwMkj7Zcp9vDJfUIsk5bBZfX2l/jFb7m0/p5tljAbnAGqv rwfgckTi8xlUFINQ/Xk53lxdK96d8C3RPCwu6kA== X-Google-Smtp-Source: AGHT+IF5LREYUE8sNZfBAmURNhqGtZuv2hdolHqQhA3hqo6nO5QpQx7+I92nLaqtemLvEECu3s4KkKhZG3gkWQzBPR8= X-Received: by 2002:a05:6a00:1d92:b0:70a:a769:196d with SMTP id d2e1a72fcca58-70b43679cb4mr4106256b3a.34.1720560165734; Tue, 09 Jul 2024 14:22:45 -0700 (PDT) MIME-Version: 1.0 References: <20240614150238.26374-1-dmarx@iol.unh.edu> <20240703164703.16809-1-dmarx@iol.unh.edu> <20240703164703.16809-3-dmarx@iol.unh.edu> In-Reply-To: <20240703164703.16809-3-dmarx@iol.unh.edu> From: Jeremy Spewock Date: Tue, 9 Jul 2024 17:22:33 -0400 Message-ID: Subject: Re: [PATCH v9 3/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 Hey Dean, one other thing that I just noticed which I should have picked up on sooner is that all of the methods that you added here are missing return type annotations. I believe these are generally things we look for on all methods, and I'm surprised that we don't have something that checks for this in the formatting script. Maybe that would be a good thing to add. Obviously the methods don't return anything so it doesn't harm much in the way of type-checking, but it is generally better to be more explicit and mark that the methods actually don't return anything. On Wed, Jul 3, 2024 at 12:47=E2=80=AFPM Dean Marx wrote= : > > 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 > fixed: bug in vlan_offload for > show port info, removed $ at end of regex > > Signed-off-by: Dean Marx > --- > dts/framework/remote_session/testpmd_shell.py | 245 +++++++++++++++++- > 1 file changed, 244 insertions(+), 1 deletion(-) > > diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framewor= k/remote_session/testpmd_shell.py > index ec22f72221..09d3bda5d6 100644 > --- a/dts/framework/remote_session/testpmd_shell.py > +++ b/dts/framework/remote_session/testpmd_shell.py > @@ -98,7 +98,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=3DTrue, > ), > @@ -806,6 +806,249 @@ def show_port_stats(self, port_id: int) -> TestPmdP= ortStats: > > return TestPmdPortStats.parse(output) > > + def vlan_filter_set(self, port: int, on: bool, verify: bool =3D True= ): > + """Set vlan filter on. With that change I was thinking that, while it isn't a big enough deal to justify a new version on its own, maybe this subject line of the doc-string could be expanded on. Just because it saying that it sets VLAN filtering on might be a little vague on its own. It makes more sense once you see that one of the parameters is a port, but maybe even just adding to this that it "Enables/Disables VLAN filtering on a port" might be more clear that it is on one port and not all of them and things like that. > + > + Args: > + port: The port number to enable VLAN filter on, should be wi= thin 0-32. > + on: Sets filter on if :data:`True`, otherwise turns off. This feels like it's missing a few words that would make it flow better, particularly "otherwise turns off" seems off to me. Maybe something like "if :data:`True` turn filtering on, otherwise turn it off" could work. > + verify: If :data:`True`, the output of the command and show = port info > + is scanned to verify that vlan filtering was enabled suc= cessfully. > + If not, it is considered an error. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True= ` and the filter > + fails to update. > + """ > + filter_cmd_output =3D self.send_command(f"vlan set filter {'on' = if on else 'off'} {port}") > + if verify: > + if on ^ ("FILTER" in str(self.show_port_info(port_id=3Dport)= .vlan_offload)): I was wondering how Flags handle being converted into a string and it seems like they just sort of put all of the other Flags they were combined with into a string separated by "|". Just to help keep things more stable in the case of things being renamed or anything, you can also do this same "in" comparison using the flag for filtering itself. So, for example, `VLANOffloadFlag.FILTER in self.show_port_info(port_id=3Dport).vlan_offload` > + 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}." > + ) > + > + def rx_vlan(self, vlan: int, port: int, add: bool, verify: bool =3D = True): > + """Add specified vlan tag to the filter list on a port. Since there is a parameter for whether or not we are adding or removing, this doc-string should likely reflect that rather than just saying it adds a specified VLAN. > + > + 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. > + add: adds the tag if :data:`True`, otherwise removes tag. Probably better to say "removes the tag" here. > + 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. > + """ > + rx_output =3D self.send_command(f"rx_vlan {'add' if add else 'rm= '} {vlan} {port}") > + 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 =3D True)= : > + """Enable vlan stripping on the specified port. This subject line should also reflect that it can be enabled or disabled in the method. > + > + Args: > + port: The port number to use, should be within 0-32. > + on: If :data:`True`, will turn strip on, otherwise will turn= off. > + 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 =3D self.send_command(f"vlan set strip {'on' if on = else 'off'} {port}") > + if verify: > + if on ^ ("STRIP" in str(self.show_port_info(port_id=3Dport).= vlan_offload)):\ Same thing here as above, it might be better to compare the Flags here rather than strings. > + 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 port_stop_all(self, verify: bool =3D True): > + """Stop all ports. > + > + Args: > + port: Specifies the port number to use, must be between 0-32= . This looks like it isn't included in the parameters anymore so it's probably better to remove this part of the doc-string as well. > + verify: If :data:`True`, the output of the command is scanne= d > + to ensure all ports are stopped. If not, it is considere= d an error. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True= ` and all ports > + fail to stop. > + """ > + port_output =3D self.send_command("port stop all") > + if verify: > + if "Done" not in port_output: > + self._logger.debug(f"Failed to stop all ports: \n{port_o= utput}") > + raise InteractiveCommandExecutionError("Testpmd failed t= o stop all ports.") > + > + > + def port_start_all(self, verify: bool =3D True): > + """Start all ports. > + > + Args: > + port: Specifies the port number to use, must be between 0-32= . Same here. > + verify: If :data:`True`, the output of the command is scanne= d > + to ensure all ports are started. If not, it is considere= d an error. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True= ` and all ports > + fail to start. > + """ > + port_output =3D self.send_command("port start all") > + if verify: > + if "Done" not in port_output: > + self._logger.debug(f"Failed to start all ports: \n{port_= output}") > + raise InteractiveCommandExecutionError("Testpmd failed t= o start all ports.") > + > + def tx_vlan_reset(self, port: int, verify: bool =3D True): > + """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 scanne= d to verify that > + vlan insertion was disabled on the specified port. If no= t, it is > + considered an error. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True= ` and the insertion > + tag is not reset. > + """ > + vlan_insert_output =3D self.send_command(f"tx_vlan set {port}") I think the testpmd command to disable hardware insertion is actually `tx_vlan reset {port_id}`, when I ran this command I was getting errors. > + 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 {po= rt}." > + ) > + > 2.44.0 >