From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
	by inbox.dpdk.org (Postfix) with ESMTP id DAA28455D5;
	Wed, 10 Jul 2024 18:44:37 +0200 (CEST)
Received: from mails.dpdk.org (localhost [127.0.0.1])
	by mails.dpdk.org (Postfix) with ESMTP id C882B40E0A;
	Wed, 10 Jul 2024 18:44:37 +0200 (CEST)
Received: from mail-pf1-f170.google.com (mail-pf1-f170.google.com
 [209.85.210.170])
 by mails.dpdk.org (Postfix) with ESMTP id 97151402C9
 for <dev@dpdk.org>; Wed, 10 Jul 2024 18:44:36 +0200 (CEST)
Received: by mail-pf1-f170.google.com with SMTP id
 d2e1a72fcca58-70af22a9c19so4789958b3a.2
 for <dev@dpdk.org>; Wed, 10 Jul 2024 09:44:36 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=iol.unh.edu; s=unh-iol; t=1720629875; x=1721234675; 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=UvnqtdbTJigtUKw15aGOGrRJ1FJvi7WoTe1/XamMEUo=;
 b=SzkM6iF/zVG5lj8giBiOm9cgluADsw/4I70G4hRzhhmGp288uALkkreJCr6Kquzyoh
 7ps2t3WRY92cKYEvDvrr5tihbIDyYb68c7oZ6CLcOBsQtMeKlC1iEYwvH41WqYJhJl/9
 ZXj0rB2zmqngipXZmQAFoMJK7hNEn+x1Px5dc=
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=1e100.net; s=20230601; t=1720629875; x=1721234675;
 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=UvnqtdbTJigtUKw15aGOGrRJ1FJvi7WoTe1/XamMEUo=;
 b=pI7GMQBbXaqvtWMlFPXIdgfgxHjHesPJbwhnLE3anrj3je+HjvPTSmvg11AsuMxI1z
 +k9fwth42qWEZYX/995wSIPpd5xeDbNiPEgCB9kAbXvM0iXKmD6sj1J2NXgtpCGLVHHv
 1y3aS6FgopV3IKoziBEdpzry0vLhoFCrRKiTLaGZl0awg9IZJ5egiddFC1OJUwcCPuFY
 cocV+fd2whmDVPyxRbJ5gWrvsMg8dyByg2W5f4Ai3wQyVOO9UMkF5u23yCCwqkJHdvJm
 JgkRh6Et1iM5gSeNsUrHZdA+7wdC1vsAzlaiVl5oocSWihZxgba7wBvsqIc2DnuLU3ee
 pk1g==
X-Forwarded-Encrypted: i=1;
 AJvYcCUm1OtlsMt2X+QJo0Vn0gUs5r+Un2x06w3I2IdPG4QP+WUj2xYGsANw7LQe6yVlTvd5xfVQDr2KK4di2XQ=
X-Gm-Message-State: AOJu0Ywk1F4e9KQ3uaVCJW5KAJwtTcukH5c7anGcBctxRq6F8jIlR06V
 UQa4oK3mICkVUFbEkBxJXm64ACpk0s7a9lxvxujemlexBi3IdC4ggmHh4kIo6xWGxdyaIsgQZgy
 zQZxE7fZ23Cfk8c+XqCY0NLdCEpfr4feQhHftDQ==
X-Google-Smtp-Source: AGHT+IFg/nDjR0z+/OCXSOSnvuVb1YIcZPDP3uYPQ5kTBdaCIgC3TEF+EejXooIJnUWzm4JG3j34mJi0LYWjH/7PmWI=
X-Received: by 2002:a05:6a00:1ca2:b0:70b:17df:cbc1 with SMTP id
 d2e1a72fcca58-70b4361b82amr6572944b3a.30.1720629875529; Wed, 10 Jul 2024
 09:44:35 -0700 (PDT)
MIME-Version: 1.0
References: <20240708191938.32132-1-dmarx@iol.unh.edu>
 <20240708191938.32132-2-dmarx@iol.unh.edu>
In-Reply-To: <20240708191938.32132-2-dmarx@iol.unh.edu>
From: Jeremy Spewock <jspewock@iol.unh.edu>
Date: Wed, 10 Jul 2024 12:44:23 -0400
Message-ID: <CAAA20UQgrMC79J49unvwy8iGUM+UQpuSdd0m36rrUen-tDju9g@mail.gmail.com>
Subject: Re: [PATCH v2 1/4] dts: add multicast set function to shell
To: Dean Marx <dmarx@iol.unh.edu>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org

On Mon, Jul 8, 2024 at 3:19=E2=80=AFPM Dean Marx <dmarx@iol.unh.edu> wrote:
>
> added set multicast function for changing allmulticast mode within testpm=
d.
>
> Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
> ---
>  dts/framework/remote_session/testpmd_shell.py | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framewor=
k/remote_session/testpmd_shell.py
> index ec22f72221..a0be0bd09d 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -806,6 +806,52 @@ def show_port_stats(self, port_id: int) -> TestPmdPo=
rtStats:
>
>          return TestPmdPortStats.parse(output)
>
> +    def set_promisc(self, port: int, on: bool, verify: bool =3D True):

This public method seems to be missing the return type, it should
probably specify that it doesn't return anything with a None-type.

> +        """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.
> +        """
> +        promisc_output =3D self.send_command(f"set promisc {port} {'on' =
if on else 'off'}")
> +        if verify:
> +            stats =3D self.show_port_info(port_id=3Dport)
> +            if on ^ stats.is_promiscuous_mode_enabled:
> +                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}.=
"
> +                )
> +
> +    def set_multicast_all(self, on: bool, verify: bool =3D True):

This method also should probably have a typehint for the return type.

> +        """Turns multicast mode on/off for the specified port.
> +
> +        Args:
> +            on: If :data:`True`, turns multicast mode on, otherwise turn=
s off.
> +            verify: If :data:`True` an additional command will be sent t=
o verify
> +            that multicast mode is properly set. Defaults to :data:`True=
`.

I'm surprised the formatting script doesn't complain about a
mismatched indent here, I thought it normally did for doc-strings. The
second line of this verify argument should probably be indented just
to better show that it is a continuation of the previous line.

> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True=
` and multicast
> +                mode is not properly set.
> +        """
> +        multicast_output =3D self.send_command(f"set allmulti all {'on' =
if on else 'off'}")
> +        if verify:
> +            stats0 =3D self.show_port_info(port_id=3D0)
> +            stats1 =3D self.show_port_info(port_id=3D1)

Getting the stats for port 0 and port 1 limit this method to only
working when there are exactly 2 ports. Right now in DTS this is a
restriction, but we should be able to make this robust enough to work
with any number of ports using self.show_port_info_all().

> +            if on ^ (stats0.is_allmulticast_mode_enabled and stats1.is_a=
llmulticast_mode_enabled):

If you used the show port info all command, you could then just
replace this `and` statement with something like this:

all(info.is_allmulticast_mode_enabled for info in all_stats)

if `all_stats` was the name of the list you stored the port stats in.


> +                self._logger.debug(
> +                    f"Failed to set multicast mode on all ports.: \n{mul=
ticast_output}"
> +                )
> +                raise InteractiveCommandExecutionError(
> +                    "Testpmd failed to set multicast mode on all ports."
> +                )
> +
>      def close(self) -> None:
>          """Overrides :meth:`~.interactive_shell.close`."""
>          self.send_command("quit", "")
> --
> 2.44.0
>