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 3AFF34554D; Tue, 2 Jul 2024 15:40:31 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0C58C402EC; Tue, 2 Jul 2024 15:40:31 +0200 (CEST) Received: from mail-lj1-f180.google.com (mail-lj1-f180.google.com [209.85.208.180]) by mails.dpdk.org (Postfix) with ESMTP id B365D402D2 for ; Tue, 2 Jul 2024 15:40:29 +0200 (CEST) Received: by mail-lj1-f180.google.com with SMTP id 38308e7fff4ca-2ee75d91d74so544461fa.0 for ; Tue, 02 Jul 2024 06:40:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1719927629; x=1720532429; 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=3k0pJlALjIopt5PRhhX23fjUiHM41ipGcgwU24m5Pzo=; b=CdVH2igfuL/v+hPqPcQezSpF44LM8JsK/flD9jhznRD51KGMGqa5ngRElHZa3f4rjY YRhzQvECRlP67GpQNfF2qASXOPHRBPxTjPC5tZYzdvv9Y3qFlGljGl7XyOfFFo5hZhJP Cz7UTbmmnu05c48Ab7+vKx7yfJEPDtXkfKW/M= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719927629; x=1720532429; 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=3k0pJlALjIopt5PRhhX23fjUiHM41ipGcgwU24m5Pzo=; b=Dn9gM4d5mXJIvI6QnTU6Gq/GMEcglMO5OjHfobdkiWX5yvsaA7WhUjjv2lm80HalXN YSCO7r5PldhrKTfSsW8VaK7cn3b9sC8rF3vytMCWVbOfHr4G5PbEL9AM0OGnnxxBPtAN 9pt7IQVq10eg2Yg+5O6L3cioDN+0Xb+fX5jm+j4arx/ipJK7Y6mnyxgIxyfa7o4ZMnns DAL2abijq2ucZp/R5tnA4x+nD3IE7p3PkHROWOeEUAr+NuzGGhjLR2ChjvS/q6GPQYFO kuP4mfmlFlbDREuqNTzCLySZDw5DVzIAWMLxt9Ch9MjqyUqA9R3rGxWp3cPUWuspwFVY ZomQ== X-Forwarded-Encrypted: i=1; AJvYcCU42sfgd6g4mEtV4HZjCDVmugx1B/pyRDzOFE3FN6D43Swtevl8UHiLiq82oV7QlSc+rZbvD76yWa3lLE4= X-Gm-Message-State: AOJu0YxPnzkVI+ybU9nBn4SO4ZgsuevhyF/QkmqjbSYmAkuQJMkCcyal dzeOJIUtrILkm8T0ex407aj5Pwh0NWgVIlgXShPhAO5rt0gsuMYL/3vb97yvm/+f3aWhkZCVW2t P8LBKjQwlgnohvC+8HbNv7J9O2IIEDfii+B5MTQ== X-Google-Smtp-Source: AGHT+IERpYrvO8BdaCJHf39WE9wHjxdVApGDQtTM33+Y1E+govbnmCuNpsVRyJnO0PuFysLyRkzppiBd2YCLJ1sw/Iw= X-Received: by 2002:a05:651c:2119:b0:2ec:42a2:7e25 with SMTP id 38308e7fff4ca-2ee5e6ba656mr62361001fa.3.1719927628932; Tue, 02 Jul 2024 06:40:28 -0700 (PDT) MIME-Version: 1.0 References: <20240621172059.8194-2-npratte@iol.unh.edu> <20240621172059.8194-6-npratte@iol.unh.edu> In-Reply-To: From: Nicholas Pratte Date: Tue, 2 Jul 2024 09:40:17 -0400 Message-ID: Subject: Re: [PATCH 2/3] dts: add testpmd methods for test suite To: Jeremy Spewock Cc: juraj.linkes@pantheon.tech, paul.szczepanek@arm.com, probb@iol.unh.edu, Honnappa.Nagarahalli@arm.com, yoan.picchi@foss.arm.com, dmarx@iol.unh.edu, luca.vizzarro@arm.com, bruce.richardson@intel.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 On Wed, Jun 26, 2024 at 11:50=E2=80=AFAM Jeremy Spewock wrote: > > I think the subject line of this commit makes sense in the context of > this series, but might be less helpful when the commit is applied to > the git tree. For this reason I might favor changing it to something > that briefly says what the added testpmd commands actually do. > > On Fri, Jun 21, 2024 at 1:23=E2=80=AFPM Nicholas Pratte wrote: > > > > Several methods are either imported from currently in-develoment test > > suites, or they have been created specifically for this test suite. > > > > Methods here that can be found in other test suites include vlan > > filtering, adding and removing vlan tags, and setting promiscuous mode. > > > > Methods that are specific to this test suite include adding or removing > > mac addresses, and adding or removing multicast mac addresses. > > You probably could make the subject about the adding/modification of > mac addresses judging by the body. > Good point. I'll make adjustments to the commit subject as well as the commit messages. > > > > Bugzilla ID: 1454 > > Signed-off-by: Nicholas Pratte > > --- > > dts/framework/remote_session/testpmd_shell.py | 209 ++++++++++++++++++ > > 1 file changed, 209 insertions(+) > > > > diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framew= ork/remote_session/testpmd_shell.py > > index ec22f72221..8a7d5905b3 100644 > > --- a/dts/framework/remote_session/testpmd_shell.py > > +++ b/dts/framework/remote_session/testpmd_shell.py > > @@ -767,6 +767,215 @@ def show_port_info(self, port_id: int) -> TestPmd= Port: > > > > return TestPmdPort.parse(output) > > > > + def set_mac_addr(self, port_id: int, mac_address: str, add: bool = =3D True, verify: bool =3D True): > > I'm not sure it makes sense to default the `add` parameter in this > method or the following. It seems to me like this method would likely > be split pretty evenly between adding and removing and I can't think > of a reason why one would be done in the general case over the other, > so I think it would be more clear if we didn't default it. > Good point. It made sense for me to do it this way in a vacuum (since I'm mostly adding addresses in each test case), but there's no good reason it should be this way. > > + """Add or remove a mac address on a given port. > > + > > Is it worth mentioning that we are adding the MAC address to the > allowlist of the port? Yes, I made a subtle change to the docstring. > > > + Args: > > + port_id: The port ID the mac address is set on. > > + mac_address: The mac address to be added or removed to the= specified port. > > + add: If :data:`True`, add the specified mac address. If :d= ata:`False`, remove specified > > + mac address. > > + verify: If :data:'True', assert that the 'mac_addr' operat= ion was successful. If > > + :data:'False', run the command and skip this assertion= . > > + > > + Raises: > > + InteractiveCommandExecutionError: If either the 'add' or '= remove' operations fail. > > This error documentation confuses me a little because it sounds like > both `add` and `remove` operations are happening in this method when > really either one happens or the other happens. Maybe changing this to > just saying the "mac address operation" failed would fix this. > Agreed. I made the change. > > + """ > > I think something that could save you a lot of space in this method > (and the next one) is creating a variable for what command to use if > `add` is true or not. > You could do something like this: > > mac_cmd =3D "add" if add else "remove" > > > + if add: > > + output =3D self.send_command(f"mac_addr add {port_id} {mac= _address}") > > + else: > > + output =3D self.send_command(f"mac_addr remove {port_id} {= mac_address}") > > Then you don't need this if-statement because this just becomes: > output =3D self.send_command(f"mac_addr {mac_cmd} {port_id} {mac_address}= ") > I love this! Good thinking. I'll make the changes. > > + ): > > + self._logger.debug(f"Failed to add {multi_addr} on por= t {port_id}") > > + raise InteractiveCommandExecutionError( > > + f"Failed to add {multi_addr} on port {port_id} \n{= output}" > > + ) > > + if ( > > + not add > > + and "Invalid multicast addr" > > Is this a typo or does testpmd remove the underscore when you're removing= ? Not a typo. Good attention to detail though. > > + > > + def rx_vlan_rm(self, vlan: int, port: int, verify: bool =3D 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 wi= thin 0-32. > > + verify: If :data:`True`, the output of the command is scan= ned to verify that > > + the vlan tag was removed from the filter list on the s= pecified port. If not, it is > > + considered an error. > > + > > + Raises: > > + InteractiveCommandExecutionError: If `verify` is :data:`Tr= ue` and the tag > > + is not removed. > > Other methods have this second line indented. I'm not sure if I > addressed this on the VLAN suite already or not but this should > probably do the same. All of the remaining methods were ripped from the VLAN suite, and I'll update these as that series progresses. I've been doing it this way to hopefully make it less confusing come the time these patches series are ready to be merged.