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 7B21545538; Mon, 1 Jul 2024 18:49:41 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5E26D4014F; Mon, 1 Jul 2024 18:49:41 +0200 (CEST) Received: from mail-lj1-f174.google.com (mail-lj1-f174.google.com [209.85.208.174]) by mails.dpdk.org (Postfix) with ESMTP id C8275400EF for ; Mon, 1 Jul 2024 18:49:39 +0200 (CEST) Received: by mail-lj1-f174.google.com with SMTP id 38308e7fff4ca-2ec89b67bfcso2551031fa.3 for ; Mon, 01 Jul 2024 09:49:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1719852579; x=1720457379; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=4xHJi7vihLKcshJcvKMGGsQWbvMM4B38BXH/32YKdKU=; b=YlW7xuS9PAmuzHY68Xhpwbepj9KuGJbN6XUlRT1EPOBOcWhtDriap89i5/9AGSNR/A Mpi80LzZI3OQCSbkVbGUFRQLhHa13t4Bnpbtx35dgBvpW1GiQTEK1QgkwlvWkgsc9roR 96kz5Y7lQOAUJQ2CCJ/5VKI7mmSO6fTIPm0GU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719852579; x=1720457379; h=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=4xHJi7vihLKcshJcvKMGGsQWbvMM4B38BXH/32YKdKU=; b=dyvAn2l6rPGKguQdpAXZAqiDbhlUQzbET/WjC/hU4WBCCGPY5gBFCWob2zKgA+2HBJ hDbvxnkug3+edVIS4AXGQOE+jfnMY5JgYx2reuwO3LmtJT6RJoi5UF5xduLfdpVzyh79 WjTzZ748Fve60ODyY0Xxbn6JIwLoMQ0rsHf76YfBJHNvBsepfrp6gibew6yfM84/Md28 clmjwinyiE82AYWrExbnS+exYOHP32fNTB9/IIhA+yugN9t0WgREoH2QulewH5ECzN6N 57tcx13L87KIypZPX5KLmc5ogHYcxHbgoj/U7ntUTv4dY4CYj4ZynCHIR5KOT1nynR1M UmAA== X-Forwarded-Encrypted: i=1; AJvYcCUQDU+tCQkQ2rwlaUz2uPJAVpp1VJHUy0Mo1FtsCKjnnE4AtDfktONy1Cws1EK7OVDEl7RXbPVeR9GlN5E= X-Gm-Message-State: AOJu0YziUWD8/Vid0V0Grtts5Hpf0MIhiHEoSOEpgiKXkZXZnqm9PzGb NHZFxR4K+ha8sBQLikH+8lvNEuh9pL1triEtjlqDmkiwg+l+JKLsDbWk6WUzAqoA6gGgKH8idXB VVJyhkxCFYuS9cu/QCQrzSioGhVuCNuGV0zJOTQ== X-Google-Smtp-Source: AGHT+IH5N8h4kkrE3tu/HIl0GM03kDsRY/Oos3R1gdjeCJzKIJGWsK8ivLIm4uIGcecULUNe61kMPlazfF6Cq92a+SY= X-Received: by 2002:a2e:9084:0:b0:2ee:4a99:6a3a with SMTP id 38308e7fff4ca-2ee5e347339mr33081871fa.2.1719852579063; Mon, 01 Jul 2024 09:49:39 -0700 (PDT) MIME-Version: 1.0 References: <20240621172059.8194-2-npratte@iol.unh.edu> <20240621172059.8194-8-npratte@iol.unh.edu> In-Reply-To: From: Nicholas Pratte Date: Mon, 1 Jul 2024 12:49:28 -0400 Message-ID: Subject: Re: [PATCH 3/3] dts: mac filter test suite refactored for new dts 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" 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 > > > + def send_packet_and_verify( > > + self, > > + mac_address: str, > > + add_vlan: bool = False, > > + should_receive: bool = True, > > + ) -> None: > > + """Generate, send, and verify a packet based on specified parameters. > > + > > + Test cases within this suite utilize this method to create, send, and verify > > + packets based on criteria relating to the packet's destination mac address, > > + vlan tag, and whether or not the packet should be received or not. Packets > > + are verified using an inserted payload. If the list of received packets > > + contains this payload within any of its packets, the test case passes. This > > + implementation does not require a quiet wire, each call with this method > > I don't think we need to mention that it doesn't require a quiet wire, > in general this is something we are writing all test cases to assume. > > Looking at this though, it would probably be a great thing to add to > the documentation for writing test suites. We never really mention > anywhere that you must write test cases without this assumption, it's > sort of just something we decided. Of course out of scope of this > patch, but something for the future. Noted. I'll get rid of this. I also wouldn't mind making these changes in documentation at some point if others think it is a good idea. > > > + sends exactly one packet. > > + > > + Args: > > + mac_address: The destination mac address of the packet being sent. > > + add_vlan: Add a vlan tag to the packet being sent. :data:'2' if the packet > > + should be received, :data:'1' if the packet should not be received but > > + requires a vlan tag, and None for any other condition. > > This confused me a little because I thought you were saying this > parameter would be 2 if the packet should be received or it would be 1 > in another case. I think this parameter is really just denoting if we > should add a VLAN tag at all, and then the should_receive specifies > what the tag will be. We should probably move the information about > either being 2 or 1 to the `should_receive` documentation and specify > that the VLAN tag will be that value based on if it should be > received. > > This is a little hard because it relies on both conditions, but you > can just say in the `should_recieve` parameter that, if there is a > vlan tag, this is what it will be. I think you make a good point here, it's probably easier to read if I move the information like you said. > > > + should_receive: If :data:'True', assert whether or not the sent packet > > + has been received. If :data:'False', assert that the send packet was not > > + received. :data:'True' by default > > + """ > > + packet = ( > > + Ether() > > + / Dot1Q(vlan=2 if add_vlan and should_receive else 1 if add_vlan else None) > > I haven't used the Dot1Q class in scapy very much myself, but it seems > like just setting vlan=None is strange. Does this just make it > completely omit the VLAN tag? Also, if we don't have a VLAN tag, Yeah, it's funny you say this because, in doing some refactoring on Jumbo Frames, I put two and two together that None is not technically an acceptable parameter, although it tested fine, and I didn't get any warnings, formatting errors, or otherwise when I was was publishing the suite to the mailing list. > should we just not include the Dot1Q layer at all? It feels like it > doesn't really do anything if you don't have a VLAN. This is the solution to the problem, yes. It shouldn't take any work at all to rework this conditional. I guess my initial mindset was that Scapy might be 'smart' enough to understand that a 'None' input in the vlan attribute implies mitigating the vlan header entirely. In retrospect it doesn't really make any sense, but when everything tested fine, I think I was wrongfully convinced that I was right. Making this change would effectively have the same output for the test suite. > > > + / IP() > > + / Raw(load="P" * 22) > > + ) > > + packet.dst = mac_address > > + received_packets = [ > > + packets > > + for packets in self.send_packet_and_capture(packet, adjust_addresses=False) > > + if hasattr(packets, "load") and "P" * 22 in str(packets.load) > > + ] > > i think this would be a little easier using the filter() built-in > Python function. It would instead be: > recieved_packets = [filter(lambda x: if hasattr(x, "load") and "P" * > 22 in str(x.load), > self.send_packet_and_capture(packet, adjust_addresses=False)] > > It doesn't really save you that much verbosity now that I've written > it out, you kind of trade the "for x in ..." for "filter(lambda x: > ..." so maybe it isn't any more valuable. However, this function for > if a packet has a certain payload is becoming more and more common, I > wonder if we could just add a method to test_suite called > packet_has_payload(p: Packet, payload: str) that just checks does the > packet have the load attribute and does that load attribute match the > desired payload. Or, even better, if we made it a high-order function > that just accepts a payload and then returns a function that validates > if a packet has that payload. Something like this: > > def packet_has_payload(payload: str) -> Callable[[Packet], bool]: > def wrap(p: Packet) -> bool: > return if hasattr(p, "load") and payload in str(p.load) > return wrap > > If we had this high-order function then filtering becomes super easy. > The filter would just become: > recieved_packets = [filter(self.packet_has_payload("P" * 22), > self.send_packet_and_capture(packet, adjust_addresses=False)] > > Maybe this is something that should be addressed in the bugzilla > ticket 1372 however. Took a lot of trial and error to get this big if statement to work properly, I was wondering how long it would take before someone mentions it. I agree that the ticket is probably the best spot to discuss this, but there is definitely a better way of doing this. In any case, your filter solution is more graceful than mine, but ultimately I agree that the best solution is to just abstract this component away from the test suite development process; it will be much easier for developers to work on test suites with this out of sight and out of mind. > > > + if should_receive: > > + self.verify(len(received_packets) == 1, "Expected packet not received") > > + else: > > + self.verify(len(received_packets) == 0, "Expected packet received") > > + > > + def test_add_remove_mac_addresses(self) -> None: > > + """Assess basic mac addressing filtering functionalities. > > + > > + This test cases validates for proper behavior of mac address filtering with both > > + a port's default, burned-in mac address, as well as additional mac addresses > > + added to the PMD. Packets should either be received or not received depending on > > + the properties applied to the PMD at any given time. > > + > > + Test: > > + Start TestPMD with promiscuous mode. > > + Send a packet with the port's default mac address. (Should receive) > > + Send a packet with fake mac address. (Should not receive) > > + Add fake mac address to the PMD's address pool. > > + Send a packet with the fake mac address to the PMD. (Should receive) > > + Remove the fake mac address from the PMD's address pool. > > + Sent a packet with the fake mac address to the PMD. (Should not receive) > > Typo: Sent a packet. > > > + """ > > + testpmd = TestPmdShell(self.sut_node) > > + testpmd.set_promisc(0, on=False, verify=False) > > Why don't we want to verify this command? If there is a reason we > should probably leave a comment explaining it. My idea at the time was that promiscuous mode is technically not something that the test case is testing for, which seems a bit silly from a debugging perspective. I'll make the change. > > > + testpmd.start() > > + mac_address = self._sut_port_ingress.mac_address > > + > > + # Send a packet with NIC default mac address > > + self.send_packet_and_verify(mac_address=mac_address, should_receive=True) > > + # Send a packet with different mac address > > + fake_address = "00:00:00:00:00:01" > > + self.send_packet_and_verify(mac_address=fake_address, should_receive=False) > > + > > + # Add mac address to pool and rerun tests > > + testpmd.set_mac_addr(0, mac_address=fake_address, verify=True) > > verify=True if the default and we generally omit this, we should > probably omit it here as well. Noted. > > > + self.send_packet_and_verify(mac_address=fake_address, should_receive=True) > > + testpmd.set_mac_addr(0, mac_address=fake_address, add=False) > > + self.send_packet_and_verify(mac_address=fake_address, should_receive=False) > > + > > + def test_invalid_address(self) -> None: > > + """Assess the behavior of a NIC mac address pool while binded to the PMD. > > Should this be binded or bound? I think it's bound, but I'm not sure > if it's different when referring to drivers. I don't think binded is a word... I'll get rid of that. That's a good catch! > > > + > > + An assessment of a NIC's behavior when mounted to a PMD as it relates to mac addresses > > + and address pooling. Devices should not be able to use invalid mac addresses, remove their > > + built-in hardware address, or exceed their address pools. > > + > > + Test: > > + Start TestPMD. > > + Attempt to set add an invalid mac address. (Should fail) > > Typo: Attempt to set add an invalid address. > > > + Attempt to remove the device's hardware address. (Should fail) I'll add an additional explanation here, with respect to your comment below. > > + Add a fake mac address to the pool twice in succession. (Should not create any errors) > > + Attempt to remove the device's hardware address. (Should fail) > > I assume the reason this is checked again is making sure it still > fails when the device mac is not the only one in the pool, but we > should probably explain that. Good point! I'll add some additional explanation for this line and the one above. > > > + Determine the device's mac address pool size, and fill the pool with fake addresses. > > + Attempt to add another fake mac address, overloading the address pool. (Should not work) > > We say "should fail" everywhere else. Doesn't really change anything, > but it would probably flow a little better to keep this consistent. > > > + """ > > + testpmd = TestPmdShell(self.sut_node) > > + testpmd.start() > > + mac_address = self._sut_port_ingress.mac_address > > + try: > > + testpmd.set_mac_addr(0, "00:00:00:00:00:00") > > + self.verify(False, "Invalid mac address added.") > > + except InteractiveCommandExecutionError: > > + pass > > + try: > > + testpmd.set_mac_addr(0, mac_address, False) > > + self.verify(False, "Default mac address removed.") > > + except InteractiveCommandExecutionError: > > + pass > > + # Should be no errors adding this twice > > + testpmd.set_mac_addr(0, f"'1' + {mac_address[1:]}") > > + testpmd.set_mac_addr(0, f"'1' + {mac_address[1:]}") > > + # Double check to see if default mac address can be removed > > + try: > > + testpmd.set_mac_addr(0, mac_address, False) > > + self.verify(False, "Default mac address removed.") > > + except InteractiveCommandExecutionError: > > + pass > > + > > + for i in range(testpmd.show_port_info(0).max_mac_addresses_num - 1): > > + number = str(hex(i)[2:].zfill(4)) > > + fake_address = f'{number[:2] + ":" + number[2:] + mac_address[5:]}'.upper() > > It's not immediately obvious to me what this is doing, maybe a comment > of an example about what this would look like for iteration 1 vs > iteration 2 would help make it clear. Sure, that seems reasonable. For the record, this adds a bunch of fake mac addresses to fill the entire mac address pool on a given NIC. I'm actually going to refactor this to make it easier to read. > > > + testpmd.set_mac_addr(0, fake_address, verify=False) > > + try: > > + testpmd.set_mac_addr(0, f'{mac_address[:-1] + "1"}') > > What happens here if the last character in the MAC address already is > 1? This would fail the test case right? Given the way it was designed above, only the first couple of octets would be manipulated to create fake addresses. So there should never be a circumstance where this address is used. > > > + self.verify(False, "Mac address limit exceeded.") > > + except InteractiveCommandExecutionError: > > + pass > > + testpmd.close() > > + > > + def test_multicast_filter(self) -> None: > > + """Assess basic multicast address filtering functionalities. > > + > > + Ensure that multicast filtering performs as intended when a given device is bound > > + to the PMD, with and without dot1q vlan tagging. > > + > > + Test: > > + Start TestPMD with promiscuous mode. > > + Add a fake multicast address to the PMD's multicast address pool. > > + Send a packet with the fake multicast address to the PMD. (Should receive) > > + Set vlan filtering on the PMD, and add vlan ID to the PMD. > > This test case seems like it is trying to test the multicast mac > filtering, do we need to worry about VLAN filtering here as well, or > just simply test when the VLAN tag is present? If possible it's > probably better to avoid the testing of VLAN filtering since that is a > different suite and I would assume that the functionality of filtering > based on VLANs isn't directly related to filtering based on MAC > addresses but I could be wrong. It's a good point, and it was kind of a head scratcher for me as well, but I threw it in the test suite since vlan filtering on multicast was also conducted in the old DTS test suite. I'd be willing to remove this if people generally agree it's not needed because I also agree that it's seemingly tangential from the rest of the test suite.