[GitHub] [nuttx] pkarashchenko commented on a diff in pull request #7750: net/ipfwd: Support ICMP/ICMPv6 error reply when forwarding

[GitBox]

pkarashchenko commented on code in PR #7750:
URL: https://github.com/apache/nuttx/pull/7750#discussion_r1037694003


##########
net/ipforward/ipv4_forward.c:
##########
@@ -505,6 +502,37 @@ int ipv4_forward(FAR struct net_driver_s *dev, FAR struct 
ipv4_hdr_s *ipv4)
 
 drop:
   ipv4_dropstats(ipv4);
+
+#ifdef CONFIG_NET_ICMP
+#ifdef CONFIG_NET_NAT

Review Comment:
   ```suggestion
   #  ifdef CONFIG_NET_NAT
   ```



##########
net/ipforward/ipv4_forward.c:
##########
@@ -505,6 +502,37 @@ int ipv4_forward(FAR struct net_driver_s *dev, FAR struct 
ipv4_hdr_s *ipv4)
 
 drop:
   ipv4_dropstats(ipv4);
+
+#ifdef CONFIG_NET_ICMP
+#ifdef CONFIG_NET_NAT
+  /* Before we reply ICMP, call NAT outbound to try to translate destination
+   * address & port back to original status.
+   */
+
+  if (ret == -ENETUNREACH || ret == -EFBIG || ret == -EMULTIHOP)
+    {
+      ipv4_nat_outbound(dev, ipv4, NAT_MANIP_DST);
+    }
+#endif /* CONFIG_NET_NAT */

Review Comment:
   ```suggestion
   #  endif /* CONFIG_NET_NAT */
   ```



##########
net/ipforward/ipv4_forward.c:
##########
@@ -505,6 +502,37 @@ int ipv4_forward(FAR struct net_driver_s *dev, FAR struct 
ipv4_hdr_s *ipv4)
 
 drop:
   ipv4_dropstats(ipv4);
+
+#ifdef CONFIG_NET_ICMP
+#ifdef CONFIG_NET_NAT
+  /* Before we reply ICMP, call NAT outbound to try to translate destination
+   * address & port back to original status.
+   */
+
+  if (ret == -ENETUNREACH || ret == -EFBIG || ret == -EMULTIHOP)
+    {
+      ipv4_nat_outbound(dev, ipv4, NAT_MANIP_DST);
+    }
+#endif /* CONFIG_NET_NAT */
+
+  /* Reply ICMP to the sender for particular errors. */
+
+  switch (ret)
+    {
+      case -ENETUNREACH:
+        icmp_reply(dev, ICMP_DEST_UNREACHABLE, ICMP_NET_UNREACH);
+        return OK;
+
+      case -EFBIG:
+        icmp_reply(dev, ICMP_DEST_UNREACHABLE, ICMP_FRAG_NEEDED);
+        return OK;
+
+      case -EMULTIHOP:
+        icmp_reply(dev, ICMP_TIME_EXCEEDED, 0);
+        return OK;
+    }

Review Comment:
   Can also consider
   ```suggestion
     switch (ret)
       {
         case -ENETUNREACH:
         case -EFBIG:
         case -EMULTIHOP:
   #  ifdef CONFIG_NET_NAT
           ipv4_nat_outbound(dev, ipv4, NAT_MANIP_DST);
   #  endif /* CONFIG_NET_NAT */
           icmp_reply(dev,
                      ret == -EMULTIHOP ? ICMP_TIME_EXCEEDED :
                                          ICMP_DEST_UNREACHABLE,
                      ret == -EMULTIHOP ? 0 :
                                          ret == -EFBIG ? ICMP_FRAG_NEEDED :
                                                          ICMP_NET_UNREACH);
           return OK;
       }
   ```



##########
net/ipforward/ipv4_forward.c:
##########
@@ -505,6 +502,37 @@ int ipv4_forward(FAR struct net_driver_s *dev, FAR struct 
ipv4_hdr_s *ipv4)
 
 drop:
   ipv4_dropstats(ipv4);
+
+#ifdef CONFIG_NET_ICMP
+#ifdef CONFIG_NET_NAT
+  /* Before we reply ICMP, call NAT outbound to try to translate destination
+   * address & port back to original status.
+   */
+
+  if (ret == -ENETUNREACH || ret == -EFBIG || ret == -EMULTIHOP)
+    {
+      ipv4_nat_outbound(dev, ipv4, NAT_MANIP_DST);
+    }
+#endif /* CONFIG_NET_NAT */
+
+  /* Reply ICMP to the sender for particular errors. */
+
+  switch (ret)
+    {
+      case -ENETUNREACH:
+        icmp_reply(dev, ICMP_DEST_UNREACHABLE, ICMP_NET_UNREACH);
+        return OK;
+
+      case -EFBIG:
+        icmp_reply(dev, ICMP_DEST_UNREACHABLE, ICMP_FRAG_NEEDED);
+        return OK;
+
+      case -EMULTIHOP:
+        icmp_reply(dev, ICMP_TIME_EXCEEDED, 0);
+        return OK;
+    }

Review Comment:
   Please add `default` case



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org