fix(dhcps): Fixed DNS server handling for previous breaking changes

Author: abhik-roy85

components/esp_netif/lwip/esp_netif_lwip.c

@@ -1994,7 +1994,7 @@ static esp_err_t esp_netif_set_dns_info_api(esp_netif_api_msg_t *msg)
             ESP_LOGD(TAG, "set dns invalid type");
             return ESP_ERR_ESP_NETIF_INVALID_PARAMS;
         } else {
-            dhcps_dns_setserver(esp_netif->dhcps, &lwip_ip, (dns_type_t)type);
+            dhcps_dns_setserver_by_type(esp_netif->dhcps, &lwip_ip, (dns_type_t)type);
         }
 #else
         LOG_NETIF_DISABLED_AND_DO("DHCP Server", return ESP_ERR_NOT_SUPPORTED);
@@ -2053,7 +2053,7 @@ static esp_err_t esp_netif_get_dns_info_api(esp_netif_api_msg_t *msg)
     if (esp_netif && esp_netif->flags & ESP_NETIF_DHCP_SERVER) {
 #if ESP_DHCPS
         ip4_addr_t dns_ip;
-        dhcps_dns_getserver(esp_netif->dhcps, &dns_ip, (dns_type_t)type);
+        dhcps_dns_getserver_by_type(esp_netif->dhcps, &dns_ip, (dns_type_t)type);
         memcpy(&dns->ip.u_addr.ip4, &dns_ip, sizeof(ip4_addr_t));
         dns->ip.type = ESP_IPADDR_TYPE_V4;
 #else

components/lwip/apps/dhcpserver/dhcpserver.c

@@ -197,6 +197,18 @@ static void get_ip_info(struct netif * netif, ip_info_t *ip_info)
     }
 }
 
