From b965e4f2b564f08861701c620551f8009d90a26c Mon Sep 17 00:00:00 2001 From: Nathaniel Wesley Filardo Date: Mon, 6 Jan 2020 16:31:04 +0000 Subject: [PATCH] espconn: De-orbit espconn_gethostbyname Further work on https://github.com/nodemcu/nodemcu-firmware/issues/3004 While here, remove `mqtt`'s charming DNS-retry logic (which is neither shared with nor duplicated in other modules) and update its :connect() return value behavior and documentation. --- app/http/httpclient.c | 9 ++-- app/include/lwip/app/espconn.h | 20 --------- app/lwip/app/espconn.c | 23 ----------- app/modules/mqtt.c | 73 +++++++++++++-------------------- app/websocket/websocketclient.c | 4 +- docs/modules/mqtt.md | 4 +- 6 files changed, 37 insertions(+), 96 deletions(-) diff --git a/app/http/httpclient.c b/app/http/httpclient.c index 1419d59771..a73f76279f 100644 --- a/app/http/httpclient.c +++ b/app/http/httpclient.c @@ -563,21 +563,20 @@ void ICACHE_FLASH_ATTR http_raw_request( const char * hostname, int port, bool s req->redirect_follow_count = redirect_follow_count; ip_addr_t addr; - err_t error = espconn_gethostbyname( (struct espconn *) req, /* It seems we don't need a real espconn pointer here. */ - hostname, &addr, http_dns_callback ); + err_t error = dns_gethostbyname( hostname, &addr, http_dns_callback, req ); - if ( error == ESPCONN_INPROGRESS ) + if ( error == ERR_INPROGRESS ) { HTTPCLIENT_DEBUG( "DNS pending" ); } - else if ( error == ESPCONN_OK ) + else if ( error == ERR_OK ) { /* Already in the local names table (or hostname was an IP address), execute the callback ourselves. */ http_dns_callback( hostname, &addr, req ); } else { - if ( error == ESPCONN_ARG ) + if ( error == ERR_ARG ) { HTTPCLIENT_ERR( "DNS arg error %s", hostname ); }else { diff --git a/app/include/lwip/app/espconn.h b/app/include/lwip/app/espconn.h index 75e7a1f66d..5769d7378d 100644 --- a/app/include/lwip/app/espconn.h +++ b/app/include/lwip/app/espconn.h @@ -475,26 +475,6 @@ extern sint8 espconn_regist_disconcb(struct espconn *espconn, espconn_connect_ca extern uint32 espconn_port(void); -/****************************************************************************** - * FunctionName : espconn_gethostbyname - * Description : Resolve a hostname (string) into an IP address. - * Parameters : pespconn -- espconn to resolve a hostname - * hostname -- the hostname that is to be queried - * addr -- pointer to a ip_addr_t where to store the address if - * it is already cached in the dns_table (only valid if - * ESPCONN_OK is returned!) - * found -- a callback function to be called on success, failure - * or timeout (only if ERR_INPROGRESS is returned!) - * Returns : err_t return code - * - ESPCONN_OK if hostname is a valid IP address string or the host - * name is already in the local names table. - * - ESPCONN_INPROGRESS enqueue a request to be sent to the DNS server - * for resolution if no errors are present. - * - ESPCONN_ARG: dns client not initialized or invalid hostname -*******************************************************************************/ - -extern err_t espconn_gethostbyname(struct espconn *pespconn, const char *name, ip_addr_t *addr, dns_found_callback found); - /****************************************************************************** * FunctionName : espconn_encry_connect * Description : The function given as connection diff --git a/app/lwip/app/espconn.c b/app/lwip/app/espconn.c index 93b16abc63..739bec8eeb 100644 --- a/app/lwip/app/espconn.c +++ b/app/lwip/app/espconn.c @@ -887,26 +887,3 @@ espconn_port(void) return port; } - -/****************************************************************************** - * FunctionName : espconn_gethostbyname - * Description : Resolve a hostname (string) into an IP address. - * Parameters : pespconn -- espconn to resolve a hostname - * hostname -- the hostname that is to be queried - * addr -- pointer to a ip_addr_t where to store the address if - * it is already cached in the dns_table (only valid if - * ESPCONN_OK is returned!) - * found -- a callback function to be called on success, failure - * or timeout (only if ERR_INPROGRESS is returned!) - * Returns : err_t return code - * - ESPCONN_OK if hostname is a valid IP address string or the host - * name is already in the local names table. - * - ESPCONN_INPROGRESS enqueue a request to be sent to the DNS server - * for resolution if no errors are present. - * - ESPCONN_ARG: dns client not initialized or invalid hostname -*******************************************************************************/ -err_t ICACHE_FLASH_ATTR -espconn_gethostbyname(struct espconn *pespconn, const char *hostname, ip_addr_t *addr, dns_found_callback found) -{ - return dns_gethostbyname(hostname, addr, found, pespconn); -} diff --git a/app/modules/mqtt.c b/app/modules/mqtt.c index 035c005932..a60817f0f9 100644 --- a/app/modules/mqtt.c +++ b/app/modules/mqtt.c @@ -1156,21 +1156,13 @@ static sint8 socket_connect(struct espconn *pesp_conn) return espconn_status; } -static sint8 socket_dns_found(const char *name, ip_addr_t *ipaddr, void *arg); -static int dns_reconn_count = 0; -static ip_addr_t host_ip; // for dns - -/* wrapper for using socket_dns_found() as callback function */ -static void socket_dns_foundcb(const char *name, ip_addr_t *ipaddr, void *arg) -{ - socket_dns_found(name, ipaddr, arg); -} - static sint8 socket_dns_found(const char *name, ip_addr_t *ipaddr, void *arg) { + lmqtt_userdata *mud = arg; + NODE_DBG("enter socket_dns_found.\n"); sint8 espconn_status = ESPCONN_OK; - struct espconn *pesp_conn = arg; + struct espconn *pesp_conn = mud->pesp_conn; if(pesp_conn == NULL){ NODE_DBG("pesp_conn null.\n"); return -1; @@ -1178,42 +1170,34 @@ static sint8 socket_dns_found(const char *name, ip_addr_t *ipaddr, void *arg) if(ipaddr == NULL) { - dns_reconn_count++; - if( dns_reconn_count >= 5 ){ - NODE_DBG( "DNS Fail!\n" ); - // Note: should delete the pesp_conn or unref self_ref here. - - struct espconn *pesp_conn = arg; - if(pesp_conn != NULL) { - lmqtt_userdata *mud = (lmqtt_userdata *)pesp_conn->reverse; - if(mud != NULL) { - mqtt_connack_fail(mud, MQTT_CONN_FAIL_DNS); - } - } + mqtt_connack_fail(mud, MQTT_CONN_FAIL_DNS); - mqtt_socket_disconnected(arg); // although not connected, but fire disconnect callback to release every thing. - return -1; - } - NODE_DBG( "DNS retry %d!\n", dns_reconn_count ); - host_ip.addr = 0; - return espconn_gethostbyname(pesp_conn, name, &host_ip, socket_dns_foundcb); + // although not connected, but fire disconnect callback to release every thing. + mqtt_socket_disconnected(arg); + return -1; } // ipaddr->addr is a uint32_t ip if(ipaddr->addr != 0) { - dns_reconn_count = 0; memcpy(pesp_conn->proto.tcp->remote_ip, &(ipaddr->addr), 4); NODE_DBG("TCP ip is set: "); NODE_DBG(IPSTR, IP2STR(&(ipaddr->addr))); NODE_DBG("\n"); espconn_status = socket_connect(pesp_conn); } + NODE_DBG("leave socket_dns_found.\n"); return espconn_status; } +/* wrapper for using socket_dns_found() as callback function */ +static void socket_dns_foundcb(const char *name, ip_addr_t *ipaddr, void *arg) +{ + socket_dns_found(name, ipaddr, arg); +} + #include "pm/swtimer.h" // Lua: mqtt:connect( host, port, secure, function(client), function(client, connect_return_code) ) static int mqtt_socket_connect( lua_State* L ) @@ -1227,7 +1211,6 @@ static int mqtt_socket_connect( lua_State* L ) int stack = 1; unsigned secure = 0; int top = lua_gettop(L); - sint8 espconn_status; mud = (lmqtt_userdata *)luaL_checkudata(L, stack, "mqtt.socket"); luaL_argcheck(L, mud, stack, "mqtt.socket expected"); @@ -1330,8 +1313,8 @@ static int mqtt_socket_connect( lua_State* L ) luaL_unref(L, LUA_REGISTRYINDEX, mud->self_ref); mud->self_ref = luaL_ref(L, LUA_REGISTRYINDEX); - espconn_status = espconn_regist_connectcb(pesp_conn, mqtt_socket_connected); - espconn_status |= espconn_regist_reconcb(pesp_conn, mqtt_socket_reconnected); + espconn_regist_connectcb(pesp_conn, mqtt_socket_connected); + espconn_regist_reconcb(pesp_conn, mqtt_socket_reconnected); os_timer_disarm(&mud->mqttTimer); os_timer_setfn(&mud->mqttTimer, (os_timer_func_t *)mqtt_socket_timer, mud); @@ -1342,25 +1325,27 @@ static int mqtt_socket_connect( lua_State* L ) if((ipaddr.addr == IPADDR_NONE) && (memcmp(domain,"255.255.255.255",16) != 0)) { - host_ip.addr = 0; - dns_reconn_count = 0; - if(ESPCONN_OK == espconn_gethostbyname(pesp_conn, domain, &host_ip, socket_dns_foundcb)){ - espconn_status |= socket_dns_found(domain, &host_ip, pesp_conn); // ip is returned in host_ip. + ip_addr_t host_ip; + switch (dns_gethostbyname(domain, &host_ip, socket_dns_foundcb, mud)) + { + case ERR_OK: + socket_dns_found(domain, &host_ip, mud); // ip is returned in host_ip. + break; + case ERR_INPROGRESS: + break; + default: + // Something has gone wrong; bail out? + mqtt_connack_fail(mud, MQTT_CONN_FAIL_DNS); } } else { - espconn_status |= socket_connect(pesp_conn); + socket_connect(pesp_conn); } NODE_DBG("leave mqtt_socket_connect.\n"); - if (espconn_status == ESPCONN_OK) { - lua_pushboolean(L, 1); - } else { - lua_pushboolean(L, 0); - } - return 1; + return 0; } // Lua: mqtt:close() diff --git a/app/websocket/websocketclient.c b/app/websocket/websocketclient.c index b03fad4dd9..4996af8d92 100644 --- a/app/websocket/websocketclient.c +++ b/app/websocket/websocketclient.c @@ -824,9 +824,9 @@ void ws_connect(ws_info *ws, const char *url) { // Attempt to resolve hostname address ip_addr_t addr; - err_t result = espconn_gethostbyname(conn, hostname, &addr, dns_callback); + err_t result = dns_gethostbyname(hostname, &addr, dns_callback, conn); - if (result == ESPCONN_INPROGRESS) { + if (result == ERR_INPROGRESS) { NODE_DBG("DNS pending\n"); } else { dns_callback(hostname, &addr, conn); diff --git a/docs/modules/mqtt.md b/docs/modules/mqtt.md index 0b9261df80..0d9f62f430 100644 --- a/docs/modules/mqtt.md +++ b/docs/modules/mqtt.md @@ -134,11 +134,11 @@ Connects to the broker specified by the given host, port, and secure options. !!! attention - Secure (`https`) connections come with quite a few limitations. Please see + Secure (`mqtts`) connections come with quite a few limitations. Please see the warnings in the [tls module](tls.md)'s documentation. #### Returns -`true` on success, `false` otherwise +`nil`; use callbacks to observe the outcome. #### Notes