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 08FED454E5; Wed, 26 Jun 2024 20:21:48 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EEAC94027B; Wed, 26 Jun 2024 20:21:47 +0200 (CEST) Received: from mail-pg1-f177.google.com (mail-pg1-f177.google.com [209.85.215.177]) by mails.dpdk.org (Postfix) with ESMTP id 4039F4021E for ; Wed, 26 Jun 2024 20:21:47 +0200 (CEST) Received: by mail-pg1-f177.google.com with SMTP id 41be03b00d2f7-707040e3018so4871675a12.1 for ; Wed, 26 Jun 2024 11:21:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1719426106; x=1720030906; 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=PJF8wH4PRQSGw0uRZ46f36XLh+eXYwckxUviHLAkxd8=; b=QoFtX58CttgfvTmTvZA3bLE+maJDvGBObmaA6plKknARxlrnBr3kOJ0al4Spj+D/tk /r8dCtSfLU0eo5i6xr4V+KtvAtgkD7BgSUYSITbrHzFSKXIT78uCUHhqi9yVInLSqgq+ Zmsntz6FW69xxvQK+ngYmqV+P1W5QMQeCg6f0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719426106; x=1720030906; 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=PJF8wH4PRQSGw0uRZ46f36XLh+eXYwckxUviHLAkxd8=; b=qOZ/r5xzCqeoisHytAeqVAHSSyFEPewl8cuNdiYaPSEz3YZUvsnn3gi4qapUtRmRaX 4Bhf4/5XTnOrudKPJHZB6Mjrn/BU97Lzjz1lXEnZ9gE0VxJEbAoxRf9WwhqCYttRZK1p ObPUO/K05xkVC4nuu/QYlMcTWpmHGz/CqZdnMom5PZz+3nXxfwhcj0Sr52gGvpjFoD0j +IiEKTTPxi1SH93HM6LbxaiL0bu60LecggJO24fR/Ay3F0rfv/PTCy+5wXuUeJOO0w7X gJTU2p+HrhGk3sYFTbWX+6CCOJ6AebN0DW5moszAwm9jDI7G3xuY8NzE2pMUchUN9hod b0tA== X-Forwarded-Encrypted: i=1; AJvYcCU1nhfXhoK6YB+be26exnWO1bSlC9QnhyLD5ZGMyDQlNVwK1P4QhOSxk2i1AXa3rMDc9grzsr1Q1Nw+bKw= X-Gm-Message-State: AOJu0Yz1kSg98TwW5HrEhEAJoLgqMEfbwEmJjP7aShFM9B7/ZkInxAmv hxbmPIlFefp4JIH6a5qLdXSWqNMD+aYBRoQuoNQxghF6GXdoLVwoiQDagqsuOYNH/rp3zolNoVk g9KFrp285URC444A6NBd5aTWCHhl9r8NZ2A3XWw== X-Google-Smtp-Source: AGHT+IEpzn3gHJqpxw2DRUQOfIK5qE7eYmq5I0Uv278Gbgbtijb9bKsEd8IT9hddefoClglObbE2xvyEn2/SF4INfsE= X-Received: by 2002:a17:90a:cf92:b0:2c8:84b:7d70 with SMTP id 98e67ed59e1d1-2c86141c75amr10751108a91.42.1719426106200; Wed, 26 Jun 2024 11:21:46 -0700 (PDT) MIME-Version: 1.0 References: <20240614150238.26374-1-dmarx@iol.unh.edu> <20240625153324.27257-1-dmarx@iol.unh.edu> In-Reply-To: <20240625153324.27257-1-dmarx@iol.unh.edu> From: Jeremy Spewock Date: Wed, 26 Jun 2024 14:21:34 -0400 Message-ID: Subject: Re: [PATCH v7 1/3] dts: VLAN test suite implementation 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 Hey Dean, thanks for the update! I just went through and added my comments to the patches, but most of my feedback was about documentation and a few places where I think the classes could be improved by refactoring a little bit and breaking some things out into their own methods. A few comments on the overall series however: I think this patch should come after the patch that adds the testpmd functions. Just because the order they appear in the series is the order they get applied, so this patch without the patch that adds the testpmd methods would throw a lot of errors because the methods don't exist yet. Also, could you run the formatting script (devtools/dts-check-format.sh in the DPDK directory) on this series when sending out the next version? I noticed it was throwing some warnings/errors. On Tue, Jun 25, 2024 at 11:34=E2=80=AFAM Dean Marx wrot= e: > > Test suite for verifying VLAN filtering, stripping, and insertion > functionality on Poll Mode Driver. > > Signed-off-by: Dean Marx > --- > + def send_vlan_packet_and_verify( > + self, should_receive: bool, strip: bool, vlan_id: int > + ) -> None: > + """Generate a vlan packet, send and verify a packet with > + the same payload is received on the dut. > + > + 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 th= e vlan tag is > + checked for a match. We probably should have these args listed in the order that they appear in the function (should_receive, strip, vlan_id). > + """ > + > + def send_packet_and_verify_insertion(self, expected_id: int) -> None= : > + """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. The should_receive parameter seems like it was removed, we should probably remove this part of the doc-string as well. > + """ > + packet =3D Ether() / Raw(load=3D'xxxxx') > + received_packets =3D self.send_packet_and_capture(packet) > + test_packet =3D None > + for packet in received_packets: > + if b'xxxxx' in packet.load: > + test_packet =3D packet > + break > + self.verify( > + test_packet is not None, "Packet was dropped when it should = have been received" > + ) > + self.verify(Dot1Q in test_packet, "The received packet did not h= ave a vlan tag") > + self.verify(test_packet.vlan =3D=3D expected_id, "The received t= ag 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 TestPmdShell(node=3Dself.sut_node) > + testpmd.set_forward_mode(SimpleForwardingModes.mac) > + testpmd.set_verbose(1) > + testpmd.set_promisc(0, False) > + testpmd.vlan_filter_set_on(0) > + filtered_vlan =3D 1 > + testpmd.rx_vlan_add(filtered_vlan, 0) > + testpmd.start() All of these test cases have the exact same start sequences (other than one additional command for the tests that involve stripping, or the 3 extra in the insertion case), we should find a way to pull this out into another function that they can all call that does the testpmd setup. Or, alternatively, you could make a decorator for these test methods that handles all of the testpmd commands. That way, these test_ methods only need to concern themselves with how they call the method for sending and verifying packets and don't need to run any testpmd commands. I slightly prefer the decorator approach since I think that would be cleaner than if you were to make a testpmd shell, pass it into a setup function, and then close it after running testing, but it might be less clear what's actually going on. Regardless, the main point of it is having less code duplication between functions. > + > + self.send_vlan_packet_and_verify(True, strip=3DFalse, vlan_id=3D= filtered_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 TestPmdShell(node=3Dself.sut_node) > + testpmd.set_forward_mode(SimpleForwardingModes.mac) > + testpmd.set_verbose(1) > + testpmd.set_promisc(0, False) > + testpmd.vlan_filter_set_on(0) > + testpmd.rx_vlan_add(1, 0) This VLAN tag should probably also be pulled out into a variable like it is in other test methods, but we really should just do it once in a separate setup method instead of changing this to match up. > + 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 TestPmdShell(node=3Dself.sut_node) > + testpmd.set_forward_mode(SimpleForwardingModes.mac) > + testpmd.set_verbose(1) > + testpmd.set_promisc(0, False) > + testpmd.vlan_filter_set_on(0) > + filtered_vlan =3D 1 > + testpmd.rx_vlan_add(filtered_vlan, 0) > + testpmd.start() > + > + self.send_vlan_packet_and_verify(should_receive=3DFalse, strip= =3DFalse, vlan_id=3Dfiltered_vlan + 1) > + testpmd.close() > + > + def test_vlan_header_insertion(self) -> None: > + """Ensure that vlan packet is received with the correct inserted= vlan tag. > + > + Test: > + Create an interactive testpmd shell and verify a non-vlan pa= cket. > + """ > + testpmd =3D TestPmdShell(node=3Dself.sut_node) > + testpmd.set_forward_mode(SimpleForwardingModes.mac) > + testpmd.set_verbose(1) > + testpmd.set_promisc(0, False) > + testpmd.port_stop_all() > + testpmd.tx_vlan_set(1, 51) > + testpmd.port_start_all() > + testpmd.start() > + > + self.send_packet_and_verify_insertion(expected_id=3D51) > + testpmd.close() > -- > 2.44.0 >