+static inline u8_t* dhcps_option_ip(u8_t *optptr, const ip4_addr_t *ip)
+{
+    LWIP_ASSERT("dhcps_option_ip: optptr must not be NULL", (optptr != NULL));
+    LWIP_ASSERT("dhcps_option_ip: ip must not be NULL", (ip != NULL));
+
+    *optptr++ = ip4_addr1(ip);
+    *optptr++ = ip4_addr2(ip);
+    *optptr++ = ip4_addr3(ip);
+    *optptr++ = ip4_addr4(ip);
+    return optptr;
+}
+
 /******************************************************************************
  * FunctionName : dhcps_option_info
  * Description  : get the DHCP message option info
@@ -422,10 +434,7 @@ static u8_t *add_offer_options(dhcps_t *dhcps, u8_t *optptr)
 
     *optptr++ = DHCP_OPTION_SUBNET_MASK;
     *optptr++ = 4;
-    *optptr++ = ip4_addr1(&dhcps->dhcps_mask);
-    *optptr++ = ip4_addr2(&dhcps->dhcps_mask);
-    *optptr++ = ip4_addr3(&dhcps->dhcps_mask);
-    *optptr++ = ip4_addr4(&dhcps->dhcps_mask);
+    optptr = dhcps_option_ip(optptr, &dhcps->dhcps_mask);
 
     *optptr++ = DHCP_OPTION_LEASE_TIME;
     *optptr++ = 4;
@@ -436,10 +445,7 @@ static u8_t *add_offer_options(dhcps_t *dhcps, u8_t *optptr)
 
     *optptr++ = DHCP_OPTION_SERVER_ID;
     *optptr++ = 4;
-    *optptr++ = ip4_addr1(&ipadd);
-    *optptr++ = ip4_addr2(&ipadd);
-    *optptr++ = ip4_addr3(&ipadd);
-    *optptr++ = ip4_addr4(&ipadd);
+    optptr = dhcps_option_ip(optptr, &ipadd);
 
     if (dhcps_router_enabled(dhcps->dhcps_offer)) {
         ip_info_t if_ip = { 0 };
@@ -450,10 +456,7 @@ static u8_t *add_offer_options(dhcps_t *dhcps, u8_t *optptr)
         if (!ip4_addr_isany_val(*gw_ip)) {
             *optptr++ = DHCP_OPTION_ROUTER;
             *optptr++ = 4;
-            *optptr++ = ip4_addr1(gw_ip);
-            *optptr++ = ip4_addr2(gw_ip);
-            *optptr++ = ip4_addr3(gw_ip);
-            *optptr++ = ip4_addr4(gw_ip);
+            optptr = dhcps_option_ip(optptr, gw_ip);
         }
     }
 
@@ -467,36 +470,23 @@ static u8_t *add_offer_options(dhcps_t *dhcps, u8_t *optptr)
 
         *optptr++ = DHCP_OPTION_DNS_SERVER;
         *optptr++ = size;
-
-        *optptr++ = ip4_addr1(&dhcps->dns_server[DNS_TYPE_MAIN]);
-        *optptr++ = ip4_addr2(&dhcps->dns_server[DNS_TYPE_MAIN]);
-        *optptr++ = ip4_addr3(&dhcps->dns_server[DNS_TYPE_MAIN]);
-        *optptr++ = ip4_addr4(&dhcps->dns_server[DNS_TYPE_MAIN]);
+        optptr = dhcps_option_ip(optptr, &dhcps->dns_server[DNS_TYPE_MAIN]);
 
         if (dhcps->dns_server[DNS_TYPE_BACKUP].addr) {
-            *optptr++ = ip4_addr1(&dhcps->dns_server[DNS_TYPE_BACKUP]);
-            *optptr++ = ip4_addr2(&dhcps->dns_server[DNS_TYPE_BACKUP]);
-            *optptr++ = ip4_addr3(&dhcps->dns_server[DNS_TYPE_BACKUP]);
-            *optptr++ = ip4_addr4(&dhcps->dns_server[DNS_TYPE_BACKUP]);
+            optptr = dhcps_option_ip(optptr, &dhcps->dns_server[DNS_TYPE_BACKUP]);
         }
 #ifdef CONFIG_LWIP_DHCPS_ADD_DNS
     } else {
         *optptr++ = DHCP_OPTION_DNS_SERVER;
         *optptr++ = 4;
-        *optptr++ = ip4_addr1(&ipadd);
-        *optptr++ = ip4_addr2(&ipadd);
-        *optptr++ = ip4_addr3(&ipadd);
-        *optptr++ = ip4_addr4(&ipadd);
+        optptr = dhcps_option_ip(optptr, &ipadd);
 #endif /* CONFIG_LWIP_DHCPS_ADD_DNS */
     }
 
     ip4_addr_t broadcast_addr = { .addr = (ipadd.addr & dhcps->dhcps_mask.addr) | ~dhcps->dhcps_mask.addr };
     *optptr++ = DHCP_OPTION_BROADCAST_ADDRESS;
     *optptr++ = 4;
-    *optptr++ = ip4_addr1(&broadcast_addr);
-    *optptr++ = ip4_addr2(&broadcast_addr);
-    *optptr++ = ip4_addr3(&broadcast_addr);
-    *optptr++ = ip4_addr4(&broadcast_addr);
+    optptr = dhcps_option_ip(optptr, &broadcast_addr);
 
     *optptr++ = DHCP_OPTION_INTERFACE_MTU;
     *optptr++ = 2;
@@ -1549,17 +1539,22 @@ bool dhcp_search_ip_on_mac(dhcps_t *dhcps, u8_t *mac, ip4_addr_t *ip)
 }
 
 /******************************************************************************
- * FunctionName : dhcps_dns_setserver
- * Description  : set DNS server address for dhcpserver
+ * FunctionName : dhcps_dns_setserver_by_type
+ * Description  : Set the DNS server address for dhcpserver with a specific type
  * Parameters   : dnsserver -- The DNS server address
  *                type      -- The DNS type
- * Returns      : ERR_ARG if invalid handle, ERR_OK on success
+ * Returns      : ERR_ARG if invalid handle, ERR_VAL if invalid type, ERR_OK on success
 *******************************************************************************/
-err_t dhcps_dns_setserver(dhcps_t *dhcps, const ip_addr_t *dnsserver, dns_type_t type)
+err_t dhcps_dns_setserver_by_type(dhcps_t *dhcps, const ip_addr_t *dnsserver, dns_type_t type)
 {
     if (dhcps == NULL) {
         return ERR_ARG;
     }
+
+    if (type >= DNS_TYPE_MAX) {
+        return ERR_VAL;
+    }
+
     if (dnsserver != NULL) {
         dhcps->dns_server[type] = *(ip_2_ip4(dnsserver));
     } else {
@@ -1569,18 +1564,46 @@ err_t dhcps_dns_setserver(dhcps_t *dhcps, const ip_addr_t *dnsserver, dns_type_t
 }
 
 /******************************************************************************
- * FunctionName : dhcps_dns_getserver
- * Description  : get DNS server address for dhcpserver
+ * FunctionName : dhcps_dns_setserver
+ * Description  : Set the main DNS server address for dhcpserver
+ * Parameters   : dnsserver -- The DNS server address
+ * Returns      : ERR_ARG if invalid handle, ERR_VAL if invalid type, ERR_OK on success
+*******************************************************************************/
+err_t dhcps_dns_setserver(dhcps_t *dhcps, const ip_addr_t *dnsserver)
+{
+    return dhcps_dns_setserver_by_type(dhcps, dnsserver, DNS_TYPE_MAIN);
+}
+
+/******************************************************************************
+ * FunctionName : dhcps_dns_getserver_by_type
+ * Description  : Get the DNS server address for dhcpserver
  * Parameters   : dnsserver -- The DNS server address
  *                type      -- The DNS type
- * Returns      : ERR_ARG if invalid handle, ERR_OK on success
+ * Returns      : ERR_ARG if invalid handle, ERR_VAL if invalid type, ERR_OK on success
 *******************************************************************************/
-err_t dhcps_dns_getserver(dhcps_t *dhcps, ip4_addr_t *dnsserver, dns_type_t type)
+err_t dhcps_dns_getserver_by_type(dhcps_t *dhcps, ip4_addr_t *dnsserver, dns_type_t type)
 {
-    if (dhcps) {
-        *dnsserver = dhcps->dns_server[type];
-        return ERR_OK;
+    if ((dhcps == NULL) || (dnsserver == NULL)) {
+        return ERR_ARG;
     }
-    return ERR_ARG;
+
+    if (type >= DNS_TYPE_MAX) {
+        return ERR_VAL;
+    }
+
+    *dnsserver = dhcps->dns_server[type];
+    return ERR_OK;
 }
+
+/******************************************************************************
+ * FunctionName : dhcps_dns_getserver_by_type
+ * Description  : Get the main DNS server address for dhcpserver
+ * Parameters   : dnsserver -- The DNS server address
+ * Returns      : ERR_ARG if invalid handle, ERR_VAL if invalid type, ERR_OK on success
+*******************************************************************************/
+err_t dhcps_dns_getserver(dhcps_t *dhcps, ip4_addr_t *dnsserver)
+{
+    return dhcps_dns_getserver_by_type(dhcps, dnsserver, DNS_TYPE_MAIN);
+}
+
 #endif // ESP_DHCPS

components/lwip/include/apps/dhcpserver/dhcpserver.h

@@ -168,22 +168,38 @@ err_t dhcps_set_option_info(dhcps_t *dhcps, u8_t op_id, void *opt_info, u32_t op
 bool dhcp_search_ip_on_mac(dhcps_t *dhcps, u8_t *mac, ip4_addr_t *ip);
 
 /**
- * @brief Sets DNS server address for the DHCP server
+ * @brief Sets the DNS server address for the DHCP server
+ * @param dhcps Pointer to the DHCP handle
+ * @param dnsserver Address of the DNS server
+ * @return ERR_ARG if invalid handle, ERR_VAL if invalid type, ERR_OK on success
+ */
+err_t dhcps_dns_setserver(dhcps_t *dhcps, const ip_addr_t *dnsserver);
+
+/**
+ * @brief Sets the DNS server address for the DHCP server with a specific type
  * @param dhcps Pointer to the DHCP handle
  * @param dnsserver Address of the DNS server
  * @param type Type of the DNS server
- * @return ERR_ARG if invalid handle, ERR_OK on success
+ * @return ERR_ARG if invalid handle, ERR_VAL if invalid type, ERR_OK on success
  */
-err_t dhcps_dns_setserver(dhcps_t *dhcps, const ip_addr_t *dnsserver, dns_type_t type);
+err_t dhcps_dns_setserver_by_type(dhcps_t *dhcps, const ip_addr_t *dnsserver, dns_type_t type);
 
 /**
- * @brief Gets DNS server associated with this DHCP server
+ * @brief Gets the DNS server associated with this DHCP server
+ * @param dhcps Pointer to the DHCP handle
+ * @param dnsserver Address of the DNS server
+ * @return ERR_ARG if invalid handle, ERR_VAL if invalid type, ERR_OK on success
+ */
+err_t dhcps_dns_getserver(dhcps_t *dhcps, ip4_addr_t *dnsserver);
+
+/**
+ * @brief Gets the DNS server associated with this DHCP server with a specific type
  * @param dhcps Pointer to the DHCP handle
  * @param dnsserver Address of the DNS server
  * @param type Type of the DNS server
- * @return ERR_ARG if invalid handle, ERR_OK on success
+ * @return ERR_ARG if invalid handle, ERR_VAL if invalid type, ERR_OK on success
  */
-err_t dhcps_dns_getserver(dhcps_t *dhcps, ip4_addr_t *dnsserver, dns_type_t type);
+err_t dhcps_dns_getserver_by_type(dhcps_t *dhcps, ip4_addr_t *dnsserver, dns_type_t type);
 
 /**
  * @brief Sets callback on assigning an IP to the connected client

components/lwip/test_apps/main/lwip_test.c

@@ -144,10 +144,24 @@ TEST(lwip, dhcp_server_init_deinit)
     dhcps_delete(dhcps);
 }
 
+typedef enum
+{
+    DNS_CALLBACK_TYPE_GET = 0, /**< DNS main server address*/
+    DNS_CALLBACK_TYPE_SET,   /**< DNS backup server address (Wi-Fi STA and Ethernet only) */
+} dns_callback_type_t;
+
+typedef struct dhcps_dns_options_ {
+    dns_callback_type_t cb_type;
+    ip_addr_t *dnsserver;
+    dns_type_t type;
+} dhcps_dns_options_t;
+
 struct dhcps_api {
     EventGroupHandle_t event;
+    dhcps_t *dhcps;
     ip4_addr_t netmask;
     ip4_addr_t ip;
+    dhcps_dns_options_t dns_options;
     err_t ret_start;
     err_t ret_stop;
 };
@@ -220,6 +234,75 @@ TEST(lwip, dhcp_server_start_stop_localhost)
     dhcps_test_net_classes(0xC0A8C808, 0xFFFFFFF8, false);
 }
 
+static void dhcps_test_dns_options_api(void* ctx)
+{
+    struct netif *netif;
+    struct dhcps_api *api = ctx;
+
+    NETIF_FOREACH(netif) {
+        if (netif->name[0] == 'l' && netif->name[1] == 'o') {
+            break;
+        }
+    }
+    TEST_ASSERT_NOT_NULL(netif);
+
+    if (api->dns_options.cb_type == DNS_CALLBACK_TYPE_GET) {
+        api->ret_start = dhcps_dns_getserver_by_type(api->dhcps,
+                                                    ip_2_ip4(api->dns_options.dnsserver),
+                                                    api->dns_options.type);
+    } else {
+        api->ret_start = dhcps_dns_setserver_by_type(api->dhcps,
+                                                    api->dns_options.dnsserver,
+                                                    api->dns_options.type);
+    }
+    xEventGroupSetBits(api->event, 1);
+}
+
+static void dhcps_test_dns_options(dns_callback_type_t cb_type,
+                                    dhcps_t *dhcps, ip_addr_t *dnsserver,
+                                    dns_type_t type, bool pass)
+{
+    struct dhcps_api api = {
+            .dhcps = dhcps,
+            .dns_options.cb_type = cb_type,
+            .dns_options.dnsserver = dnsserver,
+            .dns_options.type = type,
+            .ret_start = ERR_IF,
+            .event = xEventGroupCreate()
+    };
+
+    tcpip_callback(dhcps_test_dns_options_api, &api);
+    xEventGroupWaitBits(api.event, 1, true, true, pdMS_TO_TICKS(5000));
+    vEventGroupDelete(api.event);
+
+    TEST_ASSERT((api.ret_start == ERR_OK) == pass);
+}
+
+TEST(lwip, dhcp_server_dns_options)
+{
+    test_case_uses_tcpip();
+
+    // Class C: IP: 192.168.4.1
+    ip_addr_t ip = IPADDR4_INIT_BYTES(192, 168, 4, 1);
+
+    dhcps_test_dns_options(DNS_CALLBACK_TYPE_SET, NULL, &ip, DNS_TYPE_MAIN, false);
+    dhcps_test_dns_options(DNS_CALLBACK_TYPE_GET, NULL, &ip, DNS_TYPE_MAIN, false);
+
+    dhcps_t *dhcps = dhcps_new();
+    dhcps_test_dns_options(DNS_CALLBACK_TYPE_SET, dhcps, NULL, DNS_TYPE_MAIN, true);
+    dhcps_test_dns_options(DNS_CALLBACK_TYPE_GET, dhcps, NULL, DNS_TYPE_MAIN, false);
+
+    dhcps_test_dns_options(DNS_CALLBACK_TYPE_SET, dhcps, &ip, DNS_TYPE_MAX, false);
+    dhcps_test_dns_options(DNS_CALLBACK_TYPE_GET, dhcps, &ip, DNS_TYPE_MAX, false);
+
+    dhcps_test_dns_options(DNS_CALLBACK_TYPE_SET, dhcps, &ip, DNS_TYPE_MAIN, true);
+    dhcps_test_dns_options(DNS_CALLBACK_TYPE_GET, dhcps, &ip, DNS_TYPE_MAIN, true);
+
+    dhcps_test_dns_options(DNS_CALLBACK_TYPE_SET, dhcps, &ip, DNS_TYPE_BACKUP, true);
+    dhcps_test_dns_options(DNS_CALLBACK_TYPE_GET, dhcps, &ip, DNS_TYPE_BACKUP, true);
+
+    dhcps_delete(dhcps);
+}
 
 int test_sntp_server_create(void)
 {
@@ -324,6 +407,7 @@ TEST_GROUP_RUNNER(lwip)
     RUN_TEST_CASE(lwip, localhost_ping_test)
     RUN_TEST_CASE(lwip, dhcp_server_init_deinit)
     RUN_TEST_CASE(lwip, dhcp_server_start_stop_localhost)
+    RUN_TEST_CASE(lwip, dhcp_server_dns_options)
     RUN_TEST_CASE(lwip, sntp_client_time_2015)
     RUN_TEST_CASE(lwip, sntp_client_time_2048)
 }