[#5] Implements reply and request forward for icmp #202

John15321 · 2021-04-21T10:45:26Z

Implements echo reply and echo forward request in ICMP

CLAassistant · 2021-04-21T10:45:31Z

All committers have signed the CLA.

src/Renode/Network/NetworkServer/NetworkServer.cs

PiotrZierhoffer · 2021-04-26T07:56:32Z

src/Renode/Network/NetworkServer/IServerModule.cs

+
+    public interface IIcmpServerModule : IServerModule
+    {
+        void HandleIcmp(IPv4Packet packet);


I would consider having a similar API to UDP - with source, with packet, with callback

This still holds

PiotrZierhoffer · 2021-04-27T14:42:50Z

That was one busted review - I didn't notice it stayed as "wip" (that is - not published on gh) and you pushed new commits. Sorry for the confusion

src/Renode/Network/NetworkServer/NetworkServer.cs

PiotrZierhoffer · 2021-04-27T14:46:18Z

src/Renode/Network/NetworkServer/NetworkServer.cs

-            var src = new IPEndPoint(((IPv4Packet)packet.ParentPacket).SourceAddress, packet.SourcePort); 
-            module.HandleUdp(src, packet, (s, r) => HandleUdpResponse(s, r));
+            var src = new IPEndPoint(
+                ((IPv4Packet) packet.ParentPacket).SourceAddress,


we usually don't add spaces after casts

((IPv4Packet)packet.ParentPacket).SourceAddress

also scan other places

src/Renode/Network/NetworkServer/NetworkServer.cs

PiotrZierhoffer · 2021-05-05T09:14:21Z

src/Renode/Network/NetworkServer/NetworkServer.cs

+
+        public MACAddress MAC { get; set; }
+        public IPAddress IP { get; set; }
+        public Dictionary<IPAddress, PhysicalAddress> arpTable;


PiotrZierhoffer · 2021-05-05T09:14:32Z

src/Renode/Network/NetworkServer/NetworkServer.cs

+        public IPAddress IP { get; set; }
+        public Dictionary<IPAddress, PhysicalAddress> arpTable;
+
+        public void HandleIcmpPacket(Action<EthernetFrame> FrameReady, IPv4Packet packet)


add destinationMac as a parameter

maybe we should use ICMPv4Packet as a parameter? HandleUDP gets the UDPPacket

src/Renode/Network/NetworkServer/NetworkServer.cs

PiotrZierhoffer · 2021-05-05T09:14:58Z

src/Renode/Network/NetworkServer/NetworkServer.cs

            {
                this.Log(LogLevel.Warning, "Unsupported ARP packet: {0}", packet);
                return false;
            }

-            if(!packet.TargetProtocolAddress.Equals(IP))
+            if (!packet.TargetProtocolAddress.Equals(IP))


please drop these formatting changes

PiotrZierhoffer · 2021-05-05T09:16:21Z

src/Renode/Network/NetworkServer/NetworkServer.cs

+
+            // We create an ICMP Response and Destination address to which
+            // the response will be sent
+            var icmpResponse = ICMPv4TypeCodes.EchoReply.AsRawBytes();


maybe move this near icmpPacketResponse

PiotrZierhoffer · 2021-05-05T09:26:46Z

src/Renode/Network/NetworkServer/NetworkServer.cs

+            // we put that in the Ethernet frame, and recalculate the checksum
+            ipPacket.PayloadPacket = icmpPacketResponse;
+            ethernetResponse.PayloadPacket = ipPacket;
+            icmpPacketResponse.UpdateCalculatedValues();


rewrite missing fields from the request - data, id, seq etc.

src/Renode/Network/NetworkServer/NetworkServer.cs

src/Renode/Network/NetworkServer/Modules/IcmpServerModule.cs

src/Renode/Network/NetworkServer/NetworkServer.cs

PiotrZierhoffer · 2021-05-17T21:46:57Z

src/Renode/Network/NetworkServer/NetworkServer.cs

@@ -137,7 +136,7 @@ public void ReceiveFrame(EthernetFrame frame)

        public MACAddress MAC { get; set; }
        public IPAddress IP { get; set; }
-
+        public IcmpServerModule icmpModule;


any reason for this to be public?

PiotrZierhoffer · 2021-05-17T21:50:45Z

src/Renode/Network/NetworkServer/NetworkServer.cs

@@ -60,6 +57,8 @@ public NetworkServer(string ipAddress, string macAddress = null)
            modules = new Dictionary<int, IServerModule>();
            modulesNames = new Dictionary<string, int>();

+            IcmpServerModule icmpModule = new IcmpServerModule(IP,MAC);


you are creating a local variable, not initializing a field

also, a space before MAC

PiotrZierhoffer · 2021-05-17T21:57:36Z

src/Renode/Network/NetworkServer/Modules/IcmpServerModule.cs

+        }
+
+
+


drop the excess empty lines

PiotrZierhoffer · 2021-05-17T21:58:31Z

src/Renode/Network/NetworkServer/Modules/IcmpServerModule.cs

+
+
+        private MACAddress MAC { get; set; }
+        private IPAddress IP { get; set; }


no need to keep properties for private things. Let's just make these as fields, mac and ip

PiotrZierhoffer · 2021-05-17T21:58:49Z

src/Renode/Network/NetworkServer/Modules/IcmpServerModule.cs

+        /// <param name="FrameReady"></param>
+        /// <param name="ipv4Packet"></param>
+        /// <param name="icmpDestinationAddress"></param>
+        public void HandleIcmpPacket(Action<EthernetFrame> FrameReady, IPv4Packet ipv4Packet,


frameReady

PiotrZierhoffer · 2021-05-17T21:59:02Z

src/Renode/Network/NetworkServer/Modules/IcmpServerModule.cs

+        public void HandleIcmpPacket(Action<EthernetFrame> FrameReady, IPv4Packet ipv4Packet,
+            PhysicalAddress icmpDestinationAddress)
+        {
+


please drop empty line

PiotrZierhoffer · 2021-05-17T21:59:27Z

src/Renode/Network/NetworkServer/Modules/IcmpServerModule.cs

+
+            this.Log(LogLevel.Noisy, "Handling ICMP packet: {0}", (ICMPv4Packet)ipv4Packet.PayloadPacket);
+
+            var ipv4ResponePacket = CreateIPv4Packet(ipv4Packet);


ipv4ResnponsePacket

src/Renode/Network/NetworkServer/NetworkServer.cs

src/Renode/Network/NetworkServer/Modules/IcmpServerModule.cs

PiotrZierhoffer · 2021-05-25T09:57:15Z

@John15321 please mark the conversation points that you consider resolved, clicking the "Resolve conversation" button

PiotrZierhoffer · 2021-05-26T09:13:26Z

src/Renode/Network/NetworkServer/Modules/IcmpServerModule.cs

+        /// <param name="ipv4Packet"></param>
+        /// <param name="icmpPacketResponse"></param>
+        /// <returns></returns>
+        private bool CreateIcmpResponse(IPv4Packet ipv4Packet, out ICMPv4Packet icmpPacketResponse)


ipv4Request

PiotrZierhoffer · 2021-06-02T13:52:21Z

tests/network-server/ICMP/zedboard_icmp.robot

+    Send Key To Uart            0xd
+    Wait For Prompt On Uart     zynq> 
+    Write Line To Uart          ping 192.168.1.11 -w 1
+    Wait For Line On Uart       1 packets transmitted, 1 packets received, 0% packet loss


let's have 5 packets instead. It's a much stronger test - it verifies that you can send and receive a packet, but also recover from it and prepare for subsequent packets. And that your timer (sending the ping every 1 sec) works. Etc.

PiotrZierhoffer · 2021-06-02T13:52:58Z

tests/network-server/ICMP/zedboard_icmp.robot

+    Start Emulation
+
+    Wait For Line On Uart       xemacps e000b000.ps7-ethernet: link up (100/FULL)
+    Send Key To Uart            0xd


Is this really needed? I guess the prompt is there already, so we could probably drop both this and the next line.

PiotrZierhoffer · 2021-06-02T13:53:49Z

tests/network-server/ICMP/zedboard_icmp.robot

+    Wait For Line On Uart       xemacps e000b000.ps7-ethernet: link up (100/FULL)
+    Send Key To Uart            0xd
+    Wait For Prompt On Uart     zynq> 
+    Write Line To Uart          ping 192.168.1.11 -w 1


I assume that there is something preconfigured in the os, like IP? Can we try and set it anyway? It would make the test must more obvious

PiotrZierhoffer · 2021-06-02T13:55:19Z

tests/network-server/ICMP/zedboard_icmp.robot

+Resource                          ${RENODEKEYWORDS}
+
+*** Variables ***
+${SCRIPT}                     scripts/single-node/zedboard.resc


Suggested change

${SCRIPT} scripts/single-node/zedboard.resc

${SCRIPT} ${CURDIR}/../../../scripts/single-node/zedboard.resc

this makes the path work regardless of the way we run the test, but also aligns the formatting a bit

PiotrZierhoffer · 2021-06-02T21:00:02Z

src/Renode/Network/NetworkServer/Modules/IcmpServerModule.cs

+            };
+            for (var i = 0; i < icmpv4PacketResponse.Data.Length; i++) icmpv4PacketResponse.Data[i] = 0;
+
+


drop excess empty lines

PiotrZierhoffer · 2021-06-02T21:00:19Z

src/Renode/Network/NetworkServer/Modules/IcmpServerModule.cs

+        /// <returns></returns>
+        private IPv4Packet CreateIPv4Packet(IPv4Packet ipv4PacketRequest)
+        {
+            ParentServer.Log(LogLevel.Noisy, "Creating IPv4 packet response");


this is redundant

PiotrZierhoffer · 2021-06-02T21:00:36Z

src/Renode/Network/NetworkServer/Modules/IcmpServerModule.cs

+        {
+            ParentServer.Log(LogLevel.Noisy, "Creating IPv4 packet response");
+            var ipv4PacketResponse = new IPv4Packet(IP, ipv4PacketRequest.SourceAddress);
+            ParentServer.Log(LogLevel.Noisy, "Created IPv4 packet response: {0}", ipv4PacketResponse.ToString());


do you need ToString() here?

PiotrZierhoffer · 2021-06-02T21:01:07Z

src/Renode/Network/NetworkServer/Modules/IcmpServerModule.cs

+
+            ethernetResponse.RecursivelyUpdateCalculatedValues(new[] { EthernetPacketType.IpV4 }, new[] { PacketDotNet.IPProtocolType.ICMP });
+
+


drop one empty line

PiotrZierhoffer · 2021-06-02T21:02:50Z

src/Renode/Network/NetworkServer/Modules/IcmpServerModule.cs

+
+
+            if (!EthernetFrame.TryCreateEthernetFrame(ethernetResponse.Bytes,
+                true, out var responseEthernetFrame))


why not just out response?

then you don't have to do response = null

PiotrZierhoffer self-requested a review April 21, 2021 10:47

PiotrZierhoffer reviewed Apr 26, 2021

View reviewed changes

PiotrZierhoffer reviewed Apr 27, 2021

View reviewed changes

PiotrZierhoffer requested changes May 5, 2021

View reviewed changes

John15321 commented May 9, 2021

View reviewed changes

src/Renode/Network/NetworkServer/Modules/IcmpServerModule.cs Outdated Show resolved Hide resolved

John15321 commented May 10, 2021

View reviewed changes

src/Renode/Network/NetworkServer/Modules/IcmpServerModule.cs Outdated Show resolved Hide resolved

John15321 commented May 10, 2021

View reviewed changes

src/Renode/Network/NetworkServer/NetworkServer.cs Outdated Show resolved Hide resolved

PiotrZierhoffer requested changes May 17, 2021

View reviewed changes

John15321 force-pushed the 5_icmp_implementation branch from 62cf660 to a8551b6 Compare May 19, 2021 09:19

PiotrZierhoffer requested changes Jun 2, 2021

View reviewed changes

MatthiasMarczyszyn and others added 8 commits June 21, 2021 13:13

[#5] network-server: Extract IUdpServerModule interface

0df8a97

[#5] Move TftpServerModule to use IUdpServerModule

cfa3a83

[#5] network-server: Cast UDP modules to proper interface

1d6b8a5

[#5] Add ICMPv4 server module

cc8a0f2

[#5] Use ICMPv4 server in NetworkServer

4a37295

[#5] Add ICMP tests based on Zedboard Linux

05dd79d

[DEV] network-server: Organize usings

4020187

[#5] Add ICMP test to tests.yaml

17297c3

PiotrZierhoffer force-pushed the 5_icmp_implementation branch from 5282438 to 17297c3 Compare June 21, 2021 11:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#5] Implements reply and request forward for icmp #202

[#5] Implements reply and request forward for icmp #202

John15321 commented Apr 21, 2021

CLAassistant commented Apr 21, 2021 •

edited

PiotrZierhoffer Apr 26, 2021

PiotrZierhoffer Jun 2, 2021

PiotrZierhoffer commented Apr 27, 2021

PiotrZierhoffer Apr 27, 2021

PiotrZierhoffer Apr 27, 2021

PiotrZierhoffer Apr 27, 2021

PiotrZierhoffer May 5, 2021

PiotrZierhoffer May 5, 2021

PiotrZierhoffer May 5, 2021

PiotrZierhoffer May 5, 2021

PiotrZierhoffer May 5, 2021

PiotrZierhoffer May 5, 2021

PiotrZierhoffer May 17, 2021

PiotrZierhoffer May 17, 2021

PiotrZierhoffer May 17, 2021

PiotrZierhoffer May 17, 2021

PiotrZierhoffer May 17, 2021

PiotrZierhoffer May 17, 2021

PiotrZierhoffer May 17, 2021

PiotrZierhoffer May 17, 2021

PiotrZierhoffer commented May 25, 2021

PiotrZierhoffer May 26, 2021

PiotrZierhoffer Jun 2, 2021

PiotrZierhoffer Jun 2, 2021

PiotrZierhoffer Jun 2, 2021

PiotrZierhoffer Jun 2, 2021

PiotrZierhoffer Jun 2, 2021

PiotrZierhoffer Jun 2, 2021

PiotrZierhoffer Jun 2, 2021

PiotrZierhoffer Jun 2, 2021

PiotrZierhoffer Jun 2, 2021

PiotrZierhoffer Jun 2, 2021

PiotrZierhoffer Jun 2, 2021



		private MACAddress MAC { get; set; }
		private IPAddress IP { get; set; }


		this.Log(LogLevel.Noisy, "Handling ICMP packet: {0}", (ICMPv4Packet)ipv4Packet.PayloadPacket);

		var ipv4ResponePacket = CreateIPv4Packet(ipv4Packet);

	${SCRIPT} scripts/single-node/zedboard.resc
	${SCRIPT} ${CURDIR}/../../../scripts/single-node/zedboard.resc

		};
		for (var i = 0; i < icmpv4PacketResponse.Data.Length; i++) icmpv4PacketResponse.Data[i] = 0;


		ethernetResponse.RecursivelyUpdateCalculatedValues(new[] { EthernetPacketType.IpV4 }, new[] { PacketDotNet.IPProtocolType.ICMP });



		if (!EthernetFrame.TryCreateEthernetFrame(ethernetResponse.Bytes,
		true, out var responseEthernetFrame))

[#5] Implements reply and request forward for icmp #202

Are you sure you want to change the base?

[#5] Implements reply and request forward for icmp #202

Conversation

John15321 commented Apr 21, 2021

CLAassistant commented Apr 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PiotrZierhoffer commented Apr 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PiotrZierhoffer commented May 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Apr 21, 2021 •

edited