Address reivew comments

doc/environment_variables.md

@@ -67,7 +67,9 @@ some configuration as environment variables that can be set.
   - ERROR - log only errors
 
 * GRPC_DNS_RESOLVER
-  Declares which DNS resolver to use. Available DNS resolver include:
+  Declares which DNS resolver to use. The default is ares if gRPC is built with
+  c-ares support. Otherwise, the value of this environment variable is ignored.
+  Available DNS resolver include:
   - ares (default) - a DNS resolver based around the c-ares library
   - native - a DNS resolver based around getaddrinfo(), creates a new thread to
     perform name resolution

src/core/ext/resolver/dns/c_ares/dns_resolver_ares.c

@@ -393,7 +393,6 @@ void grpc_resolver_dns_ares_init(void) {
 void grpc_resolver_dns_ares_shutdown(void) { grpc_ares_cleanup(); }
 
 #else /* GRPC_ARES == 1 */
-#include <grpc/support/log.h>
 
 void grpc_resolver_dns_ares_init(void) {}
 

src/core/ext/resolver/dns/c_ares/grpc_ares_ev_driver_posix.c

@@ -107,7 +107,6 @@ static void grpc_ares_ev_driver_unref(grpc_ares_ev_driver *ev_driver) {
     gpr_mu_destroy(&ev_driver->mu);
     ares_destroy(ev_driver->channel);
     gpr_free(ev_driver);
-    grpc_ares_cleanup();
   }
 }
 
@@ -125,10 +124,7 @@ static void fd_node_destroy(grpc_exec_ctx *exec_ctx, fd_node *fdn) {
 grpc_error *grpc_ares_ev_driver_create(grpc_ares_ev_driver **ev_driver,
                                        grpc_pollset_set *pollset_set) {
   int status;
-  grpc_error *err = grpc_ares_init();
-  if (err != GRPC_ERROR_NONE) {
-    return err;
-  }
+  grpc_error *err = GRPC_ERROR_NONE;
   *ev_driver = gpr_malloc(sizeof(grpc_ares_ev_driver));
   status = ares_init(&(*ev_driver)->channel);
   gpr_log(GPR_DEBUG, "grpc_ares_ev_driver_create");
@@ -150,13 +146,12 @@ grpc_error *grpc_ares_ev_driver_create(grpc_ares_ev_driver **ev_driver,
   return GRPC_ERROR_NONE;
 }
 
-void grpc_ares_ev_driver_destroy(  // grpc_exec_ctx *exec_ctx,
-    grpc_ares_ev_driver *ev_driver) {
+void grpc_ares_ev_driver_destroy(grpc_ares_ev_driver *ev_driver) {
   // It's not safe to shut down remaining fds here directly, becauses
   // ares_host_callback does not provide an exec_ctx. We mark the event driver
   // as being shut down. If the event driver is working,
   // grpc_ares_notify_on_event_locked will shut down the fds; if it's not
-  // working, grpc_ares_ev_driver_unref will release it directly.
+  // working, there are no fds to shut down.
   gpr_mu_lock(&ev_driver->mu);
   ev_driver->shutting_down = true;
   gpr_mu_unlock(&ev_driver->mu);
@@ -283,8 +278,7 @@ static void grpc_ares_notify_on_event_locked(grpc_exec_ctx *exec_ctx,
           fdn->readable_registered = true;
         }
         // Register write_closure if the socket is writable and write_closure
-        // has
-        // not been registered with this socket.
+        // has not been registered with this socket.
         if (ARES_GETSOCK_WRITABLE(socks_bitmask, i) &&
             !fdn->writable_registered) {
           gpr_log(GPR_DEBUG, "notify write on: %d",
@@ -316,14 +310,12 @@ static void grpc_ares_notify_on_event_locked(grpc_exec_ctx *exec_ctx,
 
 void grpc_ares_ev_driver_start(grpc_exec_ctx *exec_ctx,
                                grpc_ares_ev_driver *ev_driver) {
-  grpc_ares_ev_driver_ref(ev_driver);
   gpr_mu_lock(&ev_driver->mu);
   if (!ev_driver->working) {
     ev_driver->working = true;
     grpc_ares_notify_on_event_locked(exec_ctx, ev_driver);
   }
   gpr_mu_unlock(&ev_driver->mu);
-  grpc_ares_ev_driver_unref(ev_driver);
 }
 
 #endif /* GRPC_ARES == 1 && defined(GRPC_POSIX_SOCKET) */

src/core/ext/resolver/dns/c_ares/grpc_ares_wrapper.c

@@ -196,7 +196,6 @@ static void on_done_cb(void *arg, int status, int timeouts,
 
 void grpc_resolve_address_ares_impl(grpc_exec_ctx *exec_ctx, const char *name,
                                     const char *default_port,
-                                    // grpc_ares_ev_driver *ev_driver,
                                     grpc_pollset_set *interested_parties,
                                     grpc_closure *on_done,
                                     grpc_resolved_addresses **addrs) {
@@ -204,16 +203,16 @@ void grpc_resolve_address_ares_impl(grpc_exec_ctx *exec_ctx, const char *name,
   char *host;
   char *port;
   gpr_split_host_port(name, &host, &port);
-  grpc_error *err = GRPC_ERROR_NONE;
   if (host == NULL) {
-    err = grpc_error_set_str(GRPC_ERROR_CREATE("unparseable host:port"),
-                             GRPC_ERROR_STR_TARGET_ADDRESS, name);
+    grpc_error *err =
+        grpc_error_set_str(GRPC_ERROR_CREATE("unparseable host:port"),
+                           GRPC_ERROR_STR_TARGET_ADDRESS, name);
     grpc_exec_ctx_sched(exec_ctx, on_done, err, NULL);
     goto error_cleanup;
   } else if (port == NULL) {
     if (default_port == NULL) {
-      err = grpc_error_set_str(GRPC_ERROR_CREATE("no port in name"),
-                               GRPC_ERROR_STR_TARGET_ADDRESS, name);
+      grpc_error *err = grpc_error_set_str(GRPC_ERROR_CREATE("no port in name"),
+                                           GRPC_ERROR_STR_TARGET_ADDRESS, name);
       grpc_exec_ctx_sched(exec_ctx, on_done, err, NULL);
       goto error_cleanup;
     }
@@ -221,7 +220,7 @@ void grpc_resolve_address_ares_impl(grpc_exec_ctx *exec_ctx, const char *name,
   }
 
   grpc_ares_ev_driver *ev_driver;
-  err = grpc_ares_ev_driver_create(&ev_driver, interested_parties);
+  grpc_error *err = grpc_ares_ev_driver_create(&ev_driver, interested_parties);
   if (err != GRPC_ERROR_NONE) {
     GRPC_LOG_IF_ERROR("grpc_ares_ev_driver_create() failed", err);
     goto error_cleanup;
@@ -258,7 +257,6 @@ error_cleanup:
 void (*grpc_resolve_address_ares)(
     grpc_exec_ctx *exec_ctx, const char *name, const char *default_port,
     grpc_pollset_set *interested_parties, grpc_closure *on_done,
-    // grpc_ares_ev_driver *ev_driver, grpc_closure *on_done,
     grpc_resolved_addresses **addrs) = grpc_resolve_address_ares_impl;
 
 grpc_error *grpc_ares_init(void) {