From fa0896bbd0323d18b4e41805510b23d0f42fc34c Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Fri, 23 Sep 2016 16:10:19 -0700 Subject: [PATCH 1/2] Rewrote handling of default resolver prefix --- .../ext/client_config/client_config_plugin.c | 6 +---- .../ext/client_config/resolver_registry.c | 22 ++++++++++++------- .../ext/client_config/resolver_registry.h | 5 ++++- test/core/surface/channel_create_test.c | 2 +- .../core/surface/secure_channel_create_test.c | 2 +- 5 files changed, 21 insertions(+), 16 deletions(-) diff --git a/src/core/ext/client_config/client_config_plugin.c b/src/core/ext/client_config/client_config_plugin.c index 5e316134205..dc3629d3833 100644 --- a/src/core/ext/client_config/client_config_plugin.c +++ b/src/core/ext/client_config/client_config_plugin.c @@ -43,10 +43,6 @@ #include "src/core/ext/client_config/subchannel_index.h" #include "src/core/lib/surface/channel_init.h" -#ifndef GRPC_DEFAULT_NAME_PREFIX -#define GRPC_DEFAULT_NAME_PREFIX "dns:///" -#endif - static bool append_filter(grpc_channel_stack_builder *builder, void *arg) { return grpc_channel_stack_builder_append_filter( builder, (const grpc_channel_filter *)arg, NULL, NULL); @@ -79,7 +75,7 @@ static bool set_default_host_if_unset(grpc_channel_stack_builder *builder, void grpc_client_config_init(void) { grpc_lb_policy_registry_init(); - grpc_resolver_registry_init(GRPC_DEFAULT_NAME_PREFIX); + grpc_resolver_registry_init(); grpc_subchannel_index_init(); grpc_channel_init_register_stage(GRPC_CLIENT_CHANNEL, INT_MIN, set_default_host_if_unset, NULL); diff --git a/src/core/ext/client_config/resolver_registry.c b/src/core/ext/client_config/resolver_registry.c index b5308a140c9..8d8e2d9d8c0 100644 --- a/src/core/ext/client_config/resolver_registry.c +++ b/src/core/ext/client_config/resolver_registry.c @@ -40,22 +40,20 @@ #include #define MAX_RESOLVERS 10 +#define DEFAULT_RESOLVER_PREFIX_MAX_LENGTH 32 static grpc_resolver_factory *g_all_of_the_resolvers[MAX_RESOLVERS]; static int g_number_of_resolvers = 0; -static char *g_default_resolver_prefix; +static char g_default_resolver_prefix[DEFAULT_RESOLVER_PREFIX_MAX_LENGTH] = + "dns:///"; -void grpc_resolver_registry_init(const char *default_resolver_prefix) { - g_default_resolver_prefix = gpr_strdup(default_resolver_prefix); -} +void grpc_resolver_registry_init() {} void grpc_resolver_registry_shutdown(void) { - int i; - for (i = 0; i < g_number_of_resolvers; i++) { + for (int i = 0; i < g_number_of_resolvers; i++) { grpc_resolver_factory_unref(g_all_of_the_resolvers[i]); } - gpr_free(g_default_resolver_prefix); // FIXME(ctiller): this should live in grpc_resolver_registry_init, // however that would have the client_config plugin call this AFTER we start // registering resolvers from third party plugins, and so they'd never show @@ -65,6 +63,14 @@ void grpc_resolver_registry_shutdown(void) { g_number_of_resolvers = 0; } +void grpc_resolver_registry_set_default_prefix( + const char *default_resolver_prefix) { + GPR_ASSERT(strlen(default_resolver_prefix) < + DEFAULT_RESOLVER_PREFIX_MAX_LENGTH); + memcpy(g_default_resolver_prefix, default_resolver_prefix, + DEFAULT_RESOLVER_PREFIX_MAX_LENGTH); +} + void grpc_register_resolver_type(grpc_resolver_factory *factory) { int i; for (i = 0; i < g_number_of_resolvers; i++) { @@ -108,7 +114,7 @@ static grpc_resolver_factory *resolve_factory(const char *target, *uri = grpc_uri_parse(target, 1); factory = lookup_factory_by_uri(*uri); if (factory == NULL) { - if (g_default_resolver_prefix != NULL) { + if (g_default_resolver_prefix[0] != '\0') { grpc_uri_destroy(*uri); gpr_asprintf(&tmp, "%s%s", g_default_resolver_prefix, target); *uri = grpc_uri_parse(tmp, 1); diff --git a/src/core/ext/client_config/resolver_registry.h b/src/core/ext/client_config/resolver_registry.h index 92e248d5488..4c6279b9789 100644 --- a/src/core/ext/client_config/resolver_registry.h +++ b/src/core/ext/client_config/resolver_registry.h @@ -36,9 +36,12 @@ #include "src/core/ext/client_config/resolver_factory.h" -void grpc_resolver_registry_init(const char *default_prefix); +void grpc_resolver_registry_init(); void grpc_resolver_registry_shutdown(void); +/** Set the default URI prefix to \a default_prefix. */ +void grpc_resolver_registry_set_default_prefix(const char *default_prefix); + /** Register a resolver type. URI's of \a scheme will be resolved with the given resolver. If \a priority is greater than zero, then the resolver will be eligible diff --git a/test/core/surface/channel_create_test.c b/test/core/surface/channel_create_test.c index 450cc372339..580eb303f68 100644 --- a/test/core/surface/channel_create_test.c +++ b/test/core/surface/channel_create_test.c @@ -40,7 +40,7 @@ void test_unknown_scheme_target(void) { grpc_channel *chan; /* avoid default prefix */ grpc_resolver_registry_shutdown(); - grpc_resolver_registry_init(""); + grpc_resolver_registry_init(); chan = grpc_insecure_channel_create("blah://blah", NULL, NULL); GPR_ASSERT(chan != NULL); diff --git a/test/core/surface/secure_channel_create_test.c b/test/core/surface/secure_channel_create_test.c index b9525031678..f8a9a642115 100644 --- a/test/core/surface/secure_channel_create_test.c +++ b/test/core/surface/secure_channel_create_test.c @@ -46,7 +46,7 @@ void test_unknown_scheme_target(void) { grpc_channel *chan; grpc_channel_credentials *creds; grpc_resolver_registry_shutdown(); - grpc_resolver_registry_init(""); + grpc_resolver_registry_init(); creds = grpc_fake_transport_security_credentials_create(); chan = grpc_secure_channel_create(creds, "blah://blah", NULL, NULL); From a60dcca5eda95df5978bf73316f365d952764418 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Mon, 26 Sep 2016 11:09:29 -0700 Subject: [PATCH 2/2] PR comments --- .../ext/client_config/resolver_registry.c | 33 +++++++++---------- 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/src/core/ext/client_config/resolver_registry.c b/src/core/ext/client_config/resolver_registry.c index 8d8e2d9d8c0..bd5c683878d 100644 --- a/src/core/ext/client_config/resolver_registry.c +++ b/src/core/ext/client_config/resolver_registry.c @@ -65,10 +65,13 @@ void grpc_resolver_registry_shutdown(void) { void grpc_resolver_registry_set_default_prefix( const char *default_resolver_prefix) { - GPR_ASSERT(strlen(default_resolver_prefix) < - DEFAULT_RESOLVER_PREFIX_MAX_LENGTH); - memcpy(g_default_resolver_prefix, default_resolver_prefix, - DEFAULT_RESOLVER_PREFIX_MAX_LENGTH); + const size_t len = strlen(default_resolver_prefix); + GPR_ASSERT(len < DEFAULT_RESOLVER_PREFIX_MAX_LENGTH && + "default resolver prefix too long"); + GPR_ASSERT(len > 0 && "default resolver prefix can't be empty"); + // By the previous assert, default_resolver_prefix is safe to be copied with a + // plain strcpy. + strcpy(g_default_resolver_prefix, default_resolver_prefix); } void grpc_register_resolver_type(grpc_resolver_factory *factory) { @@ -114,22 +117,16 @@ static grpc_resolver_factory *resolve_factory(const char *target, *uri = grpc_uri_parse(target, 1); factory = lookup_factory_by_uri(*uri); if (factory == NULL) { - if (g_default_resolver_prefix[0] != '\0') { - grpc_uri_destroy(*uri); - gpr_asprintf(&tmp, "%s%s", g_default_resolver_prefix, target); - *uri = grpc_uri_parse(tmp, 1); - factory = lookup_factory_by_uri(*uri); - if (factory == NULL) { - grpc_uri_destroy(grpc_uri_parse(target, 0)); - grpc_uri_destroy(grpc_uri_parse(tmp, 0)); - gpr_log(GPR_ERROR, "don't know how to resolve '%s' or '%s'", target, - tmp); - } - gpr_free(tmp); - } else { + grpc_uri_destroy(*uri); + gpr_asprintf(&tmp, "%s%s", g_default_resolver_prefix, target); + *uri = grpc_uri_parse(tmp, 1); + factory = lookup_factory_by_uri(*uri); + if (factory == NULL) { grpc_uri_destroy(grpc_uri_parse(target, 0)); - gpr_log(GPR_ERROR, "don't know how to resolve '%s'", target); + grpc_uri_destroy(grpc_uri_parse(tmp, 0)); + gpr_log(GPR_ERROR, "don't know how to resolve '%s' or '%s'", target, tmp); } + gpr_free(tmp); } return factory; }