[PATCH v1 0/1] dts: adjust packet addressing and sending

[PATCH v1 0/1] dts: adjust packet addressing and sending

[jspewock]

From: Jeremy Spewock <jspewock@iol.unh.edu>

This patch was originally part of the dynamic queue test suite series,
but since other patches require it this series creates an independent
patch to allow it to be prioritized.

This patch is slightly different than the one in dynamic queue as this
version supports address updating on packets with multiple IP layers by
modifying the method of checking whether or not the developer updated
the addresses.

Jeremy Spewock (1):
  dts: add send_packets to test suites and rework packet addressing

 dts/framework/test_suite.py            | 87 +++++++++++++++++++-------
 dts/framework/testbed_model/tg_node.py |  9 +++
 2 files changed, 75 insertions(+), 21 deletions(-)

-- 
2.46.0

[PATCH v1 1/1] dts: add send_packets to test suites and rework packet addressing

[jspewock]

From: Jeremy Spewock <jspewock@iol.unh.edu>

Currently the only method provided in the test suite class for sending
packets sends a single packet and then captures the results. There is,
in some cases, a need to send multiple packets at once while not really
needing to capture any traffic received back. The method to do this
exists in the traffic generator already, but this patch exposes the
method to test suites.

This patch also updates the _adjust_addresses method of test suites so
that addresses of packets are only modified if the developer did not
configure them beforehand. This allows for developers to have more
control over the content of their packets when sending them through the
framework.

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/framework/test_suite.py            | 87 +++++++++++++++++++-------
 dts/framework/testbed_model/tg_node.py |  9 +++
 2 files changed, 75 insertions(+), 21 deletions(-)

diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
index 694b2eba65..11aaa0a93a 100644
--- a/dts/framework/test_suite.py
+++ b/dts/framework/test_suite.py
@@ -199,7 +199,7 @@ def send_packet_and_capture(
         Returns:
             A list of received packets.
         """
-        packet = self._adjust_addresses(packet)
+        packet = self._adjust_addresses([packet])[0]
         return self.tg_node.send_packet_and_capture(
             packet,
             self._tg_port_egress,
@@ -208,6 +208,18 @@ def send_packet_and_capture(
             duration,
         )
 
+    def send_packets(
+        self,
+        packets: list[Packet],
+    ) -> None:
+        """Send packets using the traffic generator and do not capture received traffic.
+
+        Args:
+            packets: Packets to send.
+        """
+        packets = self._adjust_addresses(packets)
+        self.tg_node.send_packets(packets, self._tg_port_egress)
+
     def get_expected_packet(self, packet: Packet) -> Packet:
         """Inject the proper L2/L3 addresses into `packet`.
 
@@ -217,41 +229,74 @@ def get_expected_packet(self, packet: Packet) -> Packet:
         Returns:
             `packet` with injected L2/L3 addresses.
         """
-        return self._adjust_addresses(packet, expected=True)
+        return self._adjust_addresses([packet], expected=True)[0]
 
-    def _adjust_addresses(self, packet: Packet, expected: bool = False) -> Packet:
+    def _adjust_addresses(self, packets: list[Packet], expected: bool = False) -> list[Packet]:
         """L2 and L3 address additions in both directions.
 
+        Packets in `packets` will be directly modified in this method. The returned list of packets
+        however will be copies of the modified packets in order to keep the two lists distinct.
+
+        Only missing addresses are added to packets, existing addresses will not be overridden. If
+        any packet in `packets` has multiple IP layers (using GRE, for example) only the inner-most
+        IP layer will have its addresses adjusted.
+
         Assumptions:
             Two links between SUT and TG, one link is TG -> SUT, the other SUT -> TG.
 
         Args:
-            packet: The packet to modify.
+            packets: The packets to modify.
             expected: If :data:`True`, the direction is SUT -> TG,
                 otherwise the direction is TG -> SUT.
+
+        Returns:
+            A list containing copies of all packets in `packets` after modification.
         """
-        if expected:
-            # The packet enters the TG from SUT
-            # update l2 addresses
-            packet.src = self._sut_port_egress.mac_address
-            packet.dst = self._tg_port_ingress.mac_address
+        ret_packets = []
+        for packet in packets:
+            # The fields parameter of a packet does not include fields of the payload, so this can
+            # only be the Ether src/dst.
+            pkt_src_is_unset = "src" not in packet.fields
+            pkt_dst_is_unset = "dst" not in packet.fields
+            num_ip_layers = packet.layers().count(IP)
+
+            # Update the last IP layer if there are multiple to account for GRE addressing (the
+            # framework should be modifying the packet address instead of the tunnel).
+            l3_to_use = packet.getlayer(IP, num_ip_layers)
+            if num_ip_layers > 0:
+                ip_src_is_unset = "src" not in l3_to_use.fields
+                ip_dst_is_unset = "dst" not in l3_to_use.fields
+            else:
+                ip_src_is_unset = None
+                ip_dst_is_unset = None
 
-            # The packet is routed from TG egress to TG ingress
-            # update l3 addresses
-            packet.payload.src = self._tg_ip_address_egress.ip.exploded
-            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
-        else:
-            # The packet leaves TG towards SUT
             # update l2 addresses
-            packet.src = self._tg_port_egress.mac_address
-            packet.dst = self._sut_port_ingress.mac_address
+            # If `expected` is :data:`True`, the packet enters the TG from SUT, otherwise the
+            # packet leaves the TG towards the SUT
+            if pkt_src_is_unset:
+                packet.src = (
+                    self._sut_port_egress.mac_address
+                    if expected
+                    else self._tg_port_egress.mac_address
+                )
+            if pkt_dst_is_unset:
+                packet.dst = (
+                    self._tg_port_ingress.mac_address
+                    if expected
+                    else self._sut_port_ingress.mac_address
+                )
 
-            # The packet is routed from TG egress to TG ingress
             # update l3 addresses
-            packet.payload.src = self._tg_ip_address_egress.ip.exploded
-            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
+            # The packet is routed from TG egress to TG ingress regardless of whether it is
+            # expected or not.
+            if ip_src_is_unset:
+                l3_to_use.src = self._tg_ip_address_egress.ip.exploded
+
+            if ip_dst_is_unset:
+                l3_to_use.dst = self._tg_ip_address_ingress.ip.exploded
+            ret_packets.append(Ether(packet.build()))
 
-        return Ether(packet.build())
+        return ret_packets
 
     def verify(self, condition: bool, failure_description: str) -> None:
         """Verify `condition` and handle failures.
diff --git a/dts/framework/testbed_model/tg_node.py b/dts/framework/testbed_model/tg_node.py
index 4ee326e99c..758b676258 100644
--- a/dts/framework/testbed_model/tg_node.py
+++ b/dts/framework/testbed_model/tg_node.py
@@ -83,6 +83,15 @@ def send_packet_and_capture(
             duration,
         )
 
+    def send_packets(self, packets: list[Packet], port: Port):
+        """Send packets without capturing resulting received packets.
+
+        Args:
+            packets: Packets to send.
+            port: Port to send the packets on.
+        """
+        self.traffic_generator.send_packets(packets, port)
+
     def close(self) -> None:
         """Free all resources used by the node.
 
-- 
2.46.0

Re: [PATCH v1 1/1] dts: add send_packets to test suites and rework packet addressing

[Dean Marx]

[-- Attachment #1: Type: text/plain, Size: 924 bytes --]

On Wed, Sep 4, 2024 at 11:28 AM <jspewock@iol.unh.edu> wrote:

> From: Jeremy Spewock <jspewock@iol.unh.edu>
>
> Currently the only method provided in the test suite class for sending
> packets sends a single packet and then captures the results. There is,
> in some cases, a need to send multiple packets at once while not really
> needing to capture any traffic received back. The method to do this
> exists in the traffic generator already, but this patch exposes the
> method to test suites.
>
> This patch also updates the _adjust_addresses method of test suites so
> that addresses of packets are only modified if the developer did not
> configure them beforehand. This allows for developers to have more
> control over the content of their packets when sending them through the
> framework.
>
> Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
>

Reviewed-by: Dean Marx <dmarx@iol.unh.edu>

[-- Attachment #2: Type: text/html, Size: 1408 bytes --]

Re: [PATCH v1 1/1] dts: add send_packets to test suites and rework packet addressing

[Patrick Robb]

This looks good, except for the fact that a method for sending a list
of packets has been added since you submitted this patch (which does
return the packet list, not that it matters for your application).

So, this will need to be resubmitted with your patch reformatted for
_adjust_addresses() to be used by luca's send_packet_and_capture ->
send_packets_and_capture approach which is now in the framework.

On Wed, Sep 4, 2024 at 11:28 AM <jspewock@iol.unh.edu> wrote:
>
> From: Jeremy Spewock <jspewock@iol.unh.edu>
>
> Currently the only method provided in the test suite class for sending
> packets sends a single packet and then captures the results. There is,
> in some cases, a need to send multiple packets at once while not really
> needing to capture any traffic received back. The method to do this
> exists in the traffic generator already, but this patch exposes the
> method to test suites.
>
> This patch also updates the _adjust_addresses method of test suites so
> that addresses of packets are only modified if the developer did not
> configure them beforehand. This allows for developers to have more
> control over the content of their packets when sending them through the
> framework.
>
> Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> ---
>  dts/framework/test_suite.py            | 87 +++++++++++++++++++-------
>  dts/framework/testbed_model/tg_node.py |  9 +++
>  2 files changed, 75 insertions(+), 21 deletions(-)
>
> diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
> index 694b2eba65..11aaa0a93a 100644
> --- a/dts/framework/test_suite.py
> +++ b/dts/framework/test_suite.py
> @@ -199,7 +199,7 @@ def send_packet_and_capture(
>          Returns:
>              A list of received packets.
>          """
> -        packet = self._adjust_addresses(packet)
> +        packet = self._adjust_addresses([packet])[0]
>          return self.tg_node.send_packet_and_capture(
>              packet,
>              self._tg_port_egress,
> @@ -208,6 +208,18 @@ def send_packet_and_capture(
>              duration,
>          )
>
> +    def send_packets(
> +        self,
> +        packets: list[Packet],
> +    ) -> None:
> +        """Send packets using the traffic generator and do not capture received traffic.
> +
> +        Args:
> +            packets: Packets to send.
> +        """
> +        packets = self._adjust_addresses(packets)
> +        self.tg_node.send_packets(packets, self._tg_port_egress)
> +
>      def get_expected_packet(self, packet: Packet) -> Packet:
>          """Inject the proper L2/L3 addresses into `packet`.
>
> @@ -217,41 +229,74 @@ def get_expected_packet(self, packet: Packet) -> Packet:
>          Returns:
>              `packet` with injected L2/L3 addresses.
>          """
> -        return self._adjust_addresses(packet, expected=True)
> +        return self._adjust_addresses([packet], expected=True)[0]
>
> -    def _adjust_addresses(self, packet: Packet, expected: bool = False) -> Packet:
> +    def _adjust_addresses(self, packets: list[Packet], expected: bool = False) -> list[Packet]:
>          """L2 and L3 address additions in both directions.

^Just confirming again I think this implementation is fine, the patch
just needs to be reformatted to work on top of the new
testsuite/tgnode pktgen methods. Cheers.

[PATCH v2 0/1] dts: adjust packet addressing and sending

[jspewock]

From: Jeremy Spewock <jspewock@iol.unh.edu>

v2:
  * rebase on next-dts

Jeremy Spewock (1):
  dts: add send_packets to test suites and rework packet addressing

 dts/framework/test_suite.py            | 87 +++++++++++++++++++-------
 dts/framework/testbed_model/tg_node.py |  9 +++
 2 files changed, 75 insertions(+), 21 deletions(-)

-- 
2.46.0

[PATCH v2 1/1] dts: add send_packets to test suites and rework packet addressing

[jspewock]

From: Jeremy Spewock <jspewock@iol.unh.edu>

Currently the only method provided in the test suite class for sending
packets sends a single packet and then captures the results. There is,
in some cases, a need to send multiple packets at once while not really
needing to capture any traffic received back. The method to do this
exists in the traffic generator already, but this patch exposes the
method to test suites.

This patch also updates the _adjust_addresses method of test suites so
that addresses of packets are only modified if the developer did not
configure them beforehand. This allows for developers to have more
control over the content of their packets when sending them through the
framework.

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/framework/test_suite.py            | 87 +++++++++++++++++++-------
 dts/framework/testbed_model/tg_node.py |  9 +++
 2 files changed, 75 insertions(+), 21 deletions(-)

diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
index 051509fb86..4c12c4a328 100644
--- a/dts/framework/test_suite.py
+++ b/dts/framework/test_suite.py
@@ -225,7 +225,7 @@ def send_packets_and_capture(
         Returns:
             A list of received packets.
         """
-        packets = [self._adjust_addresses(packet) for packet in packets]
+        packets = self._adjust_addresses(packets)
         return self.tg_node.send_packets_and_capture(
             packets,
             self._tg_port_egress,
@@ -234,6 +234,18 @@ def send_packets_and_capture(
             duration,
         )
 
+    def send_packets(
+        self,
+        packets: list[Packet],
+    ) -> None:
+        """Send packets using the traffic generator and do not capture received traffic.
+
+        Args:
+            packets: Packets to send.
+        """
+        packets = self._adjust_addresses(packets)
+        self.tg_node.send_packets(packets, self._tg_port_egress)
+
     def get_expected_packet(self, packet: Packet) -> Packet:
         """Inject the proper L2/L3 addresses into `packet`.
 
@@ -243,41 +255,74 @@ def get_expected_packet(self, packet: Packet) -> Packet:
         Returns:
             `packet` with injected L2/L3 addresses.
         """
-        return self._adjust_addresses(packet, expected=True)
+        return self._adjust_addresses([packet], expected=True)[0]
 
-    def _adjust_addresses(self, packet: Packet, expected: bool = False) -> Packet:
+    def _adjust_addresses(self, packets: list[Packet], expected: bool = False) -> list[Packet]:
         """L2 and L3 address additions in both directions.
 
+        Packets in `packets` will be directly modified in this method. The returned list of packets
+        however will be copies of the modified packets in order to keep the two lists distinct.
+
+        Only missing addresses are added to packets, existing addresses will not be overridden. If
+        any packet in `packets` has multiple IP layers (using GRE, for example) only the inner-most
+        IP layer will have its addresses adjusted.
+
         Assumptions:
             Two links between SUT and TG, one link is TG -> SUT, the other SUT -> TG.
 
         Args:
-            packet: The packet to modify.
+            packets: The packets to modify.
             expected: If :data:`True`, the direction is SUT -> TG,
                 otherwise the direction is TG -> SUT.
+
+        Returns:
+            A list containing copies of all packets in `packets` after modification.
         """
-        if expected:
-            # The packet enters the TG from SUT
-            # update l2 addresses
-            packet.src = self._sut_port_egress.mac_address
-            packet.dst = self._tg_port_ingress.mac_address
+        ret_packets = []
+        for packet in packets:
+            # The fields parameter of a packet does not include fields of the payload, so this can
+            # only be the Ether src/dst.
+            pkt_src_is_unset = "src" not in packet.fields
+            pkt_dst_is_unset = "dst" not in packet.fields
+            num_ip_layers = packet.layers().count(IP)
+
+            # Update the last IP layer if there are multiple to account for GRE addressing (the
+            # framework should be modifying the packet address instead of the tunnel).
+            l3_to_use = packet.getlayer(IP, num_ip_layers)
+            if num_ip_layers > 0:
+                ip_src_is_unset = "src" not in l3_to_use.fields
+                ip_dst_is_unset = "dst" not in l3_to_use.fields
+            else:
+                ip_src_is_unset = None
+                ip_dst_is_unset = None
 
-            # The packet is routed from TG egress to TG ingress
-            # update l3 addresses
-            packet.payload.src = self._tg_ip_address_egress.ip.exploded
-            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
-        else:
-            # The packet leaves TG towards SUT
             # update l2 addresses
-            packet.src = self._tg_port_egress.mac_address
-            packet.dst = self._sut_port_ingress.mac_address
+            # If `expected` is :data:`True`, the packet enters the TG from SUT, otherwise the
+            # packet leaves the TG towards the SUT
+            if pkt_src_is_unset:
+                packet.src = (
+                    self._sut_port_egress.mac_address
+                    if expected
+                    else self._tg_port_egress.mac_address
+                )
+            if pkt_dst_is_unset:
+                packet.dst = (
+                    self._tg_port_ingress.mac_address
+                    if expected
+                    else self._sut_port_ingress.mac_address
+                )
 
-            # The packet is routed from TG egress to TG ingress
             # update l3 addresses
-            packet.payload.src = self._tg_ip_address_egress.ip.exploded
-            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
+            # The packet is routed from TG egress to TG ingress regardless of whether it is
+            # expected or not.
+            if ip_src_is_unset:
+                l3_to_use.src = self._tg_ip_address_egress.ip.exploded
+
+            if ip_dst_is_unset:
+                l3_to_use.dst = self._tg_ip_address_ingress.ip.exploded
+            ret_packets.append(Ether(packet.build()))
 
-        return Ether(packet.build())
+        return ret_packets
 
     def verify(self, condition: bool, failure_description: str) -> None:
         """Verify `condition` and handle failures.
diff --git a/dts/framework/testbed_model/tg_node.py b/dts/framework/testbed_model/tg_node.py
index 19b5b6e74c..4179365abb 100644
--- a/dts/framework/testbed_model/tg_node.py
+++ b/dts/framework/testbed_model/tg_node.py
@@ -83,6 +83,15 @@ def send_packets_and_capture(
             duration,
         )
 
+    def send_packets(self, packets: list[Packet], port: Port):
+        """Send packets without capturing resulting received packets.
+
+        Args:
+            packets: Packets to send.
+            port: Port to send the packets on.
+        """
+        self.traffic_generator.send_packets(packets, port)
+
     def close(self) -> None:
         """Free all resources used by the node.
 
-- 
2.46.0

Re: [PATCH v2 1/1] dts: add send_packets to test suites and rework packet addressing

[Juraj Linkeš]

On 20. 9. 2024 20:08, jspewock@iol.unh.edu wrote:
> From: Jeremy Spewock <jspewock@iol.unh.edu>
> 
> Currently the only method provided in the test suite class for sending
> packets sends a single packet and then captures the results. There is,
> in some cases, a need to send multiple packets at once while not really
> needing to capture any traffic received back. The method to do this
> exists in the traffic generator already, but this patch exposes the
> method to test suites.
> 

Patrick mentioned there is now a method that does it 
(send_packets_and_capture()), but that one captures the packets. I think 
we should add this method though, as it does something different, but 
maybe the most important point is that non-capturing TGs are going to 
support send_packets_and_capture(), so we need it either way.

> This patch also updates the _adjust_addresses method of test suites so
> that addresses of packets are only modified if the developer did not
> configure them beforehand. This allows for developers to have more
> control over the content of their packets when sending them through the
> framework.
> 

I think this could and should be split into two patches. They aren't 
really connected.


> diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py

> @@ -243,41 +255,74 @@ def get_expected_packet(self, packet: Packet) -> Packet:
>           Returns:
>               `packet` with injected L2/L3 addresses.
>           """
> -        return self._adjust_addresses(packet, expected=True)
> +        return self._adjust_addresses([packet], expected=True)[0]
>   
> -    def _adjust_addresses(self, packet: Packet, expected: bool = False) -> Packet:
> +    def _adjust_addresses(self, packets: list[Packet], expected: bool = False) -> list[Packet]:
>           """L2 and L3 address additions in both directions.
>   
> +        Packets in `packets` will be directly modified in this method. The returned list of packets
> +        however will be copies of the modified packets in order to keep the two lists distinct.
> +

Do we need to do this? I guess you needed this in a test suite - what is 
the reason?

> +        Only missing addresses are added to packets, existing addresses will not be overridden. If
> +        any packet in `packets` has multiple IP layers (using GRE, for example) only the inner-most
> +        IP layer will have its addresses adjusted.
> +
>           Assumptions:
>               Two links between SUT and TG, one link is TG -> SUT, the other SUT -> TG.
>   
>           Args:
> -            packet: The packet to modify.
> +            packets: The packets to modify.
>               expected: If :data:`True`, the direction is SUT -> TG,
>                   otherwise the direction is TG -> SUT.
> +
> +        Returns:
> +            A list containing copies of all packets in `packets` after modification.
>           """
> -        if expected:
> -            # The packet enters the TG from SUT
> -            # update l2 addresses
> -            packet.src = self._sut_port_egress.mac_address
> -            packet.dst = self._tg_port_ingress.mac_address
> +        ret_packets = []
> +        for packet in packets:
> +            # The fields parameter of a packet does not include fields of the payload, so this can
> +            # only be the Ether src/dst.
> +            pkt_src_is_unset = "src" not in packet.fields
> +            pkt_dst_is_unset = "dst" not in packet.fields
> +            num_ip_layers = packet.layers().count(IP)
> +
> +            # Update the last IP layer if there are multiple to account for GRE addressing (the

This comment should be more generic as it's going to apply to other 
things than just GRE.

> +            # framework should be modifying the packet address instead of the tunnel).
> +            l3_to_use = packet.getlayer(IP, num_ip_layers)

Would this make more sense inside the positive if branch right below?

> +            if num_ip_layers > 0:
> +                ip_src_is_unset = "src" not in l3_to_use.fields
> +                ip_dst_is_unset = "dst" not in l3_to_use.fields
> +            else:
> +                ip_src_is_unset = None
> +                ip_dst_is_unset = None
>   
> -            # The packet is routed from TG egress to TG ingress
> -            # update l3 addresses
> -            packet.payload.src = self._tg_ip_address_egress.ip.exploded
> -            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
> -        else:
> -            # The packet leaves TG towards SUT
>               # update l2 addresses
> -            packet.src = self._tg_port_egress.mac_address
> -            packet.dst = self._sut_port_ingress.mac_address
> +            # If `expected` is :data:`True`, the packet enters the TG from SUT, otherwise the
> +            # packet leaves the TG towards the SUT
> +            if pkt_src_is_unset:
> +                packet.src = (
> +                    self._sut_port_egress.mac_address
> +                    if expected
> +                    else self._tg_port_egress.mac_address
> +                )
> +            if pkt_dst_is_unset:
> +                packet.dst = (
> +                    self._tg_port_ingress.mac_address
> +                    if expected
> +                    else self._sut_port_ingress.mac_address
> +                )
>   
> -            # The packet is routed from TG egress to TG ingress
>               # update l3 addresses
> -            packet.payload.src = self._tg_ip_address_egress.ip.exploded
> -            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
> +            # The packet is routed from TG egress to TG ingress regardless of whether it is
> +            # expected or not.

Is this true? This might've been an error in the original 
implementation. If it's expected (that is, the returning packet), it 
should be routed from TG ingress to TG egress, no?

> +            if ip_src_is_unset:
> +                l3_to_use.src = self._tg_ip_address_egress.ip.exploded
> +
> +            if ip_dst_is_unset:
> +                l3_to_use.dst = self._tg_ip_address_ingress.ip.exploded
> +            ret_packets.append(Ether(packet.build()))
>   

Re: [PATCH v2 1/1] dts: add send_packets to test suites and rework packet addressing

[Jeremy Spewock]

On Tue, Sep 24, 2024 at 10:30 AM Juraj Linkeš
<juraj.linkes@pantheon.tech> wrote:
>
>
>
> On 20. 9. 2024 20:08, jspewock@iol.unh.edu wrote:
> > From: Jeremy Spewock <jspewock@iol.unh.edu>
> >
> > Currently the only method provided in the test suite class for sending
> > packets sends a single packet and then captures the results. There is,
> > in some cases, a need to send multiple packets at once while not really
> > needing to capture any traffic received back. The method to do this
> > exists in the traffic generator already, but this patch exposes the
> > method to test suites.
> >
>
> Patrick mentioned there is now a method that does it
> (send_packets_and_capture()), but that one captures the packets. I think
> we should add this method though, as it does something different, but
> maybe the most important point is that non-capturing TGs are going to
> support send_packets_and_capture(), so we need it either way.
>
> > This patch also updates the _adjust_addresses method of test suites so
> > that addresses of packets are only modified if the developer did not
> > configure them beforehand. This allows for developers to have more
> > control over the content of their packets when sending them through the
> > framework.
> >
>
> I think this could and should be split into two patches. They aren't
> really connected.

Sure, this makes sense. They were in the same one originally when this
was tied to the test suite I was writing it for because they were just
smaller framework updates that allowed the suite to function. Probably
didn't make much sense to keep them together then, but it makes even
less sense when the patch is standalone like this.

>
>
> > diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
>
> > @@ -243,41 +255,74 @@ def get_expected_packet(self, packet: Packet) -> Packet:
> >           Returns:
> >               `packet` with injected L2/L3 addresses.
> >           """
> > -        return self._adjust_addresses(packet, expected=True)
> > +        return self._adjust_addresses([packet], expected=True)[0]
> >
> > -    def _adjust_addresses(self, packet: Packet, expected: bool = False) -> Packet:
> > +    def _adjust_addresses(self, packets: list[Packet], expected: bool = False) -> list[Packet]:
> >           """L2 and L3 address additions in both directions.
> >
> > +        Packets in `packets` will be directly modified in this method. The returned list of packets
> > +        however will be copies of the modified packets in order to keep the two lists distinct.
> > +
>
> Do we need to do this? I guess you needed this in a test suite - what is
> the reason?

This is actually just me documenting something that was already
happening behind the scenes in this method that I thought would be
helpful for people to know. Right now this method ends in
`Ether(packet.build())` which essentially just copies the packet, but
it does so after the modifications were made. I think there is some
logical reason to do this which is this method is called internally in
the framework, so it allows the send methods where these packets are
used to essentially modify the list and do whatever they want to/need
to to get the packets sent and the only change that is reflected on
the user is the addresses are added.

>
> > +        Only missing addresses are added to packets, existing addresses will not be overridden. If
> > +        any packet in `packets` has multiple IP layers (using GRE, for example) only the inner-most
> > +        IP layer will have its addresses adjusted.
> > +
> >           Assumptions:
> >               Two links between SUT and TG, one link is TG -> SUT, the other SUT -> TG.
> >
> >           Args:
> > -            packet: The packet to modify.
> > +            packets: The packets to modify.
> >               expected: If :data:`True`, the direction is SUT -> TG,
> >                   otherwise the direction is TG -> SUT.
> > +
> > +        Returns:
> > +            A list containing copies of all packets in `packets` after modification.
> >           """
> > -        if expected:
> > -            # The packet enters the TG from SUT
> > -            # update l2 addresses
> > -            packet.src = self._sut_port_egress.mac_address
> > -            packet.dst = self._tg_port_ingress.mac_address
> > +        ret_packets = []
> > +        for packet in packets:
> > +            # The fields parameter of a packet does not include fields of the payload, so this can
> > +            # only be the Ether src/dst.
> > +            pkt_src_is_unset = "src" not in packet.fields
> > +            pkt_dst_is_unset = "dst" not in packet.fields
> > +            num_ip_layers = packet.layers().count(IP)
> > +
> > +            # Update the last IP layer if there are multiple to account for GRE addressing (the
>
> This comment should be more generic as it's going to apply to other
> things than just GRE.

Ack.

>
> > +            # framework should be modifying the packet address instead of the tunnel).
> > +            l3_to_use = packet.getlayer(IP, num_ip_layers)
>
> Would this make more sense inside the positive if branch right below?

Yeah, it could be defined in the positive branch. The main reason I
left it out was just because it was also used later in the method, but
the only way that part of the code is reached is if this positive
branch is hit anyway. I can update it.

>
> > +            if num_ip_layers > 0:
> > +                ip_src_is_unset = "src" not in l3_to_use.fields
> > +                ip_dst_is_unset = "dst" not in l3_to_use.fields
> > +            else:
> > +                ip_src_is_unset = None
> > +                ip_dst_is_unset = None
> >
> > -            # The packet is routed from TG egress to TG ingress
> > -            # update l3 addresses
> > -            packet.payload.src = self._tg_ip_address_egress.ip.exploded
> > -            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
> > -        else:
> > -            # The packet leaves TG towards SUT
> >               # update l2 addresses
> > -            packet.src = self._tg_port_egress.mac_address
> > -            packet.dst = self._sut_port_ingress.mac_address
> > +            # If `expected` is :data:`True`, the packet enters the TG from SUT, otherwise the
> > +            # packet leaves the TG towards the SUT
> > +            if pkt_src_is_unset:
> > +                packet.src = (
> > +                    self._sut_port_egress.mac_address
> > +                    if expected
> > +                    else self._tg_port_egress.mac_address
> > +                )
> > +            if pkt_dst_is_unset:
> > +                packet.dst = (
> > +                    self._tg_port_ingress.mac_address
> > +                    if expected
> > +                    else self._sut_port_ingress.mac_address
> > +                )
> >
> > -            # The packet is routed from TG egress to TG ingress
> >               # update l3 addresses
> > -            packet.payload.src = self._tg_ip_address_egress.ip.exploded
> > -            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
> > +            # The packet is routed from TG egress to TG ingress regardless of whether it is
> > +            # expected or not.
>
> Is this true? This might've been an error in the original
> implementation. If it's expected (that is, the returning packet), it
> should be routed from TG ingress to TG egress, no?

I guess I'm not completely sure. It would make sense that the L3
addresses should be switched as well based on either expected or not,
but currently it isn't modified, and os_udp still works which makes me
think the addresses aren't switched by the kernel interfaces, which I
believe is the only time these addresses are actually used (since we
use sendp in scapy).

>
> > +            if ip_src_is_unset:
> > +                l3_to_use.src = self._tg_ip_address_egress.ip.exploded
> > +
> > +            if ip_dst_is_unset:
> > +                l3_to_use.dst = self._tg_ip_address_ingress.ip.exploded
> > +            ret_packets.append(Ether(packet.build()))
> >
>
>

Re: [PATCH v2 1/1] dts: add send_packets to test suites and rework packet addressing

[Juraj Linkeš]

>>> +            if num_ip_layers > 0:
>>> +                ip_src_is_unset = "src" not in l3_to_use.fields
>>> +                ip_dst_is_unset = "dst" not in l3_to_use.fields
>>> +            else:
>>> +                ip_src_is_unset = None
>>> +                ip_dst_is_unset = None
>>>
>>> -            # The packet is routed from TG egress to TG ingress
>>> -            # update l3 addresses
>>> -            packet.payload.src = self._tg_ip_address_egress.ip.exploded
>>> -            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
>>> -        else:
>>> -            # The packet leaves TG towards SUT
>>>                # update l2 addresses
>>> -            packet.src = self._tg_port_egress.mac_address
>>> -            packet.dst = self._sut_port_ingress.mac_address
>>> +            # If `expected` is :data:`True`, the packet enters the TG from SUT, otherwise the
>>> +            # packet leaves the TG towards the SUT
>>> +            if pkt_src_is_unset:
>>> +                packet.src = (
>>> +                    self._sut_port_egress.mac_address
>>> +                    if expected
>>> +                    else self._tg_port_egress.mac_address
>>> +                )
>>> +            if pkt_dst_is_unset:
>>> +                packet.dst = (
>>> +                    self._tg_port_ingress.mac_address
>>> +                    if expected
>>> +                    else self._sut_port_ingress.mac_address
>>> +                )
>>>
>>> -            # The packet is routed from TG egress to TG ingress
>>>                # update l3 addresses
>>> -            packet.payload.src = self._tg_ip_address_egress.ip.exploded
>>> -            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
>>> +            # The packet is routed from TG egress to TG ingress regardless of whether it is
>>> +            # expected or not.
>>
>> Is this true? This might've been an error in the original
>> implementation. If it's expected (that is, the returning packet), it
>> should be routed from TG ingress to TG egress, no?
> 
> I guess I'm not completely sure. It would make sense that the L3
> addresses should be switched as well based on either expected or not,
> but currently it isn't modified, and os_udp still works which makes me
> think the addresses aren't switched by the kernel interfaces, which I
> believe is the only time these addresses are actually used (since we
> use sendp in scapy).
> 

Right, I went too deep in my thinking and confused myself :-). The path 
of the packet is:
TG egress -> SUT ingress -> SUT egress -> TG ingress

That is just one way path and I mixed the other direction into my 
thinking as well. With just one path, the MAC addresses are going to 
change, but not IP addresses.

Re: [PATCH v2 1/1] dts: add send_packets to test suites and rework packet addressing

[Juraj Linkeš]

>>> diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
>>
>>> @@ -243,41 +255,74 @@ def get_expected_packet(self, packet: Packet) -> Packet:
>>>            Returns:
>>>                `packet` with injected L2/L3 addresses.
>>>            """
>>> -        return self._adjust_addresses(packet, expected=True)
>>> +        return self._adjust_addresses([packet], expected=True)[0]
>>>
>>> -    def _adjust_addresses(self, packet: Packet, expected: bool = False) -> Packet:
>>> +    def _adjust_addresses(self, packets: list[Packet], expected: bool = False) -> list[Packet]:
>>>            """L2 and L3 address additions in both directions.
>>>
>>> +        Packets in `packets` will be directly modified in this method. The returned list of packets
>>> +        however will be copies of the modified packets in order to keep the two lists distinct.
>>> +
>>
>> Do we need to do this? I guess you needed this in a test suite - what is
>> the reason?
> 
> This is actually just me documenting something that was already
> happening behind the scenes in this method that I thought would be
> helpful for people to know. Right now this method ends in
> `Ether(packet.build())` which essentially just copies the packet, but
> it does so after the modifications were made. I think there is some
> logical reason to do this

Ah, ok, this only documents what already existed. I think this was here 
because packet.build() doesn't return a Packet so a conversion is 
needed. The intention is not to keep the two lists distinct.

> which is this method is called internally in
> the framework, so it allows the send methods where these packets are
> used to essentially modify the list and do whatever they want to/need
> to to get the packets sent

There are two usages for this method:
1. To update the packet before it's sent out (so that the proper 
addresses are used),
2. To update a packet, constructed in a test case, that we expect to 
receive, so as to compare with actual received packets. This is used in 
the OS UDP test case which checks that the MAC addresses are updated 
properly (and IP addresses stay the same).

> and the only change that is reflected on
> the user is the addresses are added.
> 

[PATCH v3 0/1] dts: adjust packet addressing

[jspewock]

From: Jeremy Spewock <jspewock@iol.unh.edu>

v3:
 * split send_packets into a different patch
 * updated code comments
 * moved variable delcaration

Jeremy Spewock (1):
  dts: rework packet addressing

 dts/framework/test_suite.py | 75 ++++++++++++++++++++++++++-----------
 1 file changed, 54 insertions(+), 21 deletions(-)

-- 
2.46.0

[PATCH v3 1/1] dts: rework packet addressing

[jspewock]

From: Jeremy Spewock <jspewock@iol.unh.edu>

This patch updates the _adjust_addresses method of test suites so
that addresses of packets are only modified if the developer did not
configure them beforehand. This allows for developers to have more
control over the content of their packets when sending them through the
framework.

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/framework/test_suite.py | 75 ++++++++++++++++++++++++++-----------
 1 file changed, 54 insertions(+), 21 deletions(-)

diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
index 051509fb86..69388ff5ab 100644
--- a/dts/framework/test_suite.py
+++ b/dts/framework/test_suite.py
@@ -225,7 +225,7 @@ def send_packets_and_capture(
         Returns:
             A list of received packets.
         """
-        packets = [self._adjust_addresses(packet) for packet in packets]
+        packets = self._adjust_addresses(packets)
         return self.tg_node.send_packets_and_capture(
             packets,
             self._tg_port_egress,
@@ -243,41 +243,74 @@ def get_expected_packet(self, packet: Packet) -> Packet:
         Returns:
             `packet` with injected L2/L3 addresses.
         """
-        return self._adjust_addresses(packet, expected=True)
+        return self._adjust_addresses([packet], expected=True)[0]
 
-    def _adjust_addresses(self, packet: Packet, expected: bool = False) -> Packet:
+    def _adjust_addresses(self, packets: list[Packet], expected: bool = False) -> list[Packet]:
         """L2 and L3 address additions in both directions.
 
+        Packets in `packets` will be directly modified in this method. The returned list of packets
+        however will be copies of the modified packets in order to keep the two lists distinct.
+
+        Only missing addresses are added to packets, existing addresses will not be overridden. If
+        any packet in `packets` has multiple IP layers (using GRE, for example) only the inner-most
+        IP layer will have its addresses adjusted.
+
         Assumptions:
             Two links between SUT and TG, one link is TG -> SUT, the other SUT -> TG.
 
         Args:
-            packet: The packet to modify.
+            packets: The packets to modify.
             expected: If :data:`True`, the direction is SUT -> TG,
                 otherwise the direction is TG -> SUT.
+
+        Returns:
+            A list containing copies of all packets in `packets` after modification.
         """
-        if expected:
-            # The packet enters the TG from SUT
-            # update l2 addresses
-            packet.src = self._sut_port_egress.mac_address
-            packet.dst = self._tg_port_ingress.mac_address
+        ret_packets = []
+        for packet in packets:
+            # The fields parameter of a packet does not include fields of the payload, so this can
+            # only be the Ether src/dst.
+            pkt_src_is_unset = "src" not in packet.fields
+            pkt_dst_is_unset = "dst" not in packet.fields
+            num_ip_layers = packet.layers().count(IP)
+
+            if num_ip_layers > 0:
+                # Update the last IP layer if there are multiple (the framework should be modifying
+                # the packet address instead of the tunnel address if there is one).
+                l3_to_use = packet.getlayer(IP, num_ip_layers)
+                ip_src_is_unset = "src" not in l3_to_use.fields
+                ip_dst_is_unset = "dst" not in l3_to_use.fields
+            else:
+                ip_src_is_unset = None
+                ip_dst_is_unset = None
 
-            # The packet is routed from TG egress to TG ingress
-            # update l3 addresses
-            packet.payload.src = self._tg_ip_address_egress.ip.exploded
-            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
-        else:
-            # The packet leaves TG towards SUT
             # update l2 addresses
-            packet.src = self._tg_port_egress.mac_address
-            packet.dst = self._sut_port_ingress.mac_address
+            # If `expected` is :data:`True`, the packet enters the TG from SUT, otherwise the
+            # packet leaves the TG towards the SUT
+            if pkt_src_is_unset:
+                packet.src = (
+                    self._sut_port_egress.mac_address
+                    if expected
+                    else self._tg_port_egress.mac_address
+                )
+            if pkt_dst_is_unset:
+                packet.dst = (
+                    self._tg_port_ingress.mac_address
+                    if expected
+                    else self._sut_port_ingress.mac_address
+                )
 
-            # The packet is routed from TG egress to TG ingress
             # update l3 addresses
-            packet.payload.src = self._tg_ip_address_egress.ip.exploded
-            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
+            # The packet is routed from TG egress to TG ingress regardless of whether it is
+            # expected or not.
+            if ip_src_is_unset:
+                l3_to_use.src = self._tg_ip_address_egress.ip.exploded
+
+            if ip_dst_is_unset:
+                l3_to_use.dst = self._tg_ip_address_ingress.ip.exploded
+            ret_packets.append(Ether(packet.build()))
 
-        return Ether(packet.build())
+        return ret_packets
 
     def verify(self, condition: bool, failure_description: str) -> None:
         """Verify `condition` and handle failures.
-- 
2.46.0

Re: [PATCH v3 1/1] dts: rework packet addressing

[Juraj Linkeš]

> diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py

> +    def _adjust_addresses(self, packets: list[Packet], expected: bool = False) -> list[Packet]:
> +            # The packet is routed from TG egress to TG ingress regardless of whether it is
> +            # expected or not.
> +            if ip_src_is_unset:
> +                l3_to_use.src = self._tg_ip_address_egress.ip.exploded
> +
> +            if ip_dst_is_unset:
> +                l3_to_use.dst = self._tg_ip_address_ingress.ip.exploded

So this is where l3_to_use also appears. This could also be in the same 
if branch, right? As you mentioned, ip_src_is_unset is only going to be 
true in that branch.

Now that I look at it, we're mixing the update of l2 addresses (starting 
with pkt_src_is_unset = "src" not in packet.fields) with l3 addresses 
(starting with num_ip_layers right below that). We could first do l2 
addresses, then l3 addresses. And I don't think we even need the 
*_is_unset variables, they're only used once.

> +            ret_packets.append(Ether(packet.build()))
>   
> -        return Ether(packet.build())
> +        return ret_packets
>   
>       def verify(self, condition: bool, failure_description: str) -> None:
>           """Verify `condition` and handle failures.

Re: [PATCH v3 1/1] dts: rework packet addressing

[Jeremy Spewock]

On Thu, Sep 26, 2024 at 8:31 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
>
> > diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
>
> > +    def _adjust_addresses(self, packets: list[Packet], expected: bool = False) -> list[Packet]:
> > +            # The packet is routed from TG egress to TG ingress regardless of whether it is
> > +            # expected or not.
> > +            if ip_src_is_unset:
> > +                l3_to_use.src = self._tg_ip_address_egress.ip.exploded
> > +
> > +            if ip_dst_is_unset:
> > +                l3_to_use.dst = self._tg_ip_address_ingress.ip.exploded
>
> So this is where l3_to_use also appears. This could also be in the same
> if branch, right? As you mentioned, ip_src_is_unset is only going to be
> true in that branch.
>
> Now that I look at it, we're mixing the update of l2 addresses (starting
> with pkt_src_is_unset = "src" not in packet.fields) with l3 addresses
> (starting with num_ip_layers right below that). We could first do l2
> addresses, then l3 addresses. And I don't think we even need the
> *_is_unset variables, they're only used once.

That is true, I can change it to work in this way instead.

>
> > +            ret_packets.append(Ether(packet.build()))
> >
> > -        return Ether(packet.build())
> > +        return ret_packets
> >
> >       def verify(self, condition: bool, failure_description: str) -> None:
> >           """Verify `condition` and handle failures.
>

[PATCH v4 0/1] dts: adjust packet addressing

[jspewock]

From: Jeremy Spewock <jspewock@iol.unh.edu>

v4:
 * changed _adjust_addresses so that it doesn't use unneeded variables
 * modify doc-strings

Jeremy Spewock (1):
  dts: rework packet addressing

 dts/framework/test_suite.py | 71 +++++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 23 deletions(-)

-- 
2.46.0

[PATCH v4 1/1] dts: rework packet addressing

[jspewock]

From: Jeremy Spewock <jspewock@iol.unh.edu>

This patch updates the _adjust_addresses method of test suites so
that addresses of packets are only modified if the developer did not
configure them beforehand. This allows for developers to have more
control over the content of their packets when sending them through the
framework.

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/framework/test_suite.py | 71 +++++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 23 deletions(-)

diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
index 051509fb86..b2d8de99e7 100644
--- a/dts/framework/test_suite.py
+++ b/dts/framework/test_suite.py
@@ -225,7 +225,7 @@ def send_packets_and_capture(
         Returns:
             A list of received packets.
         """
-        packets = [self._adjust_addresses(packet) for packet in packets]
+        packets = self._adjust_addresses(packets)
         return self.tg_node.send_packets_and_capture(
             packets,
             self._tg_port_egress,
@@ -243,41 +243,66 @@ def get_expected_packet(self, packet: Packet) -> Packet:
         Returns:
             `packet` with injected L2/L3 addresses.
         """
-        return self._adjust_addresses(packet, expected=True)
+        return self._adjust_addresses([packet], expected=True)[0]
 
-    def _adjust_addresses(self, packet: Packet, expected: bool = False) -> Packet:
+    def _adjust_addresses(self, packets: list[Packet], expected: bool = False) -> list[Packet]:
         """L2 and L3 address additions in both directions.
 
+        Packets in `packets` will be directly modified in this method. The returned list of packets
+        however will be copies of the modified packets.
+
+        Only missing addresses are added to packets, existing addresses will not be overridden. If
+        any packet in `packets` has multiple IP layers (using GRE, for example) only the inner-most
+        IP layer will have its addresses adjusted.
+
         Assumptions:
             Two links between SUT and TG, one link is TG -> SUT, the other SUT -> TG.
 
         Args:
-            packet: The packet to modify.
+            packets: The packets to modify.
             expected: If :data:`True`, the direction is SUT -> TG,
                 otherwise the direction is TG -> SUT.
-        """
-        if expected:
-            # The packet enters the TG from SUT
-            # update l2 addresses
-            packet.src = self._sut_port_egress.mac_address
-            packet.dst = self._tg_port_ingress.mac_address
 
-            # The packet is routed from TG egress to TG ingress
-            # update l3 addresses
-            packet.payload.src = self._tg_ip_address_egress.ip.exploded
-            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
-        else:
-            # The packet leaves TG towards SUT
+        Returns:
+            A list containing copies of all packets in `packets` after modification.
+        """
+        ret_packets = []
+        for packet in packets:
             # update l2 addresses
-            packet.src = self._tg_port_egress.mac_address
-            packet.dst = self._sut_port_ingress.mac_address
+            # If `expected` is :data:`True`, the packet enters the TG from SUT, otherwise the
+            # packet leaves the TG towards the SUT.
+
+            # The fields parameter of a packet does not include fields of the payload, so this can
+            # only be the Ether src/dst.
+            if "src" not in packet.fields:
+                packet.src = (
+                    self._sut_port_egress.mac_address
+                    if expected
+                    else self._tg_port_egress.mac_address
+                )
+            if "dst" not in packet.fields:
+                packet.dst = (
+                    self._tg_port_ingress.mac_address
+                    if expected
+                    else self._sut_port_ingress.mac_address
+                )
 
-            # The packet is routed from TG egress to TG ingress
             # update l3 addresses
-            packet.payload.src = self._tg_ip_address_egress.ip.exploded
-            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
-
-        return Ether(packet.build())
+            # The packet is routed from TG egress to TG ingress regardless of whether it is
+            # expected or not.
+            num_ip_layers = packet.layers().count(IP)
+            if num_ip_layers > 0:
+                # Update the last IP layer if there are multiple (the framework should be modifying
+                # the packet address instead of the tunnel address if there is one).
+                l3_to_use = packet.getlayer(IP, num_ip_layers)
+                if "src" not in l3_to_use.fields:
+                    l3_to_use.src = self._tg_ip_address_egress.ip.exploded
+
+                if "dst" not in l3_to_use.fields:
+                    l3_to_use.dst = self._tg_ip_address_ingress.ip.exploded
+            ret_packets.append(Ether(packet.build()))
+
+        return ret_packets
 
     def verify(self, condition: bool, failure_description: str) -> None:
         """Verify `condition` and handle failures.
-- 
2.46.0

[PATCH v5 0/2] dts: adjust packet addressing and add send_packets to test_suite

[jspewock]

From: Jeremy Spewock <jspewock@iol.unh.edu>

v5:
 * add send_packets patch back into this series but as its own patch

Jeremy Spewock (2):
  dts: rework packet addressing
  dts: add send_packets to test_suite

 dts/framework/test_suite.py            | 83 +++++++++++++++++++-------
 dts/framework/testbed_model/tg_node.py |  9 +++
 2 files changed, 69 insertions(+), 23 deletions(-)

-- 
2.46.0

[PATCH v5 1/2] dts: rework packet addressing

[jspewock]

From: Jeremy Spewock <jspewock@iol.unh.edu>

This patch updates the _adjust_addresses method of test suites so
that addresses of packets are only modified if the developer did not
configure them beforehand. This allows for developers to have more
control over the content of their packets when sending them through the
framework.

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/framework/test_suite.py | 71 +++++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 23 deletions(-)

diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
index 051509fb86..b2d8de99e7 100644
--- a/dts/framework/test_suite.py
+++ b/dts/framework/test_suite.py
@@ -225,7 +225,7 @@ def send_packets_and_capture(
         Returns:
             A list of received packets.
         """
-        packets = [self._adjust_addresses(packet) for packet in packets]
+        packets = self._adjust_addresses(packets)
         return self.tg_node.send_packets_and_capture(
             packets,
             self._tg_port_egress,
@@ -243,41 +243,66 @@ def get_expected_packet(self, packet: Packet) -> Packet:
         Returns:
             `packet` with injected L2/L3 addresses.
         """
-        return self._adjust_addresses(packet, expected=True)
+        return self._adjust_addresses([packet], expected=True)[0]
 
-    def _adjust_addresses(self, packet: Packet, expected: bool = False) -> Packet:
+    def _adjust_addresses(self, packets: list[Packet], expected: bool = False) -> list[Packet]:
         """L2 and L3 address additions in both directions.
 
+        Packets in `packets` will be directly modified in this method. The returned list of packets
+        however will be copies of the modified packets.
+
+        Only missing addresses are added to packets, existing addresses will not be overridden. If
+        any packet in `packets` has multiple IP layers (using GRE, for example) only the inner-most
+        IP layer will have its addresses adjusted.
+
         Assumptions:
             Two links between SUT and TG, one link is TG -> SUT, the other SUT -> TG.
 
         Args:
-            packet: The packet to modify.
+            packets: The packets to modify.
             expected: If :data:`True`, the direction is SUT -> TG,
                 otherwise the direction is TG -> SUT.
-        """
-        if expected:
-            # The packet enters the TG from SUT
-            # update l2 addresses
-            packet.src = self._sut_port_egress.mac_address
-            packet.dst = self._tg_port_ingress.mac_address
 
-            # The packet is routed from TG egress to TG ingress
-            # update l3 addresses
-            packet.payload.src = self._tg_ip_address_egress.ip.exploded
-            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
-        else:
-            # The packet leaves TG towards SUT
+        Returns:
+            A list containing copies of all packets in `packets` after modification.
+        """
+        ret_packets = []
+        for packet in packets:
             # update l2 addresses
-            packet.src = self._tg_port_egress.mac_address
-            packet.dst = self._sut_port_ingress.mac_address
+            # If `expected` is :data:`True`, the packet enters the TG from SUT, otherwise the
+            # packet leaves the TG towards the SUT.
+
+            # The fields parameter of a packet does not include fields of the payload, so this can
+            # only be the Ether src/dst.
+            if "src" not in packet.fields:
+                packet.src = (
+                    self._sut_port_egress.mac_address
+                    if expected
+                    else self._tg_port_egress.mac_address
+                )
+            if "dst" not in packet.fields:
+                packet.dst = (
+                    self._tg_port_ingress.mac_address
+                    if expected
+                    else self._sut_port_ingress.mac_address
+                )
 
-            # The packet is routed from TG egress to TG ingress
             # update l3 addresses
-            packet.payload.src = self._tg_ip_address_egress.ip.exploded
-            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
-
-        return Ether(packet.build())
+            # The packet is routed from TG egress to TG ingress regardless of whether it is
+            # expected or not.
+            num_ip_layers = packet.layers().count(IP)
+            if num_ip_layers > 0:
+                # Update the last IP layer if there are multiple (the framework should be modifying
+                # the packet address instead of the tunnel address if there is one).
+                l3_to_use = packet.getlayer(IP, num_ip_layers)
+                if "src" not in l3_to_use.fields:
+                    l3_to_use.src = self._tg_ip_address_egress.ip.exploded
+
+                if "dst" not in l3_to_use.fields:
+                    l3_to_use.dst = self._tg_ip_address_ingress.ip.exploded
+            ret_packets.append(Ether(packet.build()))
+
+        return ret_packets
 
     def verify(self, condition: bool, failure_description: str) -> None:
         """Verify `condition` and handle failures.
-- 
2.46.0

Re: [PATCH v5 1/2] dts: rework packet addressing

[Juraj Linkeš]

On 26. 9. 2024 20:18, jspewock@iol.unh.edu wrote:
> From: Jeremy Spewock <jspewock@iol.unh.edu>
> 
> This patch updates the _adjust_addresses method of test suites so
> that addresses of packets are only modified if the developer did not
> configure them beforehand. This allows for developers to have more
> control over the content of their packets when sending them through the
> framework.
> 
> Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> ---

Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech>

Re: [PATCH v5 1/2] dts: rework packet addressing

[Luca Vizzarro]

Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>

[PATCH v5 2/2] dts: add send_packets to test_suite

[jspewock]

From: Jeremy Spewock <jspewock@iol.unh.edu>

Currently the only methods provided in the test suite class for sending
packets capture the resulting received traffic after sending. There is,
in some cases, a need to send multiple packets at once while not really
needing to capture any of said received traffic. It is favorable to
avoid capturing received traffic when you don't need it since not all
traffic generators will necessarily be capturing traffic generators.
The method to fulfill this need exists in the traffic generator
already, but this patch exposes the method to test suites.

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
Reviewed-by: Patrick Robb <probb@iol.unh.edu>
---
 dts/framework/test_suite.py            | 12 ++++++++++++
 dts/framework/testbed_model/tg_node.py |  9 +++++++++
 2 files changed, 21 insertions(+)

diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
index b2d8de99e7..159071d0e2 100644
--- a/dts/framework/test_suite.py
+++ b/dts/framework/test_suite.py
@@ -234,6 +234,18 @@ def send_packets_and_capture(
             duration,
         )
 
+    def send_packets(
+        self,
+        packets: list[Packet],
+    ) -> None:
+        """Send packets using the traffic generator and do not capture received traffic.
+
+        Args:
+            packets: Packets to send.
+        """
+        packets = self._adjust_addresses(packets)
+        self.tg_node.send_packets(packets, self._tg_port_egress)
+
     def get_expected_packet(self, packet: Packet) -> Packet:
         """Inject the proper L2/L3 addresses into `packet`.
 
diff --git a/dts/framework/testbed_model/tg_node.py b/dts/framework/testbed_model/tg_node.py
index 19b5b6e74c..4179365abb 100644
--- a/dts/framework/testbed_model/tg_node.py
+++ b/dts/framework/testbed_model/tg_node.py
@@ -83,6 +83,15 @@ def send_packets_and_capture(
             duration,
         )
 
+    def send_packets(self, packets: list[Packet], port: Port):
+        """Send packets without capturing resulting received packets.
+
+        Args:
+            packets: Packets to send.
+            port: Port to send the packets on.
+        """
+        self.traffic_generator.send_packets(packets, port)
+
     def close(self) -> None:
         """Free all resources used by the node.
 
-- 
2.46.0

Re: [PATCH v5 2/2] dts: add send_packets to test_suite

[Luca Vizzarro]

Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>

Re: [PATCH v5 0/2] dts: adjust packet addressing and add send_packets to test_suite

[Juraj Linkeš]

On 26. 9. 2024 20:18, jspewock@iol.unh.edu wrote:
> From: Jeremy Spewock <jspewock@iol.unh.edu>
> 
> v5:
>   * add send_packets patch back into this series but as its own patch
> 
> Jeremy Spewock (2):
>    dts: rework packet addressing
>    dts: add send_packets to test_suite
> 
>   dts/framework/test_suite.py            | 83 +++++++++++++++++++-------
>   dts/framework/testbed_model/tg_node.py |  9 +++
>   2 files changed, 69 insertions(+), 23 deletions(-)
> 

Applied to next-dts, thanks.