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 AM Dean Marx <dm...@iol.unh.edu> 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
>
> Signed-off-by: Dean Marx <dm...@iol.unh.edu>
> ---
> 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/framework/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) ->
> TestPmdPortStats:
>
> return TestPmdPortStats.parse(output)
>
<snip>
> + def rx_vlan_add(self, vlan: int, port: int, verify: bool = 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
> extended.
> + port: The port number to add the tag on, should be within 0-32.
> + 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.
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 = 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 port
> {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 = 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 within
> 0-32.
> + verify: If :data:`True`, the output of the command is scanned to
> verify that
> + the vlan tag was removed from 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 removed.
Same thing here.
> + """
> + vlan_rm_output = self.send_command(f"rx_vlan rm {vlan} {port}")
> + if verify:
> + if "VLAN-filtering disabled" in vlan_rm_output or "Invalid
> vlan_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}.")
> +
<snip>
> + def tx_vlan_set(self, port: int, vlan: int, verify: bool = 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 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.
This should probably also be indented.
> + """
> + vlan_insert_output = self.send_command(f"tx_vlan set {port} {vlan}")
> + 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 insertion tag {vlan}
> on port {port}: \n{vlan_insert_output}")
> + raise InteractiveCommandExecutionError(f"Testpmd failed to
> set vlan insertion tag {vlan} on port {port}.")
> +
<snip>
> + def set_promisc(self, port: int, on: bool, verify: bool = 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 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.
This should probably also be indented.
> + """
> + if on:
> + promisc_output = self.send_command(f"set promisc {port} on")
> + else:
> + promisc_output = self.send_command(f"set promisc {port} off")
You can inline this if-else-statement to shorten it up. Something like this:
promisc_output = 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 = 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 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}")
> + 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.send_command("quit", "")
> --
> 2.44.0
>
