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 8228F454BF; Fri, 21 Jun 2024 22:54:00 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 70B464027D; Fri, 21 Jun 2024 22:54:00 +0200 (CEST) Received: from mail-pj1-f45.google.com (mail-pj1-f45.google.com [209.85.216.45]) by mails.dpdk.org (Postfix) with ESMTP id 52F4E40041 for ; Fri, 21 Jun 2024 22:53:59 +0200 (CEST) Received: by mail-pj1-f45.google.com with SMTP id 98e67ed59e1d1-2c31144881eso1983712a91.1 for ; Fri, 21 Jun 2024 13:53:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1719003238; x=1719608038; 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=PJqz+lEjJHe4ScbmnuZeaU07mb58gnpE7CBs/ilR5ew=; b=FK3EZLixY8inscKtlHQywnFTg+dYPB+Ydz6DfoLZaWE32IuLTl/YlMR7Rg2DDo9DEa lY8EpUOawOXqERlxgZDAUdfAKPxW2/Am6XHAHG1GuYyxEoLNOfqGes2vXijBenB4i+LI pIQGfzHnTBTnTO2jnfMA7hg0G0Hhv6mxNcJJs= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719003238; x=1719608038; 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=PJqz+lEjJHe4ScbmnuZeaU07mb58gnpE7CBs/ilR5ew=; b=PeORBBufyeMVYL08vfdMw/JhcQZ5jampC5KFlLe116XwhBrmst2m77f5CWd0b2NBfz oztOjzO56FrlUuxqJubQSycL0XmdfgrKu9HIbLxr3upSa84sM8gHiY0nVHX0afLw3YSO q6NISoldEdmvaTd2h2zl9PpnJU4O+V67CaLxc6D99AMEtC4RJ6RSU1oIaQk1bGP9Sz/5 TjklIxmJbbkJdjXsXUfDY34iUyGlXGjf5lyNYjQWwOL8NMDDQBWxdVC5+Z7st8obOdlC cYCwoTC4hnTlGb1egcBj2S4S6eS4qvRyvRCuayGeZ0XlWRdY6T8FVait1oUkMTPCZy7A JRQA== X-Forwarded-Encrypted: i=1; AJvYcCVk69p7w49q/3QmmHxtSHtFHhxYU1SDYN5ecKrhnZvFwLvswrPKTinsUhKSN2YP7BZy6EOejw4eQeKhQ1Q= X-Gm-Message-State: AOJu0YzjmNO94nxnto/wmxT3QzaNqIS6LbFvEz0ayltLuSNFCSWgBv90 fF3EPqbM/g3RW/i86IO2Kj3BYAO4SUvhvEjl0DaS6SKxdG18E5S3htdU+y+Tcxqzi2X4C34ecjY s1ftncZWXwfIwpVfUZ13PWniigVP8NMmgS+dQPw== X-Google-Smtp-Source: AGHT+IHVBGwZZWZIaR8K4pZf7Wf7jCN7BOKe3DTcJyU/WVSgMAw9d3aAf6UeUmJBvXYnjx48uKtBaGLsjhHFB2EbmI0= X-Received: by 2002:a17:90a:8c16:b0:2c6:de10:6ac3 with SMTP id 98e67ed59e1d1-2c7b5d7be3bmr9521889a91.31.1719003238290; Fri, 21 Jun 2024 13:53:58 -0700 (PDT) MIME-Version: 1.0 References: <20240614150238.26374-1-dmarx@iol.unh.edu> <20240618162939.23339-1-dmarx@iol.unh.edu> <20240618162939.23339-2-dmarx@iol.unh.edu> In-Reply-To: <20240618162939.23339-2-dmarx@iol.unh.edu> From: Jeremy Spewock Date: Fri, 21 Jun 2024 16:53:47 -0400 Message-ID: Subject: Re: [PATCH v5 2/3] dts: refactored 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 The subject for this commit should likely explain that this patch adds the VLAN test suite rather than refacoring it. This sort of falls in line with what I mentioned on the previous patch about the commit messages being about what the entire patch adds rather than the difference from the last version. Additionally, I think you will find that a lot of my comments on the previous version of this commit [1] still apply, but I'll try to point out anything I didn't mention there in this email. [1] http://inbox.dpdk.org/dev/CAAA20UTSSNB-t-ty+qpWJaz_hRJ1JX9HtM_kP_utnbyn= pPB0zw@mail.gmail.com/ On Tue, Jun 18, 2024 at 12:30=E2=80=AFPM Dean Marx wrot= e: > > Tweaked logic on sending and verifying packets for > more concise code, added verbose and promisc > function calls from pmd shell module. I mentioned this in more detail in the previous patch, but these descriptions should be more about what the entire patch adds than a change log from the previous. > > Signed-off-by: Dean Marx > --- > dts/tests/TestSuite_vlan.py | 167 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 167 insertions(+) > create mode 100644 dts/tests/TestSuite_vlan.py > > + > +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. The conditions should probably be stated here again briefly, and this doc-string can go more in depth about how the testing is done than the module which might help explain the test. You can also touch more here on what specific cases you are testing. > + """ > + > + def send_vlan_packet_and_verify( > + self, should_receive: bool =3D True, strip: bool =3D False, vlan= _id: int =3D -1 > + ) -> None: > + """Generate a vlan packet, send and verify on the dut. It probably makes more sense to call this a SUT rather than DUT since SUT is what we call it elsewhere in the code. > + > + 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 > + """ > + packet =3D Ether() / Dot1Q(vlan=3Dvlan_id) / Raw(load=3D'xxxxx') It looks like you are using this payload to filter the received packets but that might not be immediately obvious to other developers. A comment about what this is for might help make it more clear. Additionally, since this same filter is used later to check if you captured the packet you are looking for, it is probably better to pull this out into a seperate variable so it is only hard-coded in one place. > + received_packets =3D self.send_packet_and_capture(packet) > + test_packet =3D None > + for packet in received_packets: > + if packet.haslayer(Raw) and packet[Raw].load =3D=3D b'xxxxx'= : > + test_packet =3D packet > + break I said in the previous version you could use filter() to shrink the list, if you just want to find one that matches like this you could instead use the built in function called any() in python to get a boolean on if the packet exists or not. It does essentially the same thing you are doing but is a little more concise. I did something similar with the any function in this patch: https://patchwork.dpdk.org/project/dpdk/patch/20240613181510.30135-5-jspewo= ck@iol.unh.edu/ > + if should_receive: > + self.verify( > + test_packet is not None, "Packet was dropped when it sho= uld have been received" > + ) > + if strip: > + self.verify(Dot1Q not in test_packet, "Vlan tag was not = stripped successfully") > + else: > + self.verify( > + test_packet.vlan =3D=3D vlan_id, "The received t= ag did not match the expected tag" > + ) > + else: > + self.verify( > + test_packet is None, > + "Packet was received when it should have been dropped", > + ) > + > + def send_packet_and_verify_insertion(self, expected_id: int =3D -1) = -> 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 > + """ > + packet =3D Ether() / Raw(load=3D'xxxxx') We could do the same thing here with pulling out the filtering into its own variable. > + received_packets =3D self.send_packet_and_capture(packet) > + test_packet =3D None > + for packet in received_packets: > 2.44.0 >