Re: [PATCH v2 2/3] dts: added promisc/verbose func to testpmd shell

[Jeremy Spewock <jspewock@iol.unh.edu>]

On Mon, Jun 17, 2024 at 3:48 PM Dean Marx <dmarx@iol.unh.edu> wrote:
>
> Setting the verbose mode and promiscuous mode is a common
> command in test suites, these additions will allow the dev
> to set both modes from the testpmd shell module.

It looks like you're also adding the ability to start and stop ports
here as well, we should add that to the commit body and subject.

>
> Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
> ---
>  dts/framework/remote_session/testpmd_shell.py | 179 +++++++++++++++++-
>  1 file changed, 178 insertions(+), 1 deletion(-)
>
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index cb2ab6bd00..425f3ec220 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -147,7 +147,7 @@ def start(self, verify: bool = True) -> None:
>                          "Not all ports came up after starting packet forwarding in testpmd."
>                      )
>
> -    def stop(self, verify: bool = True) -> None:
> +    def stop(self, verify: bool = True) -> str:
>          """Stop packet forwarding.
>
>          Args:
> @@ -155,6 +155,9 @@ def stop(self, verify: bool = True) -> None:
>                  forwarding was stopped successfully or not started. If neither is found, it is
>                  considered an error.
>
> +        Returns:
> +            Output gathered from sending the stop command.
> +
>          Raises:
>              InteractiveCommandExecutionError: If `verify` is :data:`True` and the command to stop
>                  forwarding results in an error.
> @@ -167,6 +170,7 @@ def stop(self, verify: bool = True) -> None:
>              ):
>                  self._logger.debug(f"Failed to stop packet forwarding: \n{stop_cmd_output}")
>                  raise InteractiveCommandExecutionError("Testpmd failed to stop packet forwarding.")
> +        return stop_cmd_output

I know these are changes coming from us both using the same port start
and stop methods, but you don't seem to use the changes to the stop
method so we should probably omit them.

>
>      def get_devices(self) -> list[TestPmdDevice]:
>          """Get a list of device names that are known to testpmd.
<snip>
> +
> +    def setup_port_queue(self, port_id: int, queue_id: int, is_rx_queue: bool) -> None:
> +        """Setup a given queue on a port.
> +
> +        This functionality cannot be verified because the setup action only takes effect when the
> +        queue is started.
> +
> +        Args:
> +            port_id: ID of the port where the queue resides.
> +            queue_id: ID of the queue to setup.
> +            is_rx_queue: Type of queue to setup. If :data:`True` an RX queue will be setup,
> +                otherwise a TX queue will be setup.
> +        """
> +        self.send_command(f"port {port_id} {'rxq' if is_rx_queue else 'txq'} {queue_id} setup")
> +
> +    def change_queue_ring_size(
> +            self,
> +            port_id: int,
> +            queue_id: int,
> +            size: int,
> +            is_rx_queue: bool,
> +            verify: bool = True,
> +        ) -> None:
> +            """Update the ring size of an RX/TX queue on a given port.
> +
> +            Args:
> +                port_id: The port that the queue resides on.
> +                queue_id: The ID of the queue on the port.
> +                size: The size to update the ring size to.
> +                is_rx_queue: Whether to modify an RX or TX queue. If :data:`True` an RX queue will be
> +                    updated, otherwise a TX queue will be updated.
> +                verify: If :data:`True` an additional command will be sent to check the ring size of
> +                    the queue in an attempt to validate that the size was changes properly.
> +
> +            Raises:
> +                InteractiveCommandExecutionError: If `verify` is :data:`True` and there is a failure
> +                    when updating ring size.
> +            """
> +            queue_type = "rxq" if is_rx_queue else "txq"
> +            self.send_command(f"port config {port_id} {queue_type} {queue_id} ring_size {size}")
> +            self.setup_port_queue(port_id, queue_id, is_rx_queue)
> +            if verify:
> +                queue_info = self.send_command(f"show {queue_type} info {port_id} {queue_id}")
> +                if f"Number of RXDs: {size}" not in queue_info:
> +                    self._logger.debug(
> +                        f"Failed up update ring size of queue {queue_id} on port {port_id}:"
> +                        f"\n{queue_info}"
> +                    )
> +                    raise InteractiveCommandExecutionError(
> +                        f"Failed to update ring size of queue {queue_id} on port {port_id}"
> +                    )

Same thing here, it doesn't seem like you use this method for changing
the ring size or the previous for setting up ports, it's probably
better to omit them.

> +
<snip>

>      def close(self) -> None:
>          """Overrides :meth:`~.interactive_shell.close`."""
>          self.send_command("quit", "")
> --
> 2.44.0
>