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 5C2D4454BF; Fri, 21 Jun 2024 22:51:06 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E9DC44027D; Fri, 21 Jun 2024 22:51:05 +0200 (CEST) Received: from mail-pj1-f51.google.com (mail-pj1-f51.google.com [209.85.216.51]) by mails.dpdk.org (Postfix) with ESMTP id 3A81840041 for ; Fri, 21 Jun 2024 22:51:04 +0200 (CEST) Received: by mail-pj1-f51.google.com with SMTP id 98e67ed59e1d1-2c825f0b381so727937a91.1 for ; Fri, 21 Jun 2024 13:51:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1719003063; x=1719607863; 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=jERG1EYnXPfv8ICLBKnwam3zhekOMiD/x7vkRN+3sI0=; b=LENCkpux/cEn6BTrN+vgfd3obyhBFTmIu9Xk0os33mXTIMN7ZfkYOX0th+ew1nbWdQ 0Y1hcgsPu/RSmapYTgNdBcqkh8ebc48ZW/qA/XvqPgbGLF4Vxjr1OdqkLYp6tTjgfyub H3Zz2wm2BXC/WHgjWJ41NMKzA9O2qpADMvkwU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719003063; x=1719607863; 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=jERG1EYnXPfv8ICLBKnwam3zhekOMiD/x7vkRN+3sI0=; b=NSvCHhE7tfIgiGN1lLj/My5l6cmzAHUkTcegPmHBlVVhpVcgnuYF2TGBgC6qpO5IH8 b3khH+qZtHwgdbJwzsIlgfV0MJJcS5GWk7g5xCM1pPvyNISki55nuWG7U5fzwhUO+Hu6 h4M1rXLq0tdmvZz775pkS1LmsmG2EByF7l8US8SgivLCtknF43nN4OPAwAxyibtW4Okv 5zCGBnUqn/d4OY1XLYg8kJlaDK9gkpgAH8EcwsbBYuESZGosD8/mKaD8BZAhMGYD8QrF 2FWI1bD0tCaxQLctMhokz1QvCrO/U3/ZNxmNL0N9iivdYbIqngnpxc10fZPVbdzRuOV3 LNkg== X-Forwarded-Encrypted: i=1; AJvYcCXL75k7kd9B40lsJuMMNdWOmNESRS/WtgySe2bWAY3q2XnfNb709DymE3EMLIjZCwLaZZ90y5jx8g8ZNvM= X-Gm-Message-State: AOJu0Yy0eY5ia9rAX8JiEcVNarBceMEtqmriUvkVpf0nN86fi5WifCKx 5mQOQSmaLCJYcJAbDS8R+UjvJpfTNJtamUKOuvq4GpOHvlWClJf+d2ruPAWS4zDHcO3HBCoiplC rKSV047byzv/3RtRQgiLmX1ZCxUPk4DzbvTrneA== X-Google-Smtp-Source: AGHT+IEaJ442pgQu6ah0YQAHlwMVIEaKvy3I0dR1OTiiPKQ3jFcY0zStQU/OFngI6BgxFrIN8aJ3/kuhd7h8XGTDzko= X-Received: by 2002:a17:90b:3542:b0:2c6:dc3b:d6fd with SMTP id 98e67ed59e1d1-2c7b5da56e6mr9502076a91.31.1719003063030; Fri, 21 Jun 2024 13:51:03 -0700 (PDT) MIME-Version: 1.0 References: <20240614150238.26374-1-dmarx@iol.unh.edu> <20240618162939.23339-1-dmarx@iol.unh.edu> In-Reply-To: <20240618162939.23339-1-dmarx@iol.unh.edu> From: Jeremy Spewock Date: Fri, 21 Jun 2024 16:50:51 -0400 Message-ID: Subject: Re: [PATCH v5 1/3] dts: updated testpmd shell class 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 Just documentation comments, the structure of the code and the functionality look good to me. Maybe a different subject for the commit would be more descriptive. Something like "add methods required for VLAN test suite to TestpmdShell". Then you can explain what methods are added in the description. On Tue, Jun 18, 2024 at 12:30=E2=80=AFPM Dean Marx wrot= e: > > Ported over the promisc and verbose mode functions > from v2 of the queue start/stop suite to use for the > VLAN suite. Tweaked some of the verification methods to be > more concise, changed some docstrings to be more specific. The descriptions here should be the commit message explaining everything that is being added, not just the diff from the previous version. This is generally because this will end up being the description of the commit once the final version gets merged, so the people looking at the git history will only see the description of the final version and have no context for what was added in the previous versions. The same for the subject line of the commit. > > Signed-off-by: Dean Marx > --- If you want to have a change log associated with the commit however, you can put comments under these 3 hyphens that won't get included in the final commit message. > dts/framework/remote_session/testpmd_shell.py | 260 ++++++++++++++++++ > 1 file changed, 260 insertions(+) > > diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framewor= k/remote_session/testpmd_shell.py > index cb2ab6bd00..aad3a3a448 100644 > --- a/dts/framework/remote_session/testpmd_shell.py > +++ b/dts/framework/remote_session/testpmd_shell.py > @@ -225,6 +225,266 @@ def set_forward_mode(self, mode: TestPmdForwardingM= odes, verify: bool =3D True): > f"Test pmd failed to set fwd mode to {mode.value}" > ) > > + def vlan_filter_set_on(self, port: int, verify: bool =3D True): > + """Set vlan filter on. > + > + Args: > + port: The port number to use, should be within 0-32. A comment on this doc-string and the one in the other vlan filter method: This might be a little more descriptive if you explained how the port number is being used. Maybe saying something like "number of the port to add the filter to". Or, alternatively, you could change the first line of this comment to say "Enable VLAN filtering for port with id `port`". If you see the code it's clear how this port is being used, but if you were only looking at the documentation there isn't much information about how this port number comes into play. > + verify: If :data:`True`, the output of the command is scanne= d to verify that > + vlan filtering was enabled successfully. If not, it is > + considered an error. This looks like a case where an additional command is sent rather than the original output being scanned after the update. > + > + 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 {po= rt}") > + if verify: > + if "Invalid port" in filter_cmd_output or "filter: on" not i= n self.send_command(f"show port info {port}"): > + self._logger.debug(f"Failed to enable vlan filter on por= t {port}: \n{filter_cmd_output}") > + raise InteractiveCommandExecutionError(f"Testpmd failed = to enable vlan filter on port {port}.") > + > + def vlan_filter_set_off(self, port: int, verify: bool =3D True): > + """Set vlan filter off. > + > + 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 filtering was disabled successfully. If not, it is > + considered an error. Here it seems like it's not the output of the command being sent, but another command being sent to verify. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True= ` and the filter > + fails to update. > + """ > + def vlan_strip_set_on(self, port: int, verify: bool =3D True): > + """Enable vlan stripping on the specified 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 stripping was enabled on the specified port. If not= , it is > + considered an error. Same thing here. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True= ` and stripping > + fails to update. > + """ > + vlan_strip_output =3D self.send_command(f"vlan set strip on {por= t}") > + if verify: > + if "strip on" not in self.send_command(f"show port info {por= t}"): > + self._logger.debug(f"Failed to set vlan filter on for po= rt {port}: \n{vlan_strip_output}") > + raise InteractiveCommandExecutionError(f"Testpmd failed = to set vlan filter on for port {port}.") > + > + def vlan_strip_set_off(self, port: int, verify: bool =3D True): > + """Disable vlan stripping on the specified port. > + > + Args: > + port: The port number to use, should be within 0-32 This arg is missing a period at the end of the line. > + verify: If :data:`True`, the output of the command is scanne= d to verify that > + vlan stripping was disabled on the specified port. If no= t, it is > + considered an error. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True= ` and stripping > + fails to update. > + """ > + vlan_strip_output =3D self.send_command(f"vlan set strip off {po= rt}") > + if verify: > + if "strip off" not in self.send_command(f"show port info {po= rt}"): > + self._logger.debug(f"Failed to disable vlan stripping on= port {port}: \n{vlan_strip_output}") > + raise InteractiveCommandExecutionError(f"Testpmd failed = to disable vlan stripping on port {port}.") > + > + def port_stop_all(self, verify: bool =3D True): > + """Stop all ports. > + This doc-string is missing the Args section for the verify parameter. > + 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_stop(self, port: int, verify: bool =3D True): > + """Stop all ports. This seems like a copy paste error from the previous method. > + > + Args: > + port: specifies the port number to use, must be between 0-32 verify is missing from this Args section. There is also a missing period at the end of the line. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True= ` and the port > + is not stopped.""" > + port_output =3D self.send_command(f"port stop {port}") > + if verify: > + if "Done" not in port_output: > + self._logger.debug(f"Failed to stop port {port}: \n{port= _output}") > + raise InteractiveCommandExecutionError(f"Testpmd failed = to stop port {port}.") > + > + def port_start_all(self, verify: bool =3D True): > + """Start all ports. > + This method is also missing an Args section. > + 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 port_start(self, port: int, verify: bool =3D True): > + """Stop all ports. Seems like a copy-paste error here. > + > + Args: > + port: specifies the port number to use, must be between 0-32 Missing period at the end of the line here. The args section also seems to be missing the verify parameter. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True= ` and the port > + is not started.""" > + def set_promisc(self, port: int, on: bool, verify: bool =3D True): > + """Turns promiscuous mode on/off for the specified port Missing period at the end of the line here. > + > + 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: > + > + 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 These make sense, but it might help to indent the last 3 just so it's clear what they apply to. > + verify: if :data:`True` an additional command will be sent t= o verify that verbose level > + is properly set. Defaults to :data:`True`. This method doesn't seem to send another command, it just verifies from the output of the first. > + > + 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 >