espressif / esp-lwip

Fork of lwIP (https://savannah.nongnu.org/projects/lwip/) with ESP-IDF specific patches
Other
79 stars 126 forks source link

ip_napt_maint: Fix timestamp overflow handling (IDFGH-7201) #45

Closed rojer closed 2 years ago

rojer commented 2 years ago

s_last_now was not being updated once overflow happened.

david-cermak commented 2 years ago

@rojer Thanks for the contribution!

Could you please also fix the compilation warning when DHCP's VCI is disabled (IDF default)? (I'm sorry, I've missed that and it wasn't caught in the CI when merging your PR)

+++ b/src/core/ipv4/dhcp.c
@@ -364,7 +364,9 @@ dhcp_check(struct netif *netif)
 static void
 dhcp_handle_offer(struct netif *netif, struct dhcp_msg *msg_in)
 {
+#if ESP_DHCP && !ESP_DHCP_DISABLE_VENDOR_CLASS_IDENTIFIER
   u8_t n;
+#endif /* ESP_DHCP && !ESP_DHCP_DISABLE_VENDOR_CLASS_IDENTIFIER */
   struct dhcp *dhcp = netif_dhcp_data(netif);

   LWIP_DEBUGF(DHCP_DEBUG | LWIP_DBG_TRACE, ("dhcp_handle_offer(netif=%p) %c%c%"U16_F"\n",

...been also considering moving these NAPT counters, that you've introduced into the lwip stats module (so they could be turned off or moved to another place with other statistics), here's an example of such update: e85053a8

rojer commented 2 years ago

Could you please also fix the compilation warning when DHCP's VCI is disabled (IDF default)?

done

been also considering moving these NAPT counters, that you've introduced into the lwip stats module (so they could be turned off or moved to another place with other statistics)

sure, makes sense.

rojer commented 2 years ago

@david-cermak ping?

david-cermak commented 2 years ago

@rojer Sorry for the delay. Please consider this approved and ready for merging, but would still take some time till it gets officially merged.

david-cermak commented 2 years ago

@rojer Thanks again for the fixes. This has been merged internally accepting your first commit

but I've squashed your second commit (the DHCP variable fix) c6dad9e152564b03651a5fee3fe3cd654576baef with the napt stats module integration (which didn't mark the PR as done), closing manually.