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 4BB07454B7; Wed, 26 Jun 2024 17:50:32 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3657A402E4; Wed, 26 Jun 2024 17:50:32 +0200 (CEST) Received: from mail-pf1-f178.google.com (mail-pf1-f178.google.com [209.85.210.178]) by mails.dpdk.org (Postfix) with ESMTP id 049CF402CF for ; Wed, 26 Jun 2024 17:50:31 +0200 (CEST) Received: by mail-pf1-f178.google.com with SMTP id d2e1a72fcca58-706524adf91so4176824b3a.2 for ; Wed, 26 Jun 2024 08:50:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1719417030; x=1720021830; 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=lVVDN4ksF8As2q9coU+WgfGEHOySzoG5JTCxUYagnhs=; b=DS74Hv/wDebazS1rL+xZQuwfnM5SW8FIMrfMo5GmpTZCopE2KxOWOGR4E71dpjfgBV AWh2S4pyFOVRn5Q6e+Fqq++lhOab3Bz0PkyiWu8DpHVyRCx4iYft65Ms+PCClaUj53yc wju+WLNgy4Rn+e+c+8MWYKmSfmOLPF1VYcHqw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719417030; x=1720021830; 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=lVVDN4ksF8As2q9coU+WgfGEHOySzoG5JTCxUYagnhs=; b=dL2O2eHKTa79By40u2p/9PhD3l5uzUG/zHTXNFGN8fq7/9vN7BB7f454R7f2OnxPtz wOuKwc4NldCuC+j6ybeZ2D0bdypbRTsNkM6BsgMEy0y601ddUhNC+QIo2/CMFGfOvK6u TTg122EsxpXN4v66d+Yu4lMZamEhFaBmgPZqBsKUlM/HpM/aY+Qijel7IWM7Qj4PVPnF Zu2tkgyfJ7Lc9J42bEZyYYWcYmksvY6WVTmDSivmedPFPRJ3i596aiiu7vSOlJlUSMIM HnP2Im6odNt8PrvwNU7vzllpdhaU4YcFyMyZiFGKI7kLK+gPQF5Hk5+y1j14ezPFI24m UHdg== X-Forwarded-Encrypted: i=1; AJvYcCUuDm5I7MSmlvcUvdKrtujCk05+wKU4p59OerVlcB6t8Ko0OnClR5BGo+sM4qr1u11bam95zwFZEPY7ySc= X-Gm-Message-State: AOJu0Ywt34CI6UWlB2/oQJnNyUsVaADp+bw/0Oz0zAlrDSRhzZLEnj5P Rmw5aQ2ehQ0GjIU8P4DV/5XG+jVbsUpSAmp1wA7WVtU5xVrmAKINvZ9E1IJkhDvGJSNGcIzZjGQ t1f7H8Byh900A9/iD1y/oR7cwoz7LuMG1R+xPJw== X-Google-Smtp-Source: AGHT+IGXLsmZeiMfNc8cHTw+7HgPiDgBZNU0/kOiABhumtvdxzt490fj/VcWQIeaptGKHslZRrXnD3gnDwXfIcoJ1Nk= X-Received: by 2002:a05:6a20:1591:b0:1be:c762:8de3 with SMTP id adf61e73a8af0-1bec762987cmr1068419637.23.1719417029865; Wed, 26 Jun 2024 08:50:29 -0700 (PDT) MIME-Version: 1.0 References: <20240621172059.8194-2-npratte@iol.unh.edu> <20240621172059.8194-6-npratte@iol.unh.edu> In-Reply-To: <20240621172059.8194-6-npratte@iol.unh.edu> From: Jeremy Spewock Date: Wed, 26 Jun 2024 11:50:18 -0400 Message-ID: Subject: Re: [PATCH 2/3] dts: add testpmd methods for test suite To: Nicholas Pratte 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 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. > > 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/framewor= k/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) -> TestPmdPo= rt: > > 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. > + """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? > + Args: > + port_id: The port ID the mac address is set on. > + mac_address: The mac address to be added or removed to the s= pecified port. > + add: If :data:`True`, add the specified mac address. If :dat= a:`False`, remove specified > + mac address. > + verify: If :data:'True', assert that the 'mac_addr' operatio= n was successful. If > + :data:'False', run the command and skip this assertion. > + > + Raises: > + InteractiveCommandExecutionError: If either the 'add' or 're= move' 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. > + """ 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_a= ddress}") > + else: > + output =3D self.send_command(f"mac_addr remove {port_id} {ma= c_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}") > + if "Bad arguments" in output: > + self._logger.debug("Invalid argument provided to mac_addr") > + raise InteractiveCommandExecutionError("Invalid argument pro= vided") > + > + if verify: If you have the variable you can also remove this if-statement about which output/error to provide. Since it's the same error in either case you can just do: if "mac_addr_cmd error" in output: self._logger.debug(f"Failed to {mac_cmd} ... > + if add and "mac_addr_cmd error:" in output: > + self._logger.debug(f"Failed to add {mac_address} on port= {port_id}") > + raise InteractiveCommandExecutionError( > + f"Failed to add {mac_address} on port {port_id} \n{o= utput}" > + ) > + if not add and "mac_addr_cmd error" in output: > + self._logger.debug(f"Failed to remove {mac_address} on p= ort {port_id}") > + raise InteractiveCommandExecutionError( > + f"Failed to remove {mac_address} on port {port_id} \= n{output}" > + ) > + > + def set_multicast_mac_addr( > + self, port_id: int, multi_addr: str, add: bool =3D True, verify:= bool =3D True > + ): > + """Add or remove multicast mac address to a specified port filte= r. > + > + Args: > + port_id: The port ID the multicast address is set on. > + multi_addr: The multicast address to be added to the filter. > + add: If :data:'True', add the specified multicast address to= the port filter. > + If :data:'False', remove the specified multicast address= from the port filter. > + verify: If :data:'True', assert that the 'mcast_addr' operat= ions was successful. > + If :data:'False', execute the 'mcast_addr' operation and= skip the assertion. > + > + Raises: > + InteractiveCommandExecutionError: If either the 'add' or 're= move' operations fails. > + """ > + if add: > + output =3D self.send_command(f"mcast_addr add {port_id} {mul= ti_addr}") > + else: > + output =3D self.send_command(f"mcast_addr remove {port_id} {= multi_addr}") > + if "Bad arguments" in output: > + self._logger.debug("Invalid arguments provided to mcast_addr= ") > + raise InteractiveCommandExecutionError("Invalid argument pro= vided") > + > + if verify: > + if ( > + add > + and "Invalid multicast_addr" > + or "multicast address already filtered by port" in outpu= t If you use the variable here as well, this is a little trickier because this last line in the condition is different. Still can be solved however by making it: f"multicast address {'already' if add else 'not'} filtered by port" > + ): > + self._logger.debug(f"Failed to add {multi_addr} on port = {port_id}") > + raise InteractiveCommandExecutionError( > + f"Failed to add {multi_addr} on port {port_id} \n{ou= tput}" > + ) > + if ( > + not add > + and "Invalid multicast addr" Is this a typo or does testpmd remove the underscore when you're removing? > + or "multicast address not filtered by port" in output > + ): > + self._logger.debug(f"Failed to remove {multi_addr} on po= rt {port_id}") > + raise InteractiveCommandExecutionError( > + f"Failed to remove {multi_addr} on port {port_id} \n= {output}" > + ) > + > + > + 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 with= in 0-32. > + verify: If :data:`True`, the output of the command is scanne= d to verify that > + the vlan tag was removed from the filter list on the spe= cified port. If not, it is > + considered an error. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True= ` 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. > + """ > + > + def set_promisc(self, port: int, on: bool, verify: bool =3D 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 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. > + """ > + if on: > + promisc_output =3D self.send_command(f"set promisc {port} on= ") > + else: > + promisc_output =3D self.send_command(f"set promisc {port} of= f") This can also be done with a variable, or you can inline it by doing {'on' if on else 'off'}. > + if verify: > + if on and "Promiscuous mode: enabled" not in self.send_comma= nd( > + 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.sen= d_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}.= " > + ) 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 show_port_stats_all(self) -> list[TestPmdPortStats]: > """Returns the statistics of all the ports. > > -- > 2.44.0 >