From f47c9635f0ea406b85f9b0d11549c55baf4c583a Mon Sep 17 00:00:00 2001 From: Yuchen Zeng Date: Fri, 18 Nov 2016 20:41:20 -0800 Subject: [PATCH] Address reivew comments --- doc/environment_variables.md | 4 +++- .../ext/resolver/dns/c_ares/dns_resolver_ares.c | 1 - .../dns/c_ares/grpc_ares_ev_driver_posix.c | 16 ++++------------ .../ext/resolver/dns/c_ares/grpc_ares_wrapper.c | 14 ++++++-------- 4 files changed, 13 insertions(+), 22 deletions(-) diff --git a/doc/environment_variables.md b/doc/environment_variables.md index 6c13015baa5..9222cc305c5 100644 --- a/doc/environment_variables.md +++ b/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 diff --git a/src/core/ext/resolver/dns/c_ares/dns_resolver_ares.c b/src/core/ext/resolver/dns/c_ares/dns_resolver_ares.c index c0c1efbb863..01ed0114394 100644 --- a/src/core/ext/resolver/dns/c_ares/dns_resolver_ares.c +++ b/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 void grpc_resolver_dns_ares_init(void) {} diff --git a/src/core/ext/resolver/dns/c_ares/grpc_ares_ev_driver_posix.c b/src/core/ext/resolver/dns/c_ares/grpc_ares_ev_driver_posix.c index 68c52e43f06..301f52d62fd 100644 --- a/src/core/ext/resolver/dns/c_ares/grpc_ares_ev_driver_posix.c +++ b/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) */ diff --git a/src/core/ext/resolver/dns/c_ares/grpc_ares_wrapper.c b/src/core/ext/resolver/dns/c_ares/grpc_ares_wrapper.c index b63b7ebfe37..c210c40f096 100644 --- a/src/core/ext/resolver/dns/c_ares/grpc_ares_wrapper.c +++ b/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) {