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 AEC6545482; Mon, 17 Jun 2024 16:57:26 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9DC87402AB; Mon, 17 Jun 2024 16:57:26 +0200 (CEST) Received: from mail-pj1-f41.google.com (mail-pj1-f41.google.com [209.85.216.41]) by mails.dpdk.org (Postfix) with ESMTP id CB0FA402AB for ; Mon, 17 Jun 2024 16:56:48 +0200 (CEST) Received: by mail-pj1-f41.google.com with SMTP id 98e67ed59e1d1-2c2dee9d9a1so3605369a91.3 for ; Mon, 17 Jun 2024 07:56:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1718636208; x=1719241008; 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=ssjzt0ilDI+Kxu6MH0Sbsv3WomEzzSPKRtPSG/qUk2g=; b=USFh9jL7FpcbNMW8K2z+4shc94IyGnBFS50ClXcB4DLZvyW0i4+RR370ibpTodu0KV LZfLTAV6/nEvs5tPalQ+RE9C2sxlZsOkemRy1m+a1BVAB+9Awh5bJs775RgGcF6wDFu1 +HxSK2EGXMakZeb+1eIBTnsGxjUZ3hO9xhpNg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718636208; x=1719241008; 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=ssjzt0ilDI+Kxu6MH0Sbsv3WomEzzSPKRtPSG/qUk2g=; b=YMKFlC9RVgcOlsvH1vLD2VFCElh3qXTzNsCoNOusIKbJ9va3F8bnvIScO/YRVydYl9 QoR9mrmKt1uhCOjDbxJq7de9313RdWt/fcHTOqeBKODINTt8YT9qGNu/a7LGyhSeOKTs a3gfqJj/seSikZTuDH4/BkOP61+R80ylBACrJfkDlXf0miAsVlpZB2pXKJZaU/OIPjzI RGVaiGHV/ULKCa8yXkX5Z1zfjXtvAhZ8gGcoVJam6uPXBRaunV4Oliur6oPBSOuxLTcr xEodZWc5omeaMBowxEWt2G78Jh+ZSJzDEj00k4XaB5RTe0zwTIwhzCOZmu8W0PkKlOfO OPJA== X-Forwarded-Encrypted: i=1; AJvYcCX/NtXdXUcwC7yXb4FOQmFSs2c7+seVIfYunKy/NUHzXG4PgiK1JyACZyk/A5J6mZ+sEgy/J0PbKWRpprI= X-Gm-Message-State: AOJu0YzgHI0eIpGmj3n1JcaCEsdkoHxN22ILAgpLYZ69G3l3FnnjEzCl J/hv8TVUW1DpNd1e9Yb7GNu2+NpR7ppD3MzC9ChjhNxoxOVL39FWqSuIc86Ud9bEOzviabVlTXw UxwvY0g/enzxnRfbtfp89sKRMiKVFsURVobVMrg== X-Google-Smtp-Source: AGHT+IGXqqpKVp+JQHaYi15iJXR3yt5YYArTOcTxGJglIoBStTMpI5+1THb/iZshiFcNkdd4GWYXchJB4iSgzvaMcy4= X-Received: by 2002:a17:90a:ac16:b0:2c4:aab1:17f2 with SMTP id 98e67ed59e1d1-2c4dbb43eb2mr9112948a91.37.1718636207904; Mon, 17 Jun 2024 07:56:47 -0700 (PDT) MIME-Version: 1.0 References: <20240611161606.23881-2-dmarx@iol.unh.edu> <20240614150238.26374-1-dmarx@iol.unh.edu> <20240614150238.26374-3-dmarx@iol.unh.edu> In-Reply-To: <20240614150238.26374-3-dmarx@iol.unh.edu> From: Jeremy Spewock Date: Mon, 17 Jun 2024 10:56:36 -0400 Message-ID: Subject: Re: [PATCH v3 2/3] Initial implementation for VLAN test suite To: Dean Marx 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Fri, Jun 14, 2024 at 11:03=E2=80=AFAM Dean Marx wrot= e: > > Test suite for ensuring Poll Mode Driver can enable or disable > vlan filtering, stripping, and header insertion of packets sent on > a port. > > Signed-off-by: Dean Marx > --- > + > +class TestVlan(TestSuite): > + """DPDK VLAN test suite. > + > + Ensures VLAN packet reception on the Poll Mode Driver when certain c= onditions are met. > + If one or more of these conditions are not met, the packet reception= should be unsuccessful. > + """ > + > + def set_up_suite(self) -> None: > + """Set up the test suite. > + > + Setup: > + Create a testpmd session and set up tg nodes This isn't part of the setup of this suite, so you can probably leave this sentence out. > + verify that at least two ports are open for session > + """ > + self.verify(len(self._port_links) > 1, "Not enough ports") > + > + def send_vlan_packet_and_verify( > + self, should_receive: bool =3D True, strip: bool =3D False, vlan= _id: int =3D -1 > + ) -> None: I'm not sure it makes sense to default these parameters either. If we never use the default value of -1 for vlan_id, then we might as well make the argument positional instead of a keyword argument. > + """Generate a vlan packet, send and verify on the dut. > + Maybe you could add some more description about what is being verified in this method to the body of doc-string just to make things more clear. > + Args: > + should_receive: indicate whether the packet should be succes= sfully received > + vlan_id: expected vlan ID > + strip: indicates whether stripping is on or off, > + and when the vlan tag is checked for a match All of the individual arg definitions should end with periods and start with capitalized words (the descriptions should be capitalized, not the variable names). Additionally, the second line of the `strip` description should be indented just to show it's a continuation line. You can probably fit some more of this continuation on the line on the one above as well if that's easier. > + """ > + data =3D "P" * 10 > + packet =3D Ether() / Dot1Q(vlan=3Dvlan_id) / Raw(load=3Ddata) > + received_packets =3D self.send_packet_and_capture(packet) > + received_packets =3D [ > + packets > + for packets in received_packets > + if hasattr(packets, "load") and data in str((packets.load)) > + ] This might be easier to do with the built-in `filter` function in python: list(filter(lambda p: hasattr(p, "load") and data in str(p.load), received_packets)) Although what you have now is also intuitive and does the same thing so it doesn't really matter either way. > + if should_receive: > + self.verify( > + len(received_packets) =3D=3D 1, "Packet was dropped when= it should have been received" > + ) + > + def send_packet_and_verify_insertion(self, expected_id: int =3D -1) = -> None: This also uses an invalid ID but I think you could just make this positional/required instead. > + """Generate a packet with no vlan tag, send and verify on the du= t. > + > + Args: > + expected_id: the vlan id that is being inserted through tx_o= ffload configuration > + should_receive: indicate whether the packet should be succes= sfully received Same comment here about the args. > + """ > + data =3D "P" * 10 > + packet =3D Ether() / Raw(load=3Ddata) > + received_packets =3D self.send_packet_and_capture(packet) > + received_packets =3D [ > + packets > + for packets in received_packets > + if hasattr(packets, "load") and data in str((packets.load)) > + ] The start of this method is almost exactly the same as the start of the one for sending a VLAN packet. Maybe a different approach could be sending the packets in the test method, and then making this method instead just take in a list of packets and verifying them. Or, maybe you could instead make a different method for sending packets, and pass what that method returns into this one. Just to make sure there is as little duplicated code as possible. > + self.verify( > + len(received_packets) =3D=3D 1, "Packet was dropped when it = should have been received" > + ) > + received =3D received_packets[0] > + self.verify(Dot1Q in received, "The received packet did not have= a vlan tag") > + self.verify(received.vlan =3D=3D expected_id, "The received tag = did not match the expected tag") > + > + def test_vlan_receipt_no_stripping(self) -> None: > + """Ensure vlan packet is dropped when receipts are enabled and h= eader stripping is disabled. > + > + Test: > + Create an interactive testpmd shell and verify a vlan packet= . > + """ > + testpmd =3D self.sut_node.create_interactive_shell(TestPmdShell,= privileged=3DTrue) > + testpmd.set_forward_mode(TestPmdForwardingModes.mac) > + testpmd.send_command("set verbose 1", "testpmd>") > + testpmd.send_command("set promisc 0 off", "testpmd>") I saw Patrick also mentioned this, but I agree, we shouldn't need to be calling the send_command method anywhere. Maybe what we should do just to make sure people don't use it is override the method in TestpmdShell so that it throws an exception. That way we would enforce the rule a little more and it'd be less confusing. > + testpmd.vlan_filter_set_on(0) This is also the default value for this method, so if we were going to keep the defaults, this parameter would not be needed. However, we still should probably make those arguments positional and leave this as is. > + testpmd.rx_vlan_add(1, 0) This could also be testpmd.rx_vlan_add(1) with the defaults I believe, but that would be more ambiguous. > + testpmd.start() > + > + filtered_vlan =3D 1 It might make more sense to define this variable above the call to testpmd.rx_vlan_add and pass it into that function call as well. That way it isn't hard-coded in some places and used as this variable in others. > + self.send_vlan_packet_and_verify(True, vlan_id=3Dfiltered_vlan) > + testpmd.close() > + > + def test_vlan_receipt_stripping(self) -> None: > + """Ensure vlan packet received with no tag when receipts and hea= der stripping are enabled. > + > + Test: > + Create an interactive testpmd shell and verify a vlan packet= . > + """ > + testpmd =3D self.sut_node.create_interactive_shell(TestPmdShell,= privileged=3DTrue) > + testpmd.set_forward_mode(TestPmdForwardingModes.mac) > + testpmd.send_command("set verbose 1", "testpmd>") > + testpmd.send_command("set promisc 0 off", "testpmd>") > + testpmd.vlan_filter_set_on(0) > + testpmd.rx_vlan_add(1, 0) This part of the test method is identical to test_vlan_receipt_no_stripping, we could instead make one method for testing called vlan_receipt_testing(self, stripping: bool) and then set the vlan stripping to on if the boolean is true. > + testpmd.vlan_strip_set_on(0) > + testpmd.start() > + > + self.send_vlan_packet_and_verify(should_receive=3DTrue, strip=3D= True, vlan_id=3D1) > + testpmd.close() > + > + def test_vlan_no_receipt(self) -> None: > + """Ensure vlan packet dropped when filter is on and sent tag not= in the filter list. > + > + Test: > + Create an interactive testpmd shell and verify a vlan packet= . > + """ > + testpmd =3D self.sut_node.create_interactive_shell(TestPmdShell,= privileged=3DTrue) > + testpmd.set_forward_mode(TestPmdForwardingModes.mac) > + testpmd.send_command("set verbose 1", "testpmd>") > + testpmd.send_command("set promisc 0 off", "testpmd>") > + testpmd.vlan_filter_set_on(0) > + testpmd.rx_vlan_add(1, 0) > + testpmd.start() > + > + filtered_vlan =3D 1 This code is also identical to what is in test_vlan_receipt_no_stripping. Maybe in that additional test method you could also add a parameter for should_receive: bool and use that to decide what to pass into the send_vlan_pacet_and_verify function: def vlan_receipt_testig(self, stripping: bool, should_receive: bool) ^ As opposed to what I mentioned above with the vlan_receipt_testing functi= on. > + self.send_vlan_packet_and_verify(should_receive=3DFalse, vlan_id= =3Dfiltered_vlan + 1) > + testpmd.close() > + def tear_down_suite(self) -> None: > + """Tear down the suite.""" This method declaration isn't needed since we don't require any tear down steps to actually clean up after the test suite. > -- > 2.44.0 >