From 0e48a9af490c5c48802ca1f3faab36e022cfca49 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 8 Sep 2016 14:14:39 -0700 Subject: Move LB policy instantiation from resolvers into client_channel. --- src/core/ext/resolver/dns/native/dns_resolver.c | 27 ++++------------------ src/core/ext/resolver/sockaddr/sockaddr_resolver.c | 18 ++------------- 2 files changed, 7 insertions(+), 38 deletions(-) (limited to 'src/core/ext/resolver') diff --git a/src/core/ext/resolver/dns/native/dns_resolver.c b/src/core/ext/resolver/dns/native/dns_resolver.c index 8fc10d98a8..78a996788d 100644 --- a/src/core/ext/resolver/dns/native/dns_resolver.c +++ b/src/core/ext/resolver/dns/native/dns_resolver.c @@ -37,7 +37,6 @@ #include #include -#include "src/core/ext/client_config/lb_policy_registry.h" #include "src/core/ext/client_config/resolver_registry.h" #include "src/core/lib/iomgr/resolve_address.h" #include "src/core/lib/iomgr/timer.h" @@ -58,8 +57,6 @@ typedef struct { char *name; /** default port to use */ char *default_port; - /** subchannel factory */ - grpc_client_channel_factory *client_channel_factory; /** load balancing policy name */ char *lb_policy_name; @@ -166,29 +163,18 @@ static void dns_on_resolved(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { dns_resolver *r = arg; grpc_resolver_result *result = NULL; - grpc_lb_policy *lb_policy; gpr_mu_lock(&r->mu); GPR_ASSERT(r->resolving); r->resolving = 0; if (r->addresses != NULL) { - grpc_lb_policy_args lb_policy_args; - memset(&lb_policy_args, 0, sizeof(lb_policy_args)); - lb_policy_args.addresses = grpc_addresses_create(r->addresses->naddrs); + grpc_addresses *addresses = grpc_addresses_create(r->addresses->naddrs); for (size_t i = 0; i < r->addresses->naddrs; ++i) { - grpc_addresses_set_address( - lb_policy_args.addresses, i, &r->addresses->addrs[i].addr, - r->addresses->addrs[i].len, false /* is_balancer */); + grpc_addresses_set_address(addresses, i, &r->addresses->addrs[i].addr, + r->addresses->addrs[i].len, + false /* is_balancer */); } grpc_resolved_addresses_destroy(r->addresses); - lb_policy_args.client_channel_factory = r->client_channel_factory; - lb_policy = - grpc_lb_policy_create(exec_ctx, r->lb_policy_name, &lb_policy_args); - grpc_addresses_destroy(lb_policy_args.addresses); - result = grpc_resolver_result_create(); - if (lb_policy != NULL) { - grpc_resolver_result_set_lb_policy(result, lb_policy); - GRPC_LB_POLICY_UNREF(exec_ctx, lb_policy, "construction"); - } + result = grpc_resolver_result_create(addresses, r->lb_policy_name); } else { gpr_timespec now = gpr_now(GPR_CLOCK_MONOTONIC); gpr_timespec next_try = gpr_backoff_step(&r->backoff_state, now); @@ -249,7 +235,6 @@ static void dns_destroy(grpc_exec_ctx *exec_ctx, grpc_resolver *gr) { if (r->resolved_result) { grpc_resolver_result_unref(exec_ctx, r->resolved_result); } - grpc_client_channel_factory_unref(exec_ctx, r->client_channel_factory); gpr_free(r->name); gpr_free(r->default_port); gpr_free(r->lb_policy_name); @@ -276,10 +261,8 @@ static grpc_resolver *dns_create(grpc_resolver_args *args, grpc_resolver_init(&r->base, &dns_resolver_vtable); r->name = gpr_strdup(path); r->default_port = gpr_strdup(default_port); - r->client_channel_factory = args->client_channel_factory; gpr_backoff_init(&r->backoff_state, BACKOFF_MULTIPLIER, BACKOFF_JITTER, BACKOFF_MIN_SECONDS * 1000, BACKOFF_MAX_SECONDS * 1000); - grpc_client_channel_factory_ref(r->client_channel_factory); r->lb_policy_name = gpr_strdup(lb_policy_name); return &r->base; } diff --git a/src/core/ext/resolver/sockaddr/sockaddr_resolver.c b/src/core/ext/resolver/sockaddr/sockaddr_resolver.c index 94d2f892eb..328c7cb6f9 100644 --- a/src/core/ext/resolver/sockaddr/sockaddr_resolver.c +++ b/src/core/ext/resolver/sockaddr/sockaddr_resolver.c @@ -40,7 +40,6 @@ #include #include -#include "src/core/ext/client_config/lb_policy_registry.h" #include "src/core/ext/client_config/parse_address.h" #include "src/core/ext/client_config/resolver_registry.h" #include "src/core/lib/iomgr/resolve_address.h" @@ -52,8 +51,6 @@ typedef struct { grpc_resolver base; /** refcount */ gpr_refcount refs; - /** subchannel factory */ - grpc_client_channel_factory *client_channel_factory; /** load balancing policy name */ char *lb_policy_name; @@ -122,17 +119,9 @@ static void sockaddr_next(grpc_exec_ctx *exec_ctx, grpc_resolver *resolver, static void sockaddr_maybe_finish_next_locked(grpc_exec_ctx *exec_ctx, sockaddr_resolver *r) { if (r->next_completion != NULL && !r->published) { - grpc_resolver_result *result = grpc_resolver_result_create(); - grpc_lb_policy_args lb_policy_args; - memset(&lb_policy_args, 0, sizeof(lb_policy_args)); - lb_policy_args.addresses = r->addresses; - lb_policy_args.client_channel_factory = r->client_channel_factory; - grpc_lb_policy *lb_policy = - grpc_lb_policy_create(exec_ctx, r->lb_policy_name, &lb_policy_args); - grpc_resolver_result_set_lb_policy(result, lb_policy); - GRPC_LB_POLICY_UNREF(exec_ctx, lb_policy, "sockaddr"); r->published = true; - *r->target_result = result; + *r->target_result = + grpc_resolver_result_create(r->addresses, r->lb_policy_name); grpc_exec_ctx_sched(exec_ctx, r->next_completion, GRPC_ERROR_NONE, NULL); r->next_completion = NULL; } @@ -141,7 +130,6 @@ static void sockaddr_maybe_finish_next_locked(grpc_exec_ctx *exec_ctx, static void sockaddr_destroy(grpc_exec_ctx *exec_ctx, grpc_resolver *gr) { sockaddr_resolver *r = (sockaddr_resolver *)gr; gpr_mu_destroy(&r->mu); - grpc_client_channel_factory_unref(exec_ctx, r->client_channel_factory); grpc_addresses_destroy(r->addresses); gpr_free(r->lb_policy_name); gpr_free(r); @@ -243,8 +231,6 @@ static grpc_resolver *sockaddr_create( gpr_ref_init(&r->refs, 1); gpr_mu_init(&r->mu); grpc_resolver_init(&r->base, &sockaddr_resolver_vtable); - r->client_channel_factory = args->client_channel_factory; - grpc_client_channel_factory_ref(r->client_channel_factory); return &r->base; } -- cgit v1.2.3 From 38525a9a082df5c8a61efe046cde9cb14ae4170c Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 9 Sep 2016 14:45:00 -0700 Subject: Fix use-after-free bug. --- src/core/ext/client_config/resolver_result.c | 7 +++++++ src/core/ext/client_config/resolver_result.h | 2 ++ src/core/ext/resolver/sockaddr/sockaddr_resolver.c | 4 ++-- 3 files changed, 11 insertions(+), 2 deletions(-) (limited to 'src/core/ext/resolver') diff --git a/src/core/ext/client_config/resolver_result.c b/src/core/ext/client_config/resolver_result.c index ba21349d2d..b0602d583d 100644 --- a/src/core/ext/client_config/resolver_result.c +++ b/src/core/ext/client_config/resolver_result.c @@ -47,6 +47,13 @@ grpc_addresses *grpc_addresses_create(size_t num_addresses) { return addresses; } +grpc_addresses *grpc_addresses_copy(grpc_addresses* addresses) { + grpc_addresses *new = grpc_addresses_create(addresses->num_addresses); + memcpy(new->addresses, addresses->addresses, + sizeof(grpc_address) * addresses->num_addresses); + return new; +} + void grpc_addresses_set_address(grpc_addresses *addresses, size_t index, void *address, size_t address_len, bool is_balancer) { diff --git a/src/core/ext/client_config/resolver_result.h b/src/core/ext/client_config/resolver_result.h index df92bec984..b1a3457565 100644 --- a/src/core/ext/client_config/resolver_result.h +++ b/src/core/ext/client_config/resolver_result.h @@ -54,6 +54,8 @@ typedef struct grpc_addresses { \a num_addresses addresses. */ grpc_addresses *grpc_addresses_create(size_t num_addresses); +grpc_addresses *grpc_addresses_copy(grpc_addresses* addresses); + void grpc_addresses_set_address(grpc_addresses *addresses, size_t index, void *address, size_t address_len, bool is_balancer); diff --git a/src/core/ext/resolver/sockaddr/sockaddr_resolver.c b/src/core/ext/resolver/sockaddr/sockaddr_resolver.c index 328c7cb6f9..5c59e4397a 100644 --- a/src/core/ext/resolver/sockaddr/sockaddr_resolver.c +++ b/src/core/ext/resolver/sockaddr/sockaddr_resolver.c @@ -120,8 +120,8 @@ static void sockaddr_maybe_finish_next_locked(grpc_exec_ctx *exec_ctx, sockaddr_resolver *r) { if (r->next_completion != NULL && !r->published) { r->published = true; - *r->target_result = - grpc_resolver_result_create(r->addresses, r->lb_policy_name); + *r->target_result = grpc_resolver_result_create( + grpc_addresses_copy(r->addresses), r->lb_policy_name); grpc_exec_ctx_sched(exec_ctx, r->next_completion, GRPC_ERROR_NONE, NULL); r->next_completion = NULL; } -- cgit v1.2.3 From 1621c26c1ec49207e994e4b11671fb8fb38225ba Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 15 Sep 2016 12:51:29 -0700 Subject: Allow resolver to pass channel args to LB policy. --- src/core/ext/client_config/client_channel.c | 2 ++ src/core/ext/client_config/lb_policy_factory.h | 3 +++ src/core/ext/client_config/resolver_result.c | 15 +++++++++++++-- src/core/ext/client_config/resolver_result.h | 21 ++++++++++++++++++--- src/core/ext/resolver/dns/native/dns_resolver.c | 2 +- src/core/ext/resolver/sockaddr/sockaddr_resolver.c | 2 +- 6 files changed, 38 insertions(+), 7 deletions(-) (limited to 'src/core/ext/resolver') diff --git a/src/core/ext/client_config/client_channel.c b/src/core/ext/client_config/client_channel.c index 2e6f253d38..0343a5e254 100644 --- a/src/core/ext/client_config/client_channel.c +++ b/src/core/ext/client_config/client_channel.c @@ -179,6 +179,8 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg, grpc_lb_policy_args lb_policy_args; lb_policy_args.addresses = grpc_resolver_result_get_addresses(chand->resolver_result); + lb_policy_args.additional_args = + grpc_resolver_result_get_lb_policy_args(chand->resolver_result); lb_policy_args.client_channel_factory = chand->client_channel_factory; lb_policy = grpc_lb_policy_create( exec_ctx, diff --git a/src/core/ext/client_config/lb_policy_factory.h b/src/core/ext/client_config/lb_policy_factory.h index 6919b1eb98..e2364c31a8 100644 --- a/src/core/ext/client_config/lb_policy_factory.h +++ b/src/core/ext/client_config/lb_policy_factory.h @@ -47,9 +47,12 @@ struct grpc_lb_policy_factory { const grpc_lb_policy_factory_vtable *vtable; }; +// TODO(roth, ctiller): Consider replacing this struct with +// grpc_channel_args. See comment in resolver_result.h for details. typedef struct grpc_lb_policy_args { grpc_addresses *addresses; grpc_client_channel_factory *client_channel_factory; + grpc_channel_args *additional_args; } grpc_lb_policy_args; struct grpc_lb_policy_factory_vtable { diff --git a/src/core/ext/client_config/resolver_result.c b/src/core/ext/client_config/resolver_result.c index ac263b9a46..f97dc467e1 100644 --- a/src/core/ext/client_config/resolver_result.c +++ b/src/core/ext/client_config/resolver_result.c @@ -36,6 +36,8 @@ #include #include +#include "src/core/lib/channel/channel_args.h" + grpc_addresses* grpc_addresses_create(size_t num_addresses) { grpc_addresses* addresses = gpr_malloc(sizeof(grpc_addresses)); addresses->num_addresses = num_addresses; @@ -71,15 +73,18 @@ struct grpc_resolver_result { gpr_refcount refs; grpc_addresses* addresses; char* lb_policy_name; + grpc_channel_args* lb_policy_args; }; -grpc_resolver_result* grpc_resolver_result_create(grpc_addresses* addresses, - const char* lb_policy_name) { +grpc_resolver_result* grpc_resolver_result_create( + grpc_addresses* addresses, const char* lb_policy_name, + grpc_channel_args* lb_policy_args) { grpc_resolver_result* result = gpr_malloc(sizeof(*result)); memset(result, 0, sizeof(*result)); gpr_ref_init(&result->refs, 1); result->addresses = addresses; result->lb_policy_name = gpr_strdup(lb_policy_name); + result->lb_policy_args = lb_policy_args; return result; } @@ -92,6 +97,7 @@ void grpc_resolver_result_unref(grpc_exec_ctx* exec_ctx, if (gpr_unref(&result->refs)) { grpc_addresses_destroy(result->addresses); gpr_free(result->lb_policy_name); + grpc_channel_args_destroy(result->lb_policy_args); gpr_free(result); } } @@ -105,3 +111,8 @@ const char* grpc_resolver_result_get_lb_policy_name( grpc_resolver_result* result) { return result->lb_policy_name; } + +grpc_channel_args* grpc_resolver_result_get_lb_policy_args( + grpc_resolver_result* result) { + return result->lb_policy_args; +} diff --git a/src/core/ext/client_config/resolver_result.h b/src/core/ext/client_config/resolver_result.h index fa60c8cc6d..821208f709 100644 --- a/src/core/ext/client_config/resolver_result.h +++ b/src/core/ext/client_config/resolver_result.h @@ -37,6 +37,16 @@ #include "src/core/ext/client_config/lb_policy.h" #include "src/core/lib/iomgr/resolve_address.h" +// TODO(roth, ctiller): In the long term, we are considering replacing +// the resolver_result data structure with grpc_channel_args. The idea is +// that the resolver will return a set of channel args that contains the +// information that is currently in the resolver_result struct. For +// example, there will be specific args indicating the set of addresses +// and the name of the LB policy to instantiate. Note that if we did +// this, we would probably want to change the data structure of +// grpc_channel_args such to a hash table or AVL or some other data +// structure that does not require linear search to find keys. + /// Used to represent addresses returned by the resolver. typedef struct grpc_address { grpc_resolved_address address; @@ -63,9 +73,10 @@ void grpc_addresses_destroy(grpc_addresses* addresses); /// Results reported from a grpc_resolver. typedef struct grpc_resolver_result grpc_resolver_result; -/// Takes ownership of \a addresses. -grpc_resolver_result* grpc_resolver_result_create(grpc_addresses* addresses, - const char* lb_policy_name); +/// Takes ownership of \a addresses and \a lb_policy_args. +grpc_resolver_result* grpc_resolver_result_create( + grpc_addresses* addresses, const char* lb_policy_name, + grpc_channel_args* lb_policy_args); void grpc_resolver_result_ref(grpc_resolver_result* result); void grpc_resolver_result_unref(grpc_exec_ctx* exec_ctx, grpc_resolver_result* result); @@ -78,4 +89,8 @@ grpc_addresses* grpc_resolver_result_get_addresses( const char* grpc_resolver_result_get_lb_policy_name( grpc_resolver_result* result); +/// Caller does NOT take ownership of result. +grpc_channel_args* grpc_resolver_result_get_lb_policy_args( + grpc_resolver_result* result); + #endif /* GRPC_CORE_EXT_CLIENT_CONFIG_RESOLVER_RESULT_H */ diff --git a/src/core/ext/resolver/dns/native/dns_resolver.c b/src/core/ext/resolver/dns/native/dns_resolver.c index 78a996788d..103d3effbd 100644 --- a/src/core/ext/resolver/dns/native/dns_resolver.c +++ b/src/core/ext/resolver/dns/native/dns_resolver.c @@ -174,7 +174,7 @@ static void dns_on_resolved(grpc_exec_ctx *exec_ctx, void *arg, false /* is_balancer */); } grpc_resolved_addresses_destroy(r->addresses); - result = grpc_resolver_result_create(addresses, r->lb_policy_name); + result = grpc_resolver_result_create(addresses, r->lb_policy_name, NULL); } else { gpr_timespec now = gpr_now(GPR_CLOCK_MONOTONIC); gpr_timespec next_try = gpr_backoff_step(&r->backoff_state, now); diff --git a/src/core/ext/resolver/sockaddr/sockaddr_resolver.c b/src/core/ext/resolver/sockaddr/sockaddr_resolver.c index 5c59e4397a..fb67f1a227 100644 --- a/src/core/ext/resolver/sockaddr/sockaddr_resolver.c +++ b/src/core/ext/resolver/sockaddr/sockaddr_resolver.c @@ -121,7 +121,7 @@ static void sockaddr_maybe_finish_next_locked(grpc_exec_ctx *exec_ctx, if (r->next_completion != NULL && !r->published) { r->published = true; *r->target_result = grpc_resolver_result_create( - grpc_addresses_copy(r->addresses), r->lb_policy_name); + grpc_addresses_copy(r->addresses), r->lb_policy_name, NULL); grpc_exec_ctx_sched(exec_ctx, r->next_completion, GRPC_ERROR_NONE, NULL); r->next_completion = NULL; } -- cgit v1.2.3