From 21d4b2d930642b2b8d272352dfa982b5102efd1e Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 18 Nov 2016 09:53:41 -0800 Subject: [PATCH 01/13] Pass client channel factory and server name via channel args. --- include/grpc/impl/codegen/grpc_types.h | 2 + src/core/ext/client_channel/client_channel.c | 48 +++++-------- src/core/ext/client_channel/client_channel.h | 7 -- .../chttp2/client/insecure/channel_create.c | 50 ++++++++------ .../client/secure/secure_channel_create.c | 67 ++++++++++++------- 5 files changed, 93 insertions(+), 81 deletions(-) diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index 8c5215faee889..5ecc5ba04352d 100644 --- a/include/grpc/impl/codegen/grpc_types.h +++ b/include/grpc/impl/codegen/grpc_types.h @@ -220,6 +220,8 @@ typedef struct { #define GRPC_ARG_LB_ADDRESSES "grpc.lb_addresses" /** The grpc_socket_mutator instance that set the socket options. A pointer. */ #define GRPC_ARG_SOCKET_MUTATOR "grpc.socket_mutator" +/** Client channel factory. Not intended for external use. */ +#define GRPC_ARG_CLIENT_CHANNEL_FACTORY "grpc.client_channel_factory" /** \} */ /** Result of a grpc call. If the caller satisfies the prerequisites of a diff --git a/src/core/ext/client_channel/client_channel.c b/src/core/ext/client_channel/client_channel.c index b66fed4b88a3a..a607f73c04cb9 100644 --- a/src/core/ext/client_channel/client_channel.c +++ b/src/core/ext/client_channel/client_channel.c @@ -44,6 +44,7 @@ #include #include "src/core/ext/client_channel/lb_policy_registry.h" +#include "src/core/ext/client_channel/resolver_registry.h" #include "src/core/ext/client_channel/subchannel.h" #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/channel/connected_channel.h" @@ -454,20 +455,31 @@ static void cc_init_channel_elem(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, grpc_channel_element_args *args) { channel_data *chand = elem->channel_data; - memset(chand, 0, sizeof(*chand)); - GPR_ASSERT(args->is_last); GPR_ASSERT(elem->filter == &grpc_client_channel_filter); - + // Initialize data members. gpr_mu_init(&chand->mu); + chand->owning_stack = args->channel_stack; grpc_closure_init(&chand->on_resolver_result_changed, on_resolver_result_changed, chand); - chand->owning_stack = args->channel_stack; - + chand->interested_parties = grpc_pollset_set_create(); grpc_connectivity_state_init(&chand->state_tracker, GRPC_CHANNEL_IDLE, "client_channel"); - chand->interested_parties = grpc_pollset_set_create(); + // Record client channel factory. + const grpc_arg *arg = grpc_channel_args_find(args->channel_args, + GRPC_ARG_CLIENT_CHANNEL_FACTORY); + GPR_ASSERT(arg != NULL); + GPR_ASSERT(arg->type == GRPC_ARG_POINTER); + grpc_client_channel_factory_ref(arg->value.pointer.p); + chand->client_channel_factory = arg->value.pointer.p; + // Instantiate resolver. + arg = grpc_channel_args_find(args->channel_args, GRPC_ARG_SERVER_NAME); + GPR_ASSERT(arg != NULL); + GPR_ASSERT(arg->type == GRPC_ARG_STRING); + chand->resolver = grpc_resolver_create(arg->value.string, args->channel_args); +// FIXME: return failure instead of asserting + GPR_ASSERT(chand->resolver != NULL); } /* Destructor for channel_data */ @@ -1080,30 +1092,6 @@ const grpc_channel_filter grpc_client_channel_filter = { "client-channel", }; -void grpc_client_channel_finish_initialization( - grpc_exec_ctx *exec_ctx, grpc_channel_stack *channel_stack, - grpc_resolver *resolver, - grpc_client_channel_factory *client_channel_factory) { - /* post construction initialization: set the transport setup pointer */ - GPR_ASSERT(client_channel_factory != NULL); - grpc_channel_element *elem = grpc_channel_stack_last_element(channel_stack); - channel_data *chand = elem->channel_data; - gpr_mu_lock(&chand->mu); - GPR_ASSERT(!chand->resolver); - chand->resolver = resolver; - GRPC_RESOLVER_REF(resolver, "channel"); - if (!grpc_closure_list_empty(chand->waiting_for_config_closures) || - chand->exit_idle_when_lb_policy_arrives) { - chand->started_resolving = true; - GRPC_CHANNEL_STACK_REF(chand->owning_stack, "resolver"); - grpc_resolver_next(exec_ctx, resolver, &chand->resolver_result, - &chand->on_resolver_result_changed); - } - chand->client_channel_factory = client_channel_factory; - grpc_client_channel_factory_ref(client_channel_factory); - gpr_mu_unlock(&chand->mu); -} - grpc_connectivity_state grpc_client_channel_check_connectivity_state( grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, int try_to_connect) { channel_data *chand = elem->channel_data; diff --git a/src/core/ext/client_channel/client_channel.h b/src/core/ext/client_channel/client_channel.h index ab5a84fdfbe13..9ba012865cfac 100644 --- a/src/core/ext/client_channel/client_channel.h +++ b/src/core/ext/client_channel/client_channel.h @@ -47,13 +47,6 @@ extern const grpc_channel_filter grpc_client_channel_filter; -/* Post-construction initializer to give the client channel its resolver - and factory. */ -void grpc_client_channel_finish_initialization( - grpc_exec_ctx *exec_ctx, grpc_channel_stack *channel_stack, - grpc_resolver *resolver, - grpc_client_channel_factory *client_channel_factory); - grpc_connectivity_state grpc_client_channel_check_connectivity_state( grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, int try_to_connect); diff --git a/src/core/ext/transport/chttp2/client/insecure/channel_create.c b/src/core/ext/transport/chttp2/client/insecure/channel_create.c index 8e03fd82c11d2..d448b909920bb 100644 --- a/src/core/ext/transport/chttp2/client/insecure/channel_create.c +++ b/src/core/ext/transport/chttp2/client/insecure/channel_create.c @@ -42,7 +42,6 @@ #include "src/core/ext/client_channel/client_channel.h" #include "src/core/ext/client_channel/http_connect_handshaker.h" -#include "src/core/ext/client_channel/resolver_registry.h" #include "src/core/ext/transport/chttp2/transport/chttp2_transport.h" #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/channel/compress_filter.h" @@ -195,20 +194,7 @@ static grpc_channel *client_channel_factory_create_channel( grpc_exec_ctx *exec_ctx, grpc_client_channel_factory *cc_factory, const char *target, grpc_client_channel_type type, const grpc_channel_args *args) { - grpc_channel *channel = - grpc_channel_create(exec_ctx, target, args, GRPC_CLIENT_CHANNEL, NULL); - grpc_resolver *resolver = grpc_resolver_create(target, args); - if (!resolver) { - GRPC_CHANNEL_INTERNAL_UNREF(exec_ctx, channel, - "client_channel_factory_create_channel"); - return NULL; - } - - grpc_client_channel_finish_initialization( - exec_ctx, grpc_channel_get_channel_stack(channel), resolver, cc_factory); - GRPC_RESOLVER_UNREF(exec_ctx, resolver, "create_channel"); - - return channel; + return grpc_channel_create(exec_ctx, target, args, GRPC_CLIENT_CHANNEL, NULL); } static const grpc_client_channel_factory_vtable client_channel_factory_vtable = @@ -219,6 +205,21 @@ static const grpc_client_channel_factory_vtable client_channel_factory_vtable = static grpc_client_channel_factory client_channel_factory = { &client_channel_factory_vtable}; +static void *cc_factory_arg_copy(void *cc_factory) { + return cc_factory; +} + +static void cc_factory_arg_destroy(void *cc_factory) {} + +static int cc_factory_arg_cmp(void *cc_factory1, void *cc_factory2) { + if (cc_factory1 < cc_factory2) return -1; + if (cc_factory1 > cc_factory2) return 1; + return 0; +} + +static const grpc_arg_pointer_vtable cc_factory_arg_vtable = { + cc_factory_arg_copy, cc_factory_arg_destroy, cc_factory_arg_cmp}; + /* Create a client channel: Asynchronously: - resolve target - connect to it (trying alternatives as presented) @@ -231,15 +232,26 @@ grpc_channel *grpc_insecure_channel_create(const char *target, "grpc_insecure_channel_create(target=%p, args=%p, reserved=%p)", 3, (target, args, reserved)); GPR_ASSERT(!reserved); - grpc_client_channel_factory *factory = (grpc_client_channel_factory *)&client_channel_factory; + // Add channel args containing the server name and client channel factory. + grpc_arg new_args[2]; + new_args[0].type = GRPC_ARG_STRING; + new_args[0].key = GRPC_ARG_SERVER_NAME; + new_args[0].value.string = (char *)target; + new_args[1].type = GRPC_ARG_POINTER; + new_args[1].key = GRPC_ARG_CLIENT_CHANNEL_FACTORY; + new_args[1].value.pointer.p = factory; + new_args[1].value.pointer.vtable = &cc_factory_arg_vtable; + grpc_channel_args *args_copy = + grpc_channel_args_copy_and_add(args, new_args, GPR_ARRAY_SIZE(new_args)); + // Create channel. grpc_channel *channel = client_channel_factory_create_channel( - &exec_ctx, factory, target, GRPC_CLIENT_CHANNEL_TYPE_REGULAR, args); - + &exec_ctx, factory, target, GRPC_CLIENT_CHANNEL_TYPE_REGULAR, args_copy); + // Clean up. + grpc_channel_args_destroy(args_copy); grpc_client_channel_factory_unref(&exec_ctx, factory); grpc_exec_ctx_finish(&exec_ctx); - return channel != NULL ? channel : grpc_lame_client_channel_create( target, GRPC_STATUS_INTERNAL, "Failed to create client channel"); diff --git a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c index 04c88a2d36879..b53e637fc2587 100644 --- a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c +++ b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c @@ -42,7 +42,6 @@ #include "src/core/ext/client_channel/client_channel.h" #include "src/core/ext/client_channel/http_connect_handshaker.h" -#include "src/core/ext/client_channel/resolver_registry.h" #include "src/core/ext/transport/chttp2/transport/chttp2_transport.h" #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/channel/handshaker.h" @@ -273,20 +272,7 @@ static grpc_channel *client_channel_factory_create_channel( grpc_exec_ctx *exec_ctx, grpc_client_channel_factory *cc_factory, const char *target, grpc_client_channel_type type, const grpc_channel_args *args) { - client_channel_factory *f = (client_channel_factory *)cc_factory; - grpc_channel *channel = - grpc_channel_create(exec_ctx, target, args, GRPC_CLIENT_CHANNEL, NULL); - grpc_resolver *resolver = grpc_resolver_create(target, args); - if (resolver != NULL) { - grpc_client_channel_finish_initialization( - exec_ctx, grpc_channel_get_channel_stack(channel), resolver, &f->base); - GRPC_RESOLVER_UNREF(exec_ctx, resolver, "create"); - } else { - GRPC_CHANNEL_INTERNAL_UNREF(exec_ctx, channel, - "client_channel_factory_create_channel"); - channel = NULL; - } - return channel; + return grpc_channel_create(exec_ctx, target, args, GRPC_CLIENT_CHANNEL, NULL); } static const grpc_client_channel_factory_vtable client_channel_factory_vtable = @@ -294,6 +280,28 @@ static const grpc_client_channel_factory_vtable client_channel_factory_vtable = client_channel_factory_create_subchannel, client_channel_factory_create_channel}; +static void *cc_factory_arg_copy(void *cc_factory) { + client_channel_factory_ref(cc_factory); + return cc_factory; +} + +static void cc_factory_arg_destroy(void *cc_factory) { + // TODO(roth): remove local exec_ctx when + // https://github.com/grpc/grpc/pull/8705 is merged + grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; + client_channel_factory_unref(&exec_ctx, cc_factory); + grpc_exec_ctx_finish(&exec_ctx); +} + +static int cc_factory_arg_cmp(void *cc_factory1, void *cc_factory2) { + if (cc_factory1 < cc_factory2) return -1; + if (cc_factory1 > cc_factory2) return 1; + return 0; +} + +static const grpc_arg_pointer_vtable cc_factory_arg_vtable = { + cc_factory_arg_copy, cc_factory_arg_destroy, cc_factory_arg_cmp}; + /* Create a secure client channel: Asynchronously: - resolve target - connect to it (trying alternatives as presented) @@ -326,14 +334,6 @@ grpc_channel *grpc_secure_channel_create(grpc_channel_credentials *creds, return grpc_lame_client_channel_create( target, GRPC_STATUS_INTERNAL, "Failed to create security connector."); } - grpc_arg connector_arg = - grpc_security_connector_to_arg(&security_connector->base); - grpc_channel_args *new_args = grpc_channel_args_copy_and_add( - new_args_from_connector != NULL ? new_args_from_connector : args, - &connector_arg, 1); - if (new_args_from_connector != NULL) { - grpc_channel_args_destroy(new_args_from_connector); - } // Create client channel factory. client_channel_factory *f = gpr_malloc(sizeof(*f)); memset(f, 0, sizeof(*f)); @@ -342,13 +342,30 @@ grpc_channel *grpc_secure_channel_create(grpc_channel_credentials *creds, GRPC_SECURITY_CONNECTOR_REF(&security_connector->base, "grpc_secure_channel_create"); f->security_connector = security_connector; + // Add channel args containing the server name, client channel + // factory, and security connector. + grpc_arg new_args[3]; + new_args[0].type = GRPC_ARG_STRING; + new_args[0].key = GRPC_ARG_SERVER_NAME; + new_args[0].value.string = (char *)target; + new_args[1].type = GRPC_ARG_POINTER; + new_args[1].key = GRPC_ARG_CLIENT_CHANNEL_FACTORY; + new_args[1].value.pointer.p = f; + new_args[1].value.pointer.vtable = &cc_factory_arg_vtable; + new_args[2] = grpc_security_connector_to_arg(&security_connector->base); + grpc_channel_args *args_copy = grpc_channel_args_copy_and_add( + new_args_from_connector != NULL ? new_args_from_connector : args, + new_args, GPR_ARRAY_SIZE(new_args)); + if (new_args_from_connector != NULL) { + grpc_channel_args_destroy(new_args_from_connector); + } // Create channel. grpc_channel *channel = client_channel_factory_create_channel( - &exec_ctx, &f->base, target, GRPC_CLIENT_CHANNEL_TYPE_REGULAR, new_args); + &exec_ctx, &f->base, target, GRPC_CLIENT_CHANNEL_TYPE_REGULAR, args_copy); // Clean up. GRPC_SECURITY_CONNECTOR_UNREF(&f->security_connector->base, "secure_client_channel_factory_create_channel"); - grpc_channel_args_destroy(new_args); + grpc_channel_args_destroy(args_copy); grpc_client_channel_factory_unref(&exec_ctx, &f->base); grpc_exec_ctx_finish(&exec_ctx); return channel; /* may be NULL */ From 86e905901fa1331f2326f0a881dd79322eeff6c8 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 18 Nov 2016 09:56:40 -0800 Subject: [PATCH 02/13] Avoid confusion between server name and URI. --- include/grpc/impl/codegen/grpc_types.h | 2 ++ src/core/ext/client_channel/client_channel.c | 2 +- src/core/ext/transport/chttp2/client/insecure/channel_create.c | 2 +- .../ext/transport/chttp2/client/secure/secure_channel_create.c | 2 +- 4 files changed, 5 insertions(+), 3 deletions(-) diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index 5ecc5ba04352d..9a9118ceaf3c6 100644 --- a/include/grpc/impl/codegen/grpc_types.h +++ b/include/grpc/impl/codegen/grpc_types.h @@ -213,6 +213,8 @@ typedef struct { #define GRPC_ARG_SERVICE_CONFIG "grpc.service_config" /** LB policy name. */ #define GRPC_ARG_LB_POLICY_NAME "grpc.lb_policy_name" +/** Server URI. Not intended for external use. */ +#define GRPC_ARG_SERVER_URI "grpc.server_uri" /** Server name. Not intended for external use. */ #define GRPC_ARG_SERVER_NAME "grpc.server_name" /** Resolved addresses in a form used by the LB policy. diff --git a/src/core/ext/client_channel/client_channel.c b/src/core/ext/client_channel/client_channel.c index a607f73c04cb9..7df3bca5d8b01 100644 --- a/src/core/ext/client_channel/client_channel.c +++ b/src/core/ext/client_channel/client_channel.c @@ -474,7 +474,7 @@ static void cc_init_channel_elem(grpc_exec_ctx *exec_ctx, grpc_client_channel_factory_ref(arg->value.pointer.p); chand->client_channel_factory = arg->value.pointer.p; // Instantiate resolver. - arg = grpc_channel_args_find(args->channel_args, GRPC_ARG_SERVER_NAME); + arg = grpc_channel_args_find(args->channel_args, GRPC_ARG_SERVER_URI); GPR_ASSERT(arg != NULL); GPR_ASSERT(arg->type == GRPC_ARG_STRING); chand->resolver = grpc_resolver_create(arg->value.string, args->channel_args); diff --git a/src/core/ext/transport/chttp2/client/insecure/channel_create.c b/src/core/ext/transport/chttp2/client/insecure/channel_create.c index d448b909920bb..0d2395266b14b 100644 --- a/src/core/ext/transport/chttp2/client/insecure/channel_create.c +++ b/src/core/ext/transport/chttp2/client/insecure/channel_create.c @@ -237,7 +237,7 @@ grpc_channel *grpc_insecure_channel_create(const char *target, // Add channel args containing the server name and client channel factory. grpc_arg new_args[2]; new_args[0].type = GRPC_ARG_STRING; - new_args[0].key = GRPC_ARG_SERVER_NAME; + new_args[0].key = GRPC_ARG_SERVER_URI; new_args[0].value.string = (char *)target; new_args[1].type = GRPC_ARG_POINTER; new_args[1].key = GRPC_ARG_CLIENT_CHANNEL_FACTORY; diff --git a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c index b53e637fc2587..be57f30bd0adb 100644 --- a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c +++ b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c @@ -346,7 +346,7 @@ grpc_channel *grpc_secure_channel_create(grpc_channel_credentials *creds, // factory, and security connector. grpc_arg new_args[3]; new_args[0].type = GRPC_ARG_STRING; - new_args[0].key = GRPC_ARG_SERVER_NAME; + new_args[0].key = GRPC_ARG_SERVER_URI; new_args[0].value.string = (char *)target; new_args[1].type = GRPC_ARG_POINTER; new_args[1].key = GRPC_ARG_CLIENT_CHANNEL_FACTORY; From 5e2566e92b0603cfa6c6a989ab7e2372525ca03d Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 18 Nov 2016 10:53:13 -0800 Subject: [PATCH 03/13] Change destroy_call_elem() to return a grpc_error*. --- src/core/ext/census/grpc_filter.c | 7 +++--- src/core/ext/client_channel/client_channel.c | 12 ++++++---- src/core/ext/client_channel/subchannel.c | 16 +++++++++---- .../load_reporting/load_reporting_filter.c | 8 ++++--- src/core/lib/channel/channel_stack.c | 24 ++++++++++++------- src/core/lib/channel/channel_stack.h | 17 ++++++------- src/core/lib/channel/channel_stack_builder.c | 23 +++++++++--------- src/core/lib/channel/channel_stack_builder.h | 11 ++++----- src/core/lib/channel/compress_filter.c | 7 +++--- src/core/lib/channel/connected_channel.c | 7 +++--- src/core/lib/channel/deadline_filter.c | 7 +++--- src/core/lib/channel/http_client_filter.c | 7 +++--- src/core/lib/channel/http_server_filter.c | 7 +++--- src/core/lib/channel/message_size_filter.c | 7 +++--- .../security/transport/client_auth_filter.c | 7 +++--- .../security/transport/server_auth_filter.c | 7 +++--- src/core/lib/surface/channel.c | 15 ++++++++---- src/core/lib/surface/lame_client.c | 7 +++--- src/core/lib/surface/server.c | 7 +++--- .../end2end/tests/filter_call_init_fails.c | 8 ++++--- test/core/end2end/tests/filter_causes_close.c | 8 ++++--- test/core/end2end/tests/filter_latency.c | 8 ++++--- 22 files changed, 133 insertions(+), 94 deletions(-) diff --git a/src/core/ext/census/grpc_filter.c b/src/core/ext/census/grpc_filter.c index 397dbc40a871f..c73385f98b84a 100644 --- a/src/core/ext/census/grpc_filter.c +++ b/src/core/ext/census/grpc_filter.c @@ -167,11 +167,12 @@ static void server_destroy_call_elem(grpc_exec_ctx *exec_ctx, /* TODO(hongyu): record rpc server stats and census_tracing_end_op here */ } -static void init_channel_elem(grpc_exec_ctx *exec_ctx, - grpc_channel_element *elem, - grpc_channel_element_args *args) { +static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, + grpc_channel_element *elem, + grpc_channel_element_args *args) { channel_data *chand = elem->channel_data; GPR_ASSERT(chand != NULL); + return GRPC_ERROR_NONE; } static void destroy_channel_elem(grpc_exec_ctx *exec_ctx, diff --git a/src/core/ext/client_channel/client_channel.c b/src/core/ext/client_channel/client_channel.c index 7df3bca5d8b01..13d8036b8d6dd 100644 --- a/src/core/ext/client_channel/client_channel.c +++ b/src/core/ext/client_channel/client_channel.c @@ -451,9 +451,9 @@ static void cc_get_channel_info(grpc_exec_ctx *exec_ctx, } /* Constructor for channel_data */ -static void cc_init_channel_elem(grpc_exec_ctx *exec_ctx, - grpc_channel_element *elem, - grpc_channel_element_args *args) { +static grpc_error* cc_init_channel_elem(grpc_exec_ctx *exec_ctx, + grpc_channel_element *elem, + grpc_channel_element_args *args) { channel_data *chand = elem->channel_data; memset(chand, 0, sizeof(*chand)); GPR_ASSERT(args->is_last); @@ -478,8 +478,10 @@ static void cc_init_channel_elem(grpc_exec_ctx *exec_ctx, GPR_ASSERT(arg != NULL); GPR_ASSERT(arg->type == GRPC_ARG_STRING); chand->resolver = grpc_resolver_create(arg->value.string, args->channel_args); -// FIXME: return failure instead of asserting - GPR_ASSERT(chand->resolver != NULL); + if (chand->resolver == NULL) { + return GRPC_ERROR_CREATE("resolver creation failed"); + } + return GRPC_ERROR_NONE; } /* Destructor for channel_data */ diff --git a/src/core/ext/client_channel/subchannel.c b/src/core/ext/client_channel/subchannel.c index a148b2a0e1dd6..f1ed95ba9d5f9 100644 --- a/src/core/ext/client_channel/subchannel.c +++ b/src/core/ext/client_channel/subchannel.c @@ -542,14 +542,20 @@ static void publish_transport_locked(grpc_exec_ctx *exec_ctx, grpc_channel_stack_builder_set_transport(builder, c->connecting_result.transport); - if (grpc_channel_init_create_stack(exec_ctx, builder, - GRPC_CLIENT_SUBCHANNEL)) { - con = grpc_channel_stack_builder_finish(exec_ctx, builder, 0, 1, - connection_destroy, NULL); - } else { + if (!grpc_channel_init_create_stack(exec_ctx, builder, + GRPC_CLIENT_SUBCHANNEL)) { grpc_channel_stack_builder_destroy(builder); abort(); /* TODO(ctiller): what to do here (previously we just crashed) */ } + grpc_error *error = grpc_channel_stack_builder_finish( + exec_ctx, builder, 0, 1, connection_destroy, NULL, (void**)&con); + if (error != GRPC_ERROR_NONE) { + const char* msg = grpc_error_string(error); + gpr_log(GPR_ERROR, "error initializing subchannel stack: %s", msg); + grpc_error_free_string(msg); + GRPC_ERROR_UNREF(error); + abort(); /* TODO(ctiller): what to do here? */ + } stk = CHANNEL_STACK_FROM_CONNECTION(con); memset(&c->connecting_result, 0, sizeof(c->connecting_result)); diff --git a/src/core/ext/load_reporting/load_reporting_filter.c b/src/core/ext/load_reporting/load_reporting_filter.c index b810e20bb98d5..a0feb086c7d6f 100644 --- a/src/core/ext/load_reporting/load_reporting_filter.c +++ b/src/core/ext/load_reporting/load_reporting_filter.c @@ -152,9 +152,9 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, } /* Constructor for channel_data */ -static void init_channel_elem(grpc_exec_ctx *exec_ctx, - grpc_channel_element *elem, - grpc_channel_element_args *args) { +static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, + grpc_channel_element *elem, + grpc_channel_element_args *args) { GPR_ASSERT(!args->is_last); channel_data *chand = elem->channel_data; @@ -171,6 +171,8 @@ static void init_channel_elem(grpc_exec_ctx *exec_ctx, NULL, NULL}; */ + + return GRPC_ERROR_NONE; } /* Destructor for channel data */ diff --git a/src/core/lib/channel/channel_stack.c b/src/core/lib/channel/channel_stack.c index 999ad5f50743f..ab78dfbf3e301 100644 --- a/src/core/lib/channel/channel_stack.c +++ b/src/core/lib/channel/channel_stack.c @@ -102,13 +102,11 @@ grpc_call_element *grpc_call_stack_element(grpc_call_stack *call_stack, return CALL_ELEMS_FROM_STACK(call_stack) + index; } -void grpc_channel_stack_init(grpc_exec_ctx *exec_ctx, int initial_refs, - grpc_iomgr_cb_func destroy, void *destroy_arg, - const grpc_channel_filter **filters, - size_t filter_count, - const grpc_channel_args *channel_args, - grpc_transport *optional_transport, - const char *name, grpc_channel_stack *stack) { +grpc_error* grpc_channel_stack_init( + grpc_exec_ctx *exec_ctx, int initial_refs, grpc_iomgr_cb_func destroy, + void *destroy_arg, const grpc_channel_filter **filters, size_t filter_count, + const grpc_channel_args *channel_args, grpc_transport *optional_transport, + const char *name, grpc_channel_stack *stack) { size_t call_size = ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(grpc_call_stack)) + ROUND_UP_TO_ALIGNMENT_SIZE(filter_count * sizeof(grpc_call_element)); @@ -126,6 +124,7 @@ void grpc_channel_stack_init(grpc_exec_ctx *exec_ctx, int initial_refs, ROUND_UP_TO_ALIGNMENT_SIZE(filter_count * sizeof(grpc_channel_element)); /* init per-filter data */ + grpc_error *first_error = GRPC_ERROR_NONE; for (i = 0; i < filter_count; i++) { args.channel_stack = stack; args.channel_args = channel_args; @@ -134,7 +133,15 @@ void grpc_channel_stack_init(grpc_exec_ctx *exec_ctx, int initial_refs, args.is_last = i == (filter_count - 1); elems[i].filter = filters[i]; elems[i].channel_data = user_data; - elems[i].filter->init_channel_elem(exec_ctx, &elems[i], &args); + grpc_error *error = + elems[i].filter->init_channel_elem(exec_ctx, &elems[i], &args); + if (error != GRPC_ERROR_NONE) { + if (first_error == GRPC_ERROR_NONE) { + first_error = error; + } else { + GRPC_ERROR_UNREF(error); + } + } user_data += ROUND_UP_TO_ALIGNMENT_SIZE(filters[i]->sizeof_channel_data); call_size += ROUND_UP_TO_ALIGNMENT_SIZE(filters[i]->sizeof_call_data); } @@ -144,6 +151,7 @@ void grpc_channel_stack_init(grpc_exec_ctx *exec_ctx, int initial_refs, grpc_channel_stack_size(filters, filter_count)); stack->call_stack_size = call_size; + return first_error; } void grpc_channel_stack_destroy(grpc_exec_ctx *exec_ctx, diff --git a/src/core/lib/channel/channel_stack.h b/src/core/lib/channel/channel_stack.h index 004643d45f6cb..9491b9ab1ba3b 100644 --- a/src/core/lib/channel/channel_stack.h +++ b/src/core/lib/channel/channel_stack.h @@ -146,8 +146,9 @@ typedef struct { is_first, is_last designate this elements position in the stack, and are useful for asserting correct configuration by upper layer code. The filter does not need to do any chaining */ - void (*init_channel_elem)(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, - grpc_channel_element_args *args); + grpc_error *(*init_channel_elem)(grpc_exec_ctx *exec_ctx, + grpc_channel_element *elem, + grpc_channel_element_args *args); /* Destroy per channel data. The filter does not need to do any chaining */ void (*destroy_channel_elem)(grpc_exec_ctx *exec_ctx, @@ -214,12 +215,12 @@ grpc_call_element *grpc_call_stack_element(grpc_call_stack *stack, size_t i); size_t grpc_channel_stack_size(const grpc_channel_filter **filters, size_t filter_count); /* Initialize a channel stack given some filters */ -void grpc_channel_stack_init(grpc_exec_ctx *exec_ctx, int initial_refs, - grpc_iomgr_cb_func destroy, void *destroy_arg, - const grpc_channel_filter **filters, - size_t filter_count, const grpc_channel_args *args, - grpc_transport *optional_transport, - const char *name, grpc_channel_stack *stack); +grpc_error* grpc_channel_stack_init( + grpc_exec_ctx *exec_ctx, int initial_refs, grpc_iomgr_cb_func destroy, + void *destroy_arg, const grpc_channel_filter **filters, + size_t filter_count, const grpc_channel_args *args, + grpc_transport *optional_transport, const char *name, + grpc_channel_stack *stack); /* Destroy a channel stack */ void grpc_channel_stack_destroy(grpc_exec_ctx *exec_ctx, grpc_channel_stack *stack); diff --git a/src/core/lib/channel/channel_stack_builder.c b/src/core/lib/channel/channel_stack_builder.c index eda4968f486cc..747db0749e6a5 100644 --- a/src/core/lib/channel/channel_stack_builder.c +++ b/src/core/lib/channel/channel_stack_builder.c @@ -227,11 +227,10 @@ void grpc_channel_stack_builder_destroy(grpc_channel_stack_builder *builder) { gpr_free(builder); } -void *grpc_channel_stack_builder_finish(grpc_exec_ctx *exec_ctx, - grpc_channel_stack_builder *builder, - size_t prefix_bytes, int initial_refs, - grpc_iomgr_cb_func destroy, - void *destroy_arg) { +grpc_error *grpc_channel_stack_builder_finish( + grpc_exec_ctx *exec_ctx, grpc_channel_stack_builder *builder, + size_t prefix_bytes, int initial_refs, grpc_iomgr_cb_func destroy, + void *destroy_arg, void **result) { // count the number of filters size_t num_filters = 0; for (filter_node *p = builder->begin.next; p != &builder->end; p = p->next) { @@ -250,15 +249,15 @@ void *grpc_channel_stack_builder_finish(grpc_exec_ctx *exec_ctx, size_t channel_stack_size = grpc_channel_stack_size(filters, num_filters); // allocate memory, with prefix_bytes followed by channel_stack_size - char *result = gpr_malloc(prefix_bytes + channel_stack_size); + *result = gpr_malloc(prefix_bytes + channel_stack_size); // fetch a pointer to the channel stack grpc_channel_stack *channel_stack = - (grpc_channel_stack *)(result + prefix_bytes); + (grpc_channel_stack *)(*result + prefix_bytes); // and initialize it - grpc_channel_stack_init(exec_ctx, initial_refs, destroy, - destroy_arg == NULL ? result : destroy_arg, filters, - num_filters, builder->args, builder->transport, - builder->name, channel_stack); + grpc_error* error = grpc_channel_stack_init( + exec_ctx, initial_refs, destroy, + destroy_arg == NULL ? *result : destroy_arg, filters, num_filters, + builder->args, builder->transport, builder->name, channel_stack); // run post-initialization functions i = 0; @@ -273,5 +272,5 @@ void *grpc_channel_stack_builder_finish(grpc_exec_ctx *exec_ctx, grpc_channel_stack_builder_destroy(builder); gpr_free((grpc_channel_filter **)filters); - return result; + return error; } diff --git a/src/core/lib/channel/channel_stack_builder.h b/src/core/lib/channel/channel_stack_builder.h index 4a00f7bfdbdb8..65bfebcabc26f 100644 --- a/src/core/lib/channel/channel_stack_builder.h +++ b/src/core/lib/channel/channel_stack_builder.h @@ -146,16 +146,15 @@ bool grpc_channel_stack_builder_append_filter( void grpc_channel_stack_builder_iterator_destroy( grpc_channel_stack_builder_iterator *iterator); -/// Destroy the builder, return the freshly minted channel stack +/// Destroy the builder, return the freshly minted channel stack in \a result. /// Allocates \a prefix_bytes bytes before the channel stack /// Returns the base pointer of the allocated block /// \a initial_refs, \a destroy, \a destroy_arg are as per /// grpc_channel_stack_init -void *grpc_channel_stack_builder_finish(grpc_exec_ctx *exec_ctx, - grpc_channel_stack_builder *builder, - size_t prefix_bytes, int initial_refs, - grpc_iomgr_cb_func destroy, - void *destroy_arg); +grpc_error *grpc_channel_stack_builder_finish( + grpc_exec_ctx *exec_ctx, grpc_channel_stack_builder *builder, + size_t prefix_bytes, int initial_refs, grpc_iomgr_cb_func destroy, + void *destroy_arg, void **result); /// Destroy the builder without creating a channel stack void grpc_channel_stack_builder_destroy(grpc_channel_stack_builder *builder); diff --git a/src/core/lib/channel/compress_filter.c b/src/core/lib/channel/compress_filter.c index 2874d63fc780a..2afe28941a01f 100644 --- a/src/core/lib/channel/compress_filter.c +++ b/src/core/lib/channel/compress_filter.c @@ -285,9 +285,9 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, } /* Constructor for channel_data */ -static void init_channel_elem(grpc_exec_ctx *exec_ctx, - grpc_channel_element *elem, - grpc_channel_element_args *args) { +static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, + grpc_channel_element *elem, + grpc_channel_element_args *args) { channel_data *channeld = elem->channel_data; channeld->enabled_algorithms_bitset = @@ -315,6 +315,7 @@ static void init_channel_elem(grpc_exec_ctx *exec_ctx, } GPR_ASSERT(!args->is_last); + return GRPC_ERROR_NONE; } /* Destructor for channel data */ diff --git a/src/core/lib/channel/connected_channel.c b/src/core/lib/channel/connected_channel.c index 038e819f7207c..92739f70c7eab 100644 --- a/src/core/lib/channel/connected_channel.c +++ b/src/core/lib/channel/connected_channel.c @@ -114,12 +114,13 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, } /* Constructor for channel_data */ -static void init_channel_elem(grpc_exec_ctx *exec_ctx, - grpc_channel_element *elem, - grpc_channel_element_args *args) { +static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, + grpc_channel_element *elem, + grpc_channel_element_args *args) { channel_data *cd = (channel_data *)elem->channel_data; GPR_ASSERT(args->is_last); cd->transport = NULL; + return GRPC_ERROR_NONE; } /* Destructor for channel_data */ diff --git a/src/core/lib/channel/deadline_filter.c b/src/core/lib/channel/deadline_filter.c index 0e703d8d27969..470ccfea578a8 100644 --- a/src/core/lib/channel/deadline_filter.c +++ b/src/core/lib/channel/deadline_filter.c @@ -207,10 +207,11 @@ void grpc_deadline_state_client_start_transport_stream_op( // // Constructor for channel_data. Used for both client and server filters. -static void init_channel_elem(grpc_exec_ctx* exec_ctx, - grpc_channel_element* elem, - grpc_channel_element_args* args) { +static grpc_error* init_channel_elem(grpc_exec_ctx* exec_ctx, + grpc_channel_element* elem, + grpc_channel_element_args* args) { GPR_ASSERT(!args->is_last); + return GRPC_ERROR_NONE; } // Destructor for channel_data. Used for both client and server filters. diff --git a/src/core/lib/channel/http_client_filter.c b/src/core/lib/channel/http_client_filter.c index f57d7c24536a2..8be9e0a2cba89 100644 --- a/src/core/lib/channel/http_client_filter.c +++ b/src/core/lib/channel/http_client_filter.c @@ -415,9 +415,9 @@ static grpc_mdstr *user_agent_from_args(const grpc_channel_args *args, } /* Constructor for channel_data */ -static void init_channel_elem(grpc_exec_ctx *exec_ctx, - grpc_channel_element *elem, - grpc_channel_element_args *args) { +static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, + grpc_channel_element *elem, + grpc_channel_element_args *args) { channel_data *chand = elem->channel_data; GPR_ASSERT(!args->is_last); GPR_ASSERT(args->optional_transport != NULL); @@ -428,6 +428,7 @@ static void init_channel_elem(grpc_exec_ctx *exec_ctx, GRPC_MDSTR_USER_AGENT, user_agent_from_args(args->channel_args, args->optional_transport->vtable->name)); + return GRPC_ERROR_NONE; } /* Destructor for channel data */ diff --git a/src/core/lib/channel/http_server_filter.c b/src/core/lib/channel/http_server_filter.c index 6a33689fecf91..035124ade9be5 100644 --- a/src/core/lib/channel/http_server_filter.c +++ b/src/core/lib/channel/http_server_filter.c @@ -326,10 +326,11 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, } /* Constructor for channel_data */ -static void init_channel_elem(grpc_exec_ctx *exec_ctx, - grpc_channel_element *elem, - grpc_channel_element_args *args) { +static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, + grpc_channel_element *elem, + grpc_channel_element_args *args) { GPR_ASSERT(!args->is_last); + return GRPC_ERROR_NONE; } /* Destructor for channel data */ diff --git a/src/core/lib/channel/message_size_filter.c b/src/core/lib/channel/message_size_filter.c index 1331fe1c6592c..c6bfb80d78f99 100644 --- a/src/core/lib/channel/message_size_filter.c +++ b/src/core/lib/channel/message_size_filter.c @@ -195,9 +195,9 @@ static void destroy_call_elem(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, void* ignored) {} // Constructor for channel_data. -static void init_channel_elem(grpc_exec_ctx* exec_ctx, - grpc_channel_element* elem, - grpc_channel_element_args* args) { +static grpc_error* init_channel_elem(grpc_exec_ctx* exec_ctx, + grpc_channel_element* elem, + grpc_channel_element_args* args) { GPR_ASSERT(!args->is_last); channel_data* chand = elem->channel_data; memset(chand, 0, sizeof(*chand)); @@ -228,6 +228,7 @@ static void init_channel_elem(grpc_exec_ctx* exec_ctx, (grpc_method_config_table*)channel_arg->value.pointer.p, method_config_convert_value, &message_size_limits_vtable); } + return GRPC_ERROR_NONE; } // Destructor for channel_data. diff --git a/src/core/lib/security/transport/client_auth_filter.c b/src/core/lib/security/transport/client_auth_filter.c index 053bf5972c2b4..eeb11feeb23c1 100644 --- a/src/core/lib/security/transport/client_auth_filter.c +++ b/src/core/lib/security/transport/client_auth_filter.c @@ -303,9 +303,9 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, } /* Constructor for channel_data */ -static void init_channel_elem(grpc_exec_ctx *exec_ctx, - grpc_channel_element *elem, - grpc_channel_element_args *args) { +static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, + grpc_channel_element *elem, + grpc_channel_element_args *args) { grpc_security_connector *sc = grpc_find_security_connector_in_args(args->channel_args); grpc_auth_context *auth_context = @@ -327,6 +327,7 @@ static void init_channel_elem(grpc_exec_ctx *exec_ctx, sc, "client_auth_filter"); chand->auth_context = GRPC_AUTH_CONTEXT_REF(auth_context, "client_auth_filter"); + return GRPC_ERROR_NONE; } /* Destructor for channel data */ diff --git a/src/core/lib/security/transport/server_auth_filter.c b/src/core/lib/security/transport/server_auth_filter.c index eaa1d0720b25b..3abbeb35ef0f7 100644 --- a/src/core/lib/security/transport/server_auth_filter.c +++ b/src/core/lib/security/transport/server_auth_filter.c @@ -238,9 +238,9 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, void *ignored) {} /* Constructor for channel_data */ -static void init_channel_elem(grpc_exec_ctx *exec_ctx, - grpc_channel_element *elem, - grpc_channel_element_args *args) { +static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, + grpc_channel_element *elem, + grpc_channel_element_args *args) { grpc_auth_context *auth_context = grpc_find_auth_context_in_args(args->channel_args); grpc_server_credentials *creds = @@ -256,6 +256,7 @@ static void init_channel_elem(grpc_exec_ctx *exec_ctx, chand->auth_context = GRPC_AUTH_CONTEXT_REF(auth_context, "server_auth_filter"); chand->creds = grpc_server_credentials_ref(creds); + return GRPC_ERROR_NONE; } /* Destructor for channel data */ diff --git a/src/core/lib/surface/channel.c b/src/core/lib/surface/channel.c index 1389df688625f..1b5cf5ffec66c 100644 --- a/src/core/lib/surface/channel.c +++ b/src/core/lib/surface/channel.c @@ -97,11 +97,16 @@ grpc_channel *grpc_channel_create(grpc_exec_ctx *exec_ctx, const char *target, if (!grpc_channel_init_create_stack(exec_ctx, builder, channel_stack_type)) { grpc_channel_stack_builder_destroy(builder); return NULL; - } else { - args = grpc_channel_args_copy( - grpc_channel_stack_builder_get_channel_arguments(builder)); - channel = grpc_channel_stack_builder_finish( - exec_ctx, builder, sizeof(grpc_channel), 1, destroy_channel, NULL); + } + args = grpc_channel_args_copy( + grpc_channel_stack_builder_get_channel_arguments(builder)); + grpc_error* error = grpc_channel_stack_builder_finish( + exec_ctx, builder, sizeof(grpc_channel), 1, destroy_channel, NULL, + (void**)&channel); + if (error != GRPC_ERROR_NONE) { + grpc_channel_stack_destroy(exec_ctx, (grpc_channel_stack *)channel); + gpr_free(channel); + return NULL; } memset(channel, 0, sizeof(*channel)); diff --git a/src/core/lib/surface/lame_client.c b/src/core/lib/surface/lame_client.c index d0df8e7e17436..995a88de9ee42 100644 --- a/src/core/lib/surface/lame_client.c +++ b/src/core/lib/surface/lame_client.c @@ -123,11 +123,12 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, gpr_free(and_free_memory); } -static void init_channel_elem(grpc_exec_ctx *exec_ctx, - grpc_channel_element *elem, - grpc_channel_element_args *args) { +static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, + grpc_channel_element *elem, + grpc_channel_element_args *args) { GPR_ASSERT(args->is_first); GPR_ASSERT(args->is_last); + return GRPC_ERROR_NONE; } static void destroy_channel_elem(grpc_exec_ctx *exec_ctx, diff --git a/src/core/lib/surface/server.c b/src/core/lib/surface/server.c index 89dd825460320..eeeb6cd43250b 100644 --- a/src/core/lib/surface/server.c +++ b/src/core/lib/surface/server.c @@ -913,9 +913,9 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, server_unref(exec_ctx, chand->server); } -static void init_channel_elem(grpc_exec_ctx *exec_ctx, - grpc_channel_element *elem, - grpc_channel_element_args *args) { +static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, + grpc_channel_element *elem, + grpc_channel_element_args *args) { channel_data *chand = elem->channel_data; GPR_ASSERT(args->is_first); GPR_ASSERT(!args->is_last); @@ -926,6 +926,7 @@ static void init_channel_elem(grpc_exec_ctx *exec_ctx, chand->connectivity_state = GRPC_CHANNEL_IDLE; grpc_closure_init(&chand->channel_connectivity_changed, channel_connectivity_changed, chand); + return GRPC_ERROR_NONE; } static void destroy_channel_elem(grpc_exec_ctx *exec_ctx, diff --git a/test/core/end2end/tests/filter_call_init_fails.c b/test/core/end2end/tests/filter_call_init_fails.c index 41ae575fff455..13a32bf64d3d3 100644 --- a/test/core/end2end/tests/filter_call_init_fails.c +++ b/test/core/end2end/tests/filter_call_init_fails.c @@ -216,9 +216,11 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, const grpc_call_final_info *final_info, void *and_free_memory) {} -static void init_channel_elem(grpc_exec_ctx *exec_ctx, - grpc_channel_element *elem, - grpc_channel_element_args *args) {} +static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, + grpc_channel_element *elem, + grpc_channel_element_args *args) { + return GRPC_ERROR_NONE; +} static void destroy_channel_elem(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem) {} diff --git a/test/core/end2end/tests/filter_causes_close.c b/test/core/end2end/tests/filter_causes_close.c index bf9fd9073d17e..4c1ff8459afb4 100644 --- a/test/core/end2end/tests/filter_causes_close.c +++ b/test/core/end2end/tests/filter_causes_close.c @@ -243,9 +243,11 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, const grpc_call_final_info *final_info, void *and_free_memory) {} -static void init_channel_elem(grpc_exec_ctx *exec_ctx, - grpc_channel_element *elem, - grpc_channel_element_args *args) {} +static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, + grpc_channel_element *elem, + grpc_channel_element_args *args) { + return GRPC_ERROR_NONE; +} static void destroy_channel_elem(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem) {} diff --git a/test/core/end2end/tests/filter_latency.c b/test/core/end2end/tests/filter_latency.c index 37ce3b122210f..3e9c0352a5f71 100644 --- a/test/core/end2end/tests/filter_latency.c +++ b/test/core/end2end/tests/filter_latency.c @@ -275,9 +275,11 @@ static void server_destroy_call_elem(grpc_exec_ctx *exec_ctx, gpr_mu_unlock(&g_mu); } -static void init_channel_elem(grpc_exec_ctx *exec_ctx, - grpc_channel_element *elem, - grpc_channel_element_args *args) {} +static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, + grpc_channel_element *elem, + grpc_channel_element_args *args) { + return GRPC_ERROR_NONE; +} static void destroy_channel_elem(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem) {} From c1087883579714691dc5cb3a66445da497c1a08c Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 18 Nov 2016 10:54:45 -0800 Subject: [PATCH 04/13] clang-format --- src/core/ext/census/grpc_filter.c | 2 +- src/core/ext/client_channel/client_channel.c | 2 +- src/core/ext/client_channel/subchannel.c | 4 ++-- src/core/ext/load_reporting/load_reporting_filter.c | 2 +- .../transport/chttp2/client/insecure/channel_create.c | 4 +--- src/core/lib/channel/channel_stack.c | 2 +- src/core/lib/channel/channel_stack.h | 9 ++++----- src/core/lib/channel/channel_stack_builder.c | 2 +- src/core/lib/channel/compress_filter.c | 2 +- src/core/lib/channel/connected_channel.c | 2 +- src/core/lib/channel/http_client_filter.c | 2 +- src/core/lib/channel/http_server_filter.c | 2 +- src/core/lib/security/transport/client_auth_filter.c | 2 +- src/core/lib/security/transport/server_auth_filter.c | 2 +- src/core/lib/surface/channel.c | 4 ++-- src/core/lib/surface/lame_client.c | 2 +- src/core/lib/surface/server.c | 2 +- test/core/end2end/tests/filter_call_init_fails.c | 2 +- test/core/end2end/tests/filter_causes_close.c | 2 +- test/core/end2end/tests/filter_latency.c | 2 +- 20 files changed, 25 insertions(+), 28 deletions(-) diff --git a/src/core/ext/census/grpc_filter.c b/src/core/ext/census/grpc_filter.c index c73385f98b84a..3e8acc85e1397 100644 --- a/src/core/ext/census/grpc_filter.c +++ b/src/core/ext/census/grpc_filter.c @@ -167,7 +167,7 @@ static void server_destroy_call_elem(grpc_exec_ctx *exec_ctx, /* TODO(hongyu): record rpc server stats and census_tracing_end_op here */ } -static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, +static grpc_error *init_channel_elem(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, grpc_channel_element_args *args) { channel_data *chand = elem->channel_data; diff --git a/src/core/ext/client_channel/client_channel.c b/src/core/ext/client_channel/client_channel.c index 13d8036b8d6dd..64f507f42492f 100644 --- a/src/core/ext/client_channel/client_channel.c +++ b/src/core/ext/client_channel/client_channel.c @@ -451,7 +451,7 @@ static void cc_get_channel_info(grpc_exec_ctx *exec_ctx, } /* Constructor for channel_data */ -static grpc_error* cc_init_channel_elem(grpc_exec_ctx *exec_ctx, +static grpc_error *cc_init_channel_elem(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, grpc_channel_element_args *args) { channel_data *chand = elem->channel_data; diff --git a/src/core/ext/client_channel/subchannel.c b/src/core/ext/client_channel/subchannel.c index f1ed95ba9d5f9..b25957518ad9f 100644 --- a/src/core/ext/client_channel/subchannel.c +++ b/src/core/ext/client_channel/subchannel.c @@ -548,9 +548,9 @@ static void publish_transport_locked(grpc_exec_ctx *exec_ctx, abort(); /* TODO(ctiller): what to do here (previously we just crashed) */ } grpc_error *error = grpc_channel_stack_builder_finish( - exec_ctx, builder, 0, 1, connection_destroy, NULL, (void**)&con); + exec_ctx, builder, 0, 1, connection_destroy, NULL, (void **)&con); if (error != GRPC_ERROR_NONE) { - const char* msg = grpc_error_string(error); + const char *msg = grpc_error_string(error); gpr_log(GPR_ERROR, "error initializing subchannel stack: %s", msg); grpc_error_free_string(msg); GRPC_ERROR_UNREF(error); diff --git a/src/core/ext/load_reporting/load_reporting_filter.c b/src/core/ext/load_reporting/load_reporting_filter.c index a0feb086c7d6f..18bb826948d94 100644 --- a/src/core/ext/load_reporting/load_reporting_filter.c +++ b/src/core/ext/load_reporting/load_reporting_filter.c @@ -152,7 +152,7 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, } /* Constructor for channel_data */ -static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, +static grpc_error *init_channel_elem(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, grpc_channel_element_args *args) { GPR_ASSERT(!args->is_last); diff --git a/src/core/ext/transport/chttp2/client/insecure/channel_create.c b/src/core/ext/transport/chttp2/client/insecure/channel_create.c index 0d2395266b14b..9e0478feab477 100644 --- a/src/core/ext/transport/chttp2/client/insecure/channel_create.c +++ b/src/core/ext/transport/chttp2/client/insecure/channel_create.c @@ -205,9 +205,7 @@ static const grpc_client_channel_factory_vtable client_channel_factory_vtable = static grpc_client_channel_factory client_channel_factory = { &client_channel_factory_vtable}; -static void *cc_factory_arg_copy(void *cc_factory) { - return cc_factory; -} +static void *cc_factory_arg_copy(void *cc_factory) { return cc_factory; } static void cc_factory_arg_destroy(void *cc_factory) {} diff --git a/src/core/lib/channel/channel_stack.c b/src/core/lib/channel/channel_stack.c index ab78dfbf3e301..1d0b7d4f3153d 100644 --- a/src/core/lib/channel/channel_stack.c +++ b/src/core/lib/channel/channel_stack.c @@ -102,7 +102,7 @@ grpc_call_element *grpc_call_stack_element(grpc_call_stack *call_stack, return CALL_ELEMS_FROM_STACK(call_stack) + index; } -grpc_error* grpc_channel_stack_init( +grpc_error *grpc_channel_stack_init( grpc_exec_ctx *exec_ctx, int initial_refs, grpc_iomgr_cb_func destroy, void *destroy_arg, const grpc_channel_filter **filters, size_t filter_count, const grpc_channel_args *channel_args, grpc_transport *optional_transport, diff --git a/src/core/lib/channel/channel_stack.h b/src/core/lib/channel/channel_stack.h index 9491b9ab1ba3b..5d064c5695bbe 100644 --- a/src/core/lib/channel/channel_stack.h +++ b/src/core/lib/channel/channel_stack.h @@ -215,12 +215,11 @@ grpc_call_element *grpc_call_stack_element(grpc_call_stack *stack, size_t i); size_t grpc_channel_stack_size(const grpc_channel_filter **filters, size_t filter_count); /* Initialize a channel stack given some filters */ -grpc_error* grpc_channel_stack_init( +grpc_error *grpc_channel_stack_init( grpc_exec_ctx *exec_ctx, int initial_refs, grpc_iomgr_cb_func destroy, - void *destroy_arg, const grpc_channel_filter **filters, - size_t filter_count, const grpc_channel_args *args, - grpc_transport *optional_transport, const char *name, - grpc_channel_stack *stack); + void *destroy_arg, const grpc_channel_filter **filters, size_t filter_count, + const grpc_channel_args *args, grpc_transport *optional_transport, + const char *name, grpc_channel_stack *stack); /* Destroy a channel stack */ void grpc_channel_stack_destroy(grpc_exec_ctx *exec_ctx, grpc_channel_stack *stack); diff --git a/src/core/lib/channel/channel_stack_builder.c b/src/core/lib/channel/channel_stack_builder.c index 747db0749e6a5..047d85f44d20b 100644 --- a/src/core/lib/channel/channel_stack_builder.c +++ b/src/core/lib/channel/channel_stack_builder.c @@ -254,7 +254,7 @@ grpc_error *grpc_channel_stack_builder_finish( grpc_channel_stack *channel_stack = (grpc_channel_stack *)(*result + prefix_bytes); // and initialize it - grpc_error* error = grpc_channel_stack_init( + grpc_error *error = grpc_channel_stack_init( exec_ctx, initial_refs, destroy, destroy_arg == NULL ? *result : destroy_arg, filters, num_filters, builder->args, builder->transport, builder->name, channel_stack); diff --git a/src/core/lib/channel/compress_filter.c b/src/core/lib/channel/compress_filter.c index 2afe28941a01f..0e336dc330609 100644 --- a/src/core/lib/channel/compress_filter.c +++ b/src/core/lib/channel/compress_filter.c @@ -285,7 +285,7 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, } /* Constructor for channel_data */ -static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, +static grpc_error *init_channel_elem(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, grpc_channel_element_args *args) { channel_data *channeld = elem->channel_data; diff --git a/src/core/lib/channel/connected_channel.c b/src/core/lib/channel/connected_channel.c index 92739f70c7eab..c2a36b55585f5 100644 --- a/src/core/lib/channel/connected_channel.c +++ b/src/core/lib/channel/connected_channel.c @@ -114,7 +114,7 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, } /* Constructor for channel_data */ -static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, +static grpc_error *init_channel_elem(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, grpc_channel_element_args *args) { channel_data *cd = (channel_data *)elem->channel_data; diff --git a/src/core/lib/channel/http_client_filter.c b/src/core/lib/channel/http_client_filter.c index 8be9e0a2cba89..35528e1b8c612 100644 --- a/src/core/lib/channel/http_client_filter.c +++ b/src/core/lib/channel/http_client_filter.c @@ -415,7 +415,7 @@ static grpc_mdstr *user_agent_from_args(const grpc_channel_args *args, } /* Constructor for channel_data */ -static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, +static grpc_error *init_channel_elem(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, grpc_channel_element_args *args) { channel_data *chand = elem->channel_data; diff --git a/src/core/lib/channel/http_server_filter.c b/src/core/lib/channel/http_server_filter.c index 035124ade9be5..4b976ed8d5565 100644 --- a/src/core/lib/channel/http_server_filter.c +++ b/src/core/lib/channel/http_server_filter.c @@ -326,7 +326,7 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, } /* Constructor for channel_data */ -static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, +static grpc_error *init_channel_elem(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, grpc_channel_element_args *args) { GPR_ASSERT(!args->is_last); diff --git a/src/core/lib/security/transport/client_auth_filter.c b/src/core/lib/security/transport/client_auth_filter.c index eeb11feeb23c1..da897296e4adf 100644 --- a/src/core/lib/security/transport/client_auth_filter.c +++ b/src/core/lib/security/transport/client_auth_filter.c @@ -303,7 +303,7 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, } /* Constructor for channel_data */ -static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, +static grpc_error *init_channel_elem(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, grpc_channel_element_args *args) { grpc_security_connector *sc = diff --git a/src/core/lib/security/transport/server_auth_filter.c b/src/core/lib/security/transport/server_auth_filter.c index 3abbeb35ef0f7..e6a242e68f197 100644 --- a/src/core/lib/security/transport/server_auth_filter.c +++ b/src/core/lib/security/transport/server_auth_filter.c @@ -238,7 +238,7 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, void *ignored) {} /* Constructor for channel_data */ -static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, +static grpc_error *init_channel_elem(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, grpc_channel_element_args *args) { grpc_auth_context *auth_context = diff --git a/src/core/lib/surface/channel.c b/src/core/lib/surface/channel.c index 1b5cf5ffec66c..22bb55c7b4699 100644 --- a/src/core/lib/surface/channel.c +++ b/src/core/lib/surface/channel.c @@ -100,9 +100,9 @@ grpc_channel *grpc_channel_create(grpc_exec_ctx *exec_ctx, const char *target, } args = grpc_channel_args_copy( grpc_channel_stack_builder_get_channel_arguments(builder)); - grpc_error* error = grpc_channel_stack_builder_finish( + grpc_error *error = grpc_channel_stack_builder_finish( exec_ctx, builder, sizeof(grpc_channel), 1, destroy_channel, NULL, - (void**)&channel); + (void **)&channel); if (error != GRPC_ERROR_NONE) { grpc_channel_stack_destroy(exec_ctx, (grpc_channel_stack *)channel); gpr_free(channel); diff --git a/src/core/lib/surface/lame_client.c b/src/core/lib/surface/lame_client.c index 995a88de9ee42..57da94ac1e94a 100644 --- a/src/core/lib/surface/lame_client.c +++ b/src/core/lib/surface/lame_client.c @@ -123,7 +123,7 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, gpr_free(and_free_memory); } -static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, +static grpc_error *init_channel_elem(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, grpc_channel_element_args *args) { GPR_ASSERT(args->is_first); diff --git a/src/core/lib/surface/server.c b/src/core/lib/surface/server.c index eeeb6cd43250b..78bd13bbbd630 100644 --- a/src/core/lib/surface/server.c +++ b/src/core/lib/surface/server.c @@ -913,7 +913,7 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, server_unref(exec_ctx, chand->server); } -static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, +static grpc_error *init_channel_elem(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, grpc_channel_element_args *args) { channel_data *chand = elem->channel_data; diff --git a/test/core/end2end/tests/filter_call_init_fails.c b/test/core/end2end/tests/filter_call_init_fails.c index 13a32bf64d3d3..6d9351ed8cbb8 100644 --- a/test/core/end2end/tests/filter_call_init_fails.c +++ b/test/core/end2end/tests/filter_call_init_fails.c @@ -216,7 +216,7 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, const grpc_call_final_info *final_info, void *and_free_memory) {} -static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, +static grpc_error *init_channel_elem(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, grpc_channel_element_args *args) { return GRPC_ERROR_NONE; diff --git a/test/core/end2end/tests/filter_causes_close.c b/test/core/end2end/tests/filter_causes_close.c index 4c1ff8459afb4..21905b98faf0c 100644 --- a/test/core/end2end/tests/filter_causes_close.c +++ b/test/core/end2end/tests/filter_causes_close.c @@ -243,7 +243,7 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, const grpc_call_final_info *final_info, void *and_free_memory) {} -static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, +static grpc_error *init_channel_elem(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, grpc_channel_element_args *args) { return GRPC_ERROR_NONE; diff --git a/test/core/end2end/tests/filter_latency.c b/test/core/end2end/tests/filter_latency.c index 3e9c0352a5f71..9263dcc203325 100644 --- a/test/core/end2end/tests/filter_latency.c +++ b/test/core/end2end/tests/filter_latency.c @@ -275,7 +275,7 @@ static void server_destroy_call_elem(grpc_exec_ctx *exec_ctx, gpr_mu_unlock(&g_mu); } -static grpc_error* init_channel_elem(grpc_exec_ctx *exec_ctx, +static grpc_error *init_channel_elem(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, grpc_channel_element_args *args) { return GRPC_ERROR_NONE; From e85477646c5b6ec159715a7e8d9ebaaf6d451777 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 28 Nov 2016 17:42:53 +0000 Subject: [PATCH 05/13] Fix build problems. --- src/core/lib/channel/channel_stack_builder.c | 2 +- src/cpp/common/channel_filter.h | 13 +++++++++---- test/core/channel/channel_stack_test.c | 7 ++++--- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/core/lib/channel/channel_stack_builder.c b/src/core/lib/channel/channel_stack_builder.c index 047d85f44d20b..dd11e5bf6bafc 100644 --- a/src/core/lib/channel/channel_stack_builder.c +++ b/src/core/lib/channel/channel_stack_builder.c @@ -252,7 +252,7 @@ grpc_error *grpc_channel_stack_builder_finish( *result = gpr_malloc(prefix_bytes + channel_stack_size); // fetch a pointer to the channel stack grpc_channel_stack *channel_stack = - (grpc_channel_stack *)(*result + prefix_bytes); + (grpc_channel_stack *)((char *)(*result) + prefix_bytes); // and initialize it grpc_error *error = grpc_channel_stack_init( exec_ctx, initial_refs, destroy, diff --git a/src/cpp/common/channel_filter.h b/src/cpp/common/channel_filter.h index e420efc71c9b0..6bda74b9be782 100644 --- a/src/cpp/common/channel_filter.h +++ b/src/cpp/common/channel_filter.h @@ -220,6 +220,9 @@ class ChannelData { if (peer_) gpr_free((void *)peer_); } + /// Initializes the call data. + virtual grpc_error *Init() { return GRPC_ERROR_NONE; } + /// Caller does NOT take ownership of result. const char *peer() const { return peer_; } @@ -276,15 +279,17 @@ class ChannelFilter final { public: static const size_t channel_data_size = sizeof(ChannelDataType); - static void InitChannelElement(grpc_exec_ctx *exec_ctx, - grpc_channel_element *elem, - grpc_channel_element_args *args) { + static grpc_error *InitChannelElement(grpc_exec_ctx *exec_ctx, + grpc_channel_element *elem, + grpc_channel_element_args *args) { const char *peer = args->optional_transport ? grpc_transport_get_peer(exec_ctx, args->optional_transport) : nullptr; // Construct the object in the already-allocated memory. - new (elem->channel_data) ChannelDataType(*args->channel_args, peer); + ChannelDataType* channel_data = + new (elem->channel_data) ChannelDataType(*args->channel_args, peer); + return channel_data->Init(); } static void DestroyChannelElement(grpc_exec_ctx *exec_ctx, diff --git a/test/core/channel/channel_stack_test.c b/test/core/channel/channel_stack_test.c index 0840820cca96e..b43d05eec3104 100644 --- a/test/core/channel/channel_stack_test.c +++ b/test/core/channel/channel_stack_test.c @@ -41,9 +41,9 @@ #include "test/core/util/test_config.h" -static void channel_init_func(grpc_exec_ctx *exec_ctx, - grpc_channel_element *elem, - grpc_channel_element_args *args) { +static grpc_error *channel_init_func(grpc_exec_ctx *exec_ctx, + grpc_channel_element *elem, + grpc_channel_element_args *args) { GPR_ASSERT(args->channel_args->num_args == 1); GPR_ASSERT(args->channel_args->args[0].type == GRPC_ARG_INTEGER); GPR_ASSERT(0 == strcmp(args->channel_args->args[0].key, "test_key")); @@ -51,6 +51,7 @@ static void channel_init_func(grpc_exec_ctx *exec_ctx, GPR_ASSERT(args->is_first); GPR_ASSERT(args->is_last); *(int *)(elem->channel_data) = 0; + return GRPC_ERROR_NONE; } static grpc_error *call_init_func(grpc_exec_ctx *exec_ctx, From e62605f41e00c3e4652e0a7ad19846d8995a101f Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 29 Nov 2016 16:31:36 +0000 Subject: [PATCH 06/13] Fix error handling in channel initialization. --- src/core/lib/channel/channel_stack_builder.c | 21 ++-- src/core/lib/surface/channel.c | 113 +++++++++---------- test/core/surface/channel_create_test.c | 12 ++ 3 files changed, 82 insertions(+), 64 deletions(-) diff --git a/src/core/lib/channel/channel_stack_builder.c b/src/core/lib/channel/channel_stack_builder.c index dd11e5bf6bafc..f54eac06ec613 100644 --- a/src/core/lib/channel/channel_stack_builder.c +++ b/src/core/lib/channel/channel_stack_builder.c @@ -259,14 +259,21 @@ grpc_error *grpc_channel_stack_builder_finish( destroy_arg == NULL ? *result : destroy_arg, filters, num_filters, builder->args, builder->transport, builder->name, channel_stack); - // run post-initialization functions - i = 0; - for (filter_node *p = builder->begin.next; p != &builder->end; p = p->next) { - if (p->init != NULL) { - p->init(channel_stack, grpc_channel_stack_element(channel_stack, i), - p->init_arg); + if (error != GRPC_ERROR_NONE) { + grpc_channel_stack_destroy(exec_ctx, channel_stack); + gpr_free(*result); + *result = NULL; + } else { + // run post-initialization functions + i = 0; + for (filter_node *p = builder->begin.next; p != &builder->end;\ + p = p->next) { + if (p->init != NULL) { + p->init(channel_stack, grpc_channel_stack_element(channel_stack, i), + p->init_arg); + } + i++; } - i++; } grpc_channel_stack_builder_destroy(builder); diff --git a/src/core/lib/surface/channel.c b/src/core/lib/surface/channel.c index 22bb55c7b4699..72e64a2076d02 100644 --- a/src/core/lib/surface/channel.c +++ b/src/core/lib/surface/channel.c @@ -86,92 +86,91 @@ grpc_channel *grpc_channel_create(grpc_exec_ctx *exec_ctx, const char *target, const grpc_channel_args *input_args, grpc_channel_stack_type channel_stack_type, grpc_transport *optional_transport) { - bool is_client = grpc_channel_stack_type_is_client(channel_stack_type); - grpc_channel_stack_builder *builder = grpc_channel_stack_builder_create(); grpc_channel_stack_builder_set_channel_arguments(builder, input_args); grpc_channel_stack_builder_set_target(builder, target); grpc_channel_stack_builder_set_transport(builder, optional_transport); - grpc_channel *channel; - grpc_channel_args *args; if (!grpc_channel_init_create_stack(exec_ctx, builder, channel_stack_type)) { grpc_channel_stack_builder_destroy(builder); return NULL; } - args = grpc_channel_args_copy( + grpc_channel_args *args = grpc_channel_args_copy( grpc_channel_stack_builder_get_channel_arguments(builder)); + grpc_channel *channel; grpc_error *error = grpc_channel_stack_builder_finish( exec_ctx, builder, sizeof(grpc_channel), 1, destroy_channel, NULL, (void **)&channel); if (error != GRPC_ERROR_NONE) { - grpc_channel_stack_destroy(exec_ctx, (grpc_channel_stack *)channel); - gpr_free(channel); - return NULL; + const char* msg = grpc_error_string(error); + gpr_log(GPR_ERROR, "channel stack builder failed: %s", msg); + grpc_error_free_string(msg); + GRPC_ERROR_UNREF(error); + goto done; } memset(channel, 0, sizeof(*channel)); channel->target = gpr_strdup(target); - channel->is_client = is_client; + channel->is_client = grpc_channel_stack_type_is_client(channel_stack_type); gpr_mu_init(&channel->registered_call_mu); channel->registered_calls = NULL; grpc_compression_options_init(&channel->compression_options); - if (args) { - for (size_t i = 0; i < args->num_args; i++) { - if (0 == strcmp(args->args[i].key, GRPC_ARG_DEFAULT_AUTHORITY)) { - if (args->args[i].type != GRPC_ARG_STRING) { - gpr_log(GPR_ERROR, "%s ignored: it must be a string", - GRPC_ARG_DEFAULT_AUTHORITY); - } else { - if (channel->default_authority) { - /* setting this takes precedence over anything else */ - GRPC_MDELEM_UNREF(channel->default_authority); - } - channel->default_authority = grpc_mdelem_from_strings( - ":authority", args->args[i].value.string); + + for (size_t i = 0; i < args->num_args; i++) { + if (0 == strcmp(args->args[i].key, GRPC_ARG_DEFAULT_AUTHORITY)) { + if (args->args[i].type != GRPC_ARG_STRING) { + gpr_log(GPR_ERROR, "%s ignored: it must be a string", + GRPC_ARG_DEFAULT_AUTHORITY); + } else { + if (channel->default_authority) { + /* setting this takes precedence over anything else */ + GRPC_MDELEM_UNREF(channel->default_authority); } - } else if (0 == - strcmp(args->args[i].key, GRPC_SSL_TARGET_NAME_OVERRIDE_ARG)) { - if (args->args[i].type != GRPC_ARG_STRING) { - gpr_log(GPR_ERROR, "%s ignored: it must be a string", + channel->default_authority = grpc_mdelem_from_strings( + ":authority", args->args[i].value.string); + } + } else if (0 == + strcmp(args->args[i].key, GRPC_SSL_TARGET_NAME_OVERRIDE_ARG)) { + if (args->args[i].type != GRPC_ARG_STRING) { + gpr_log(GPR_ERROR, "%s ignored: it must be a string", + GRPC_SSL_TARGET_NAME_OVERRIDE_ARG); + } else { + if (channel->default_authority) { + /* other ways of setting this (notably ssl) take precedence */ + gpr_log(GPR_ERROR, + "%s ignored: default host already set some other way", GRPC_SSL_TARGET_NAME_OVERRIDE_ARG); } else { - if (channel->default_authority) { - /* other ways of setting this (notably ssl) take precedence */ - gpr_log(GPR_ERROR, - "%s ignored: default host already set some other way", - GRPC_SSL_TARGET_NAME_OVERRIDE_ARG); - } else { - channel->default_authority = grpc_mdelem_from_strings( - ":authority", args->args[i].value.string); - } + channel->default_authority = grpc_mdelem_from_strings( + ":authority", args->args[i].value.string); } - } else if (0 == strcmp(args->args[i].key, - GRPC_COMPRESSION_CHANNEL_DEFAULT_LEVEL)) { - channel->compression_options.default_level.is_set = true; - GPR_ASSERT(args->args[i].value.integer >= 0 && - args->args[i].value.integer < GRPC_COMPRESS_LEVEL_COUNT); - channel->compression_options.default_level.level = - (grpc_compression_level)args->args[i].value.integer; - } else if (0 == strcmp(args->args[i].key, - GRPC_COMPRESSION_CHANNEL_DEFAULT_ALGORITHM)) { - channel->compression_options.default_algorithm.is_set = true; - GPR_ASSERT(args->args[i].value.integer >= 0 && - args->args[i].value.integer < - GRPC_COMPRESS_ALGORITHMS_COUNT); - channel->compression_options.default_algorithm.algorithm = - (grpc_compression_algorithm)args->args[i].value.integer; - } else if (0 == - strcmp(args->args[i].key, - GRPC_COMPRESSION_CHANNEL_ENABLED_ALGORITHMS_BITSET)) { - channel->compression_options.enabled_algorithms_bitset = - (uint32_t)args->args[i].value.integer | - 0x1; /* always support no compression */ } + } else if (0 == strcmp(args->args[i].key, + GRPC_COMPRESSION_CHANNEL_DEFAULT_LEVEL)) { + channel->compression_options.default_level.is_set = true; + GPR_ASSERT(args->args[i].value.integer >= 0 && + args->args[i].value.integer < GRPC_COMPRESS_LEVEL_COUNT); + channel->compression_options.default_level.level = + (grpc_compression_level)args->args[i].value.integer; + } else if (0 == strcmp(args->args[i].key, + GRPC_COMPRESSION_CHANNEL_DEFAULT_ALGORITHM)) { + channel->compression_options.default_algorithm.is_set = true; + GPR_ASSERT(args->args[i].value.integer >= 0 && + args->args[i].value.integer < + GRPC_COMPRESS_ALGORITHMS_COUNT); + channel->compression_options.default_algorithm.algorithm = + (grpc_compression_algorithm)args->args[i].value.integer; + } else if (0 == + strcmp(args->args[i].key, + GRPC_COMPRESSION_CHANNEL_ENABLED_ALGORITHMS_BITSET)) { + channel->compression_options.enabled_algorithms_bitset = + (uint32_t)args->args[i].value.integer | + 0x1; /* always support no compression */ } - grpc_channel_args_destroy(args); } +done: + grpc_channel_args_destroy(args); return channel; } diff --git a/test/core/surface/channel_create_test.c b/test/core/surface/channel_create_test.c index ad7970aab9d22..654e5324d980a 100644 --- a/test/core/surface/channel_create_test.c +++ b/test/core/surface/channel_create_test.c @@ -31,9 +31,14 @@ * */ +#include + #include #include + #include "src/core/ext/client_channel/resolver_registry.h" +#include "src/core/lib/channel/channel_stack.h" +#include "src/core/lib/surface/channel.h" #include "test/core/util/test_config.h" void test_unknown_scheme_target(void) { @@ -44,6 +49,13 @@ void test_unknown_scheme_target(void) { chan = grpc_insecure_channel_create("blah://blah", NULL, NULL); GPR_ASSERT(chan != NULL); + + grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; + grpc_channel_element *elem = + grpc_channel_stack_element(grpc_channel_get_channel_stack(chan), 0); + GPR_ASSERT(0 == strcmp(elem->filter->name, "lame-client")); + grpc_exec_ctx_finish(&exec_ctx); + grpc_channel_destroy(chan); } From 977f5d4e7dc8bdf21bda2e8b9a7a232f88de8e73 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 30 Nov 2016 14:03:58 -0800 Subject: [PATCH 07/13] clang-format --- src/core/lib/channel/channel_stack_builder.c | 2 +- src/core/lib/surface/channel.c | 9 ++++----- src/cpp/common/channel_filter.h | 2 +- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/core/lib/channel/channel_stack_builder.c b/src/core/lib/channel/channel_stack_builder.c index f54eac06ec613..b959517afb3fd 100644 --- a/src/core/lib/channel/channel_stack_builder.c +++ b/src/core/lib/channel/channel_stack_builder.c @@ -266,7 +266,7 @@ grpc_error *grpc_channel_stack_builder_finish( } else { // run post-initialization functions i = 0; - for (filter_node *p = builder->begin.next; p != &builder->end;\ + for (filter_node *p = builder->begin.next; p != &builder->end; p = p->next) { if (p->init != NULL) { p->init(channel_stack, grpc_channel_stack_element(channel_stack, i), diff --git a/src/core/lib/surface/channel.c b/src/core/lib/surface/channel.c index 72e64a2076d02..9405015c50184 100644 --- a/src/core/lib/surface/channel.c +++ b/src/core/lib/surface/channel.c @@ -101,7 +101,7 @@ grpc_channel *grpc_channel_create(grpc_exec_ctx *exec_ctx, const char *target, exec_ctx, builder, sizeof(grpc_channel), 1, destroy_channel, NULL, (void **)&channel); if (error != GRPC_ERROR_NONE) { - const char* msg = grpc_error_string(error); + const char *msg = grpc_error_string(error); gpr_log(GPR_ERROR, "channel stack builder failed: %s", msg); grpc_error_free_string(msg); GRPC_ERROR_UNREF(error); @@ -126,8 +126,8 @@ grpc_channel *grpc_channel_create(grpc_exec_ctx *exec_ctx, const char *target, /* setting this takes precedence over anything else */ GRPC_MDELEM_UNREF(channel->default_authority); } - channel->default_authority = grpc_mdelem_from_strings( - ":authority", args->args[i].value.string); + channel->default_authority = + grpc_mdelem_from_strings(":authority", args->args[i].value.string); } } else if (0 == strcmp(args->args[i].key, GRPC_SSL_TARGET_NAME_OVERRIDE_ARG)) { @@ -156,8 +156,7 @@ grpc_channel *grpc_channel_create(grpc_exec_ctx *exec_ctx, const char *target, GRPC_COMPRESSION_CHANNEL_DEFAULT_ALGORITHM)) { channel->compression_options.default_algorithm.is_set = true; GPR_ASSERT(args->args[i].value.integer >= 0 && - args->args[i].value.integer < - GRPC_COMPRESS_ALGORITHMS_COUNT); + args->args[i].value.integer < GRPC_COMPRESS_ALGORITHMS_COUNT); channel->compression_options.default_algorithm.algorithm = (grpc_compression_algorithm)args->args[i].value.integer; } else if (0 == diff --git a/src/cpp/common/channel_filter.h b/src/cpp/common/channel_filter.h index 6bda74b9be782..107522ea04a94 100644 --- a/src/cpp/common/channel_filter.h +++ b/src/cpp/common/channel_filter.h @@ -287,7 +287,7 @@ class ChannelFilter final { ? grpc_transport_get_peer(exec_ctx, args->optional_transport) : nullptr; // Construct the object in the already-allocated memory. - ChannelDataType* channel_data = + ChannelDataType *channel_data = new (elem->channel_data) ChannelDataType(*args->channel_args, peer); return channel_data->Init(); } From c217e490b176669bf93c04e772218d88b5fef764 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 9 Dec 2016 14:03:58 -0800 Subject: [PATCH 08/13] Add function to create channel arg for client channel factory. --- .../client_channel/client_channel_factory.c | 32 +++++++++++++++++++ .../client_channel/client_channel_factory.h | 3 ++ .../chttp2/client/insecure/channel_create.c | 18 +---------- .../client/secure/secure_channel_create.c | 27 +--------------- 4 files changed, 37 insertions(+), 43 deletions(-) diff --git a/src/core/ext/client_channel/client_channel_factory.c b/src/core/ext/client_channel/client_channel_factory.c index 4900832d5734e..01eee02979256 100644 --- a/src/core/ext/client_channel/client_channel_factory.c +++ b/src/core/ext/client_channel/client_channel_factory.c @@ -55,3 +55,35 @@ grpc_channel* grpc_client_channel_factory_create_channel( return factory->vtable->create_client_channel(exec_ctx, factory, target, type, args); } + +static void *factory_arg_copy(void *factory) { + grpc_client_channel_factory_ref(factory); + return factory; +} + +static void factory_arg_destroy(void *factory) { + // TODO(roth): Remove local exec_ctx when + // https://github.com/grpc/grpc/pull/8705 is merged. + grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; + grpc_client_channel_factory_unref(&exec_ctx, factory); + grpc_exec_ctx_finish(&exec_ctx); +} + +static int factory_arg_cmp(void *factory1, void *factory2) { + if (factory1 < factory2) return -1; + if (factory1 > factory2) return 1; + return 0; +} + +static const grpc_arg_pointer_vtable factory_arg_vtable = { + factory_arg_copy, factory_arg_destroy, factory_arg_cmp}; + +grpc_arg grpc_client_channel_factory_create_channel_arg( + grpc_client_channel_factory *factory) { + grpc_arg arg; + arg.type = GRPC_ARG_POINTER; + arg.key = GRPC_ARG_CLIENT_CHANNEL_FACTORY; + arg.value.pointer.p = factory; + arg.value.pointer.vtable = &factory_arg_vtable; + return arg; +} diff --git a/src/core/ext/client_channel/client_channel_factory.h b/src/core/ext/client_channel/client_channel_factory.h index 2b8fc577b3b67..e7ad9188813a0 100644 --- a/src/core/ext/client_channel/client_channel_factory.h +++ b/src/core/ext/client_channel/client_channel_factory.h @@ -83,4 +83,7 @@ grpc_channel *grpc_client_channel_factory_create_channel( const char *target, grpc_client_channel_type type, const grpc_channel_args *args); +grpc_arg grpc_client_channel_factory_create_channel_arg( + grpc_client_channel_factory *factory); + #endif /* GRPC_CORE_EXT_CLIENT_CHANNEL_CLIENT_CHANNEL_FACTORY_H */ diff --git a/src/core/ext/transport/chttp2/client/insecure/channel_create.c b/src/core/ext/transport/chttp2/client/insecure/channel_create.c index 3ad34b087076e..1e1bed10dc963 100644 --- a/src/core/ext/transport/chttp2/client/insecure/channel_create.c +++ b/src/core/ext/transport/chttp2/client/insecure/channel_create.c @@ -76,19 +76,6 @@ static const grpc_client_channel_factory_vtable client_channel_factory_vtable = static grpc_client_channel_factory client_channel_factory = { &client_channel_factory_vtable}; -static void *cc_factory_arg_copy(void *cc_factory) { return cc_factory; } - -static void cc_factory_arg_destroy(void *cc_factory) {} - -static int cc_factory_arg_cmp(void *cc_factory1, void *cc_factory2) { - if (cc_factory1 < cc_factory2) return -1; - if (cc_factory1 > cc_factory2) return 1; - return 0; -} - -static const grpc_arg_pointer_vtable cc_factory_arg_vtable = { - cc_factory_arg_copy, cc_factory_arg_destroy, cc_factory_arg_cmp}; - /* Create a client channel: Asynchronously: - resolve target - connect to it (trying alternatives as presented) @@ -108,10 +95,7 @@ grpc_channel *grpc_insecure_channel_create(const char *target, new_args[0].type = GRPC_ARG_STRING; new_args[0].key = GRPC_ARG_SERVER_URI; new_args[0].value.string = (char *)target; - new_args[1].type = GRPC_ARG_POINTER; - new_args[1].key = GRPC_ARG_CLIENT_CHANNEL_FACTORY; - new_args[1].value.pointer.p = factory; - new_args[1].value.pointer.vtable = &cc_factory_arg_vtable; + new_args[1] = grpc_client_channel_factory_create_channel_arg(factory); grpc_channel_args *args_copy = grpc_channel_args_copy_and_add(args, new_args, GPR_ARRAY_SIZE(new_args)); // Create channel. diff --git a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c index 2bef684398729..2474f544cfdeb 100644 --- a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c +++ b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c @@ -97,28 +97,6 @@ static const grpc_client_channel_factory_vtable client_channel_factory_vtable = client_channel_factory_create_subchannel, client_channel_factory_create_channel}; -static void *cc_factory_arg_copy(void *cc_factory) { - client_channel_factory_ref(cc_factory); - return cc_factory; -} - -static void cc_factory_arg_destroy(void *cc_factory) { - // TODO(roth): remove local exec_ctx when - // https://github.com/grpc/grpc/pull/8705 is merged - grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; - client_channel_factory_unref(&exec_ctx, cc_factory); - grpc_exec_ctx_finish(&exec_ctx); -} - -static int cc_factory_arg_cmp(void *cc_factory1, void *cc_factory2) { - if (cc_factory1 < cc_factory2) return -1; - if (cc_factory1 > cc_factory2) return 1; - return 0; -} - -static const grpc_arg_pointer_vtable cc_factory_arg_vtable = { - cc_factory_arg_copy, cc_factory_arg_destroy, cc_factory_arg_cmp}; - /* Create a secure client channel: Asynchronously: - resolve target - connect to it (trying alternatives as presented) @@ -165,10 +143,7 @@ grpc_channel *grpc_secure_channel_create(grpc_channel_credentials *creds, new_args[0].type = GRPC_ARG_STRING; new_args[0].key = GRPC_ARG_SERVER_URI; new_args[0].value.string = (char *)target; - new_args[1].type = GRPC_ARG_POINTER; - new_args[1].key = GRPC_ARG_CLIENT_CHANNEL_FACTORY; - new_args[1].value.pointer.p = f; - new_args[1].value.pointer.vtable = &cc_factory_arg_vtable; + new_args[1] = grpc_client_channel_factory_create_channel_arg(&f->base); new_args[2] = grpc_security_connector_to_arg(&security_connector->base); grpc_channel_args *args_copy = grpc_channel_args_copy_and_add( new_args_from_connector != NULL ? new_args_from_connector : args, From f75d26925f3371b44b774b15165d79484f3f12e1 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 12 Dec 2016 08:41:42 -0800 Subject: [PATCH 09/13] Fix grpclb code. --- src/core/ext/lb_policy/grpclb/grpclb.c | 8 +++++- .../chttp2/client/insecure/channel_create.c | 26 +++++++++++-------- .../client/secure/secure_channel_create.c | 24 ++++++++++------- 3 files changed, 37 insertions(+), 21 deletions(-) diff --git a/src/core/ext/lb_policy/grpclb/grpclb.c b/src/core/ext/lb_policy/grpclb/grpclb.c index df0db61c22619..da1cf65060ac7 100644 --- a/src/core/ext/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/lb_policy/grpclb/grpclb.c @@ -818,9 +818,15 @@ static grpc_lb_policy *glb_create(grpc_exec_ctx *exec_ctx, * We need the LB channel to return addresses with is_balancer=false * so that it does not wind up recursively using the grpclb LB policy, * as per the special case logic in client_channel.c. + * + * Finally, we also strip out the channel arg for the server URI, + * since that will be different for the LB channel than for the parent + * channel. (The client channel factory will re-add this arg with + * the right value.) */ static const char *keys_to_remove[] = {GRPC_ARG_LB_POLICY_NAME, - GRPC_ARG_LB_ADDRESSES}; + GRPC_ARG_LB_ADDRESSES, + GRPC_ARG_SERVER_URI}; grpc_channel_args *new_args = grpc_channel_args_copy_and_remove( args->args, keys_to_remove, GPR_ARRAY_SIZE(keys_to_remove)); glb_policy->lb_channel = grpc_client_channel_factory_create_channel( diff --git a/src/core/ext/transport/chttp2/client/insecure/channel_create.c b/src/core/ext/transport/chttp2/client/insecure/channel_create.c index 1e1bed10dc963..90d3503959226 100644 --- a/src/core/ext/transport/chttp2/client/insecure/channel_create.c +++ b/src/core/ext/transport/chttp2/client/insecure/channel_create.c @@ -65,7 +65,16 @@ static grpc_channel *client_channel_factory_create_channel( grpc_exec_ctx *exec_ctx, grpc_client_channel_factory *cc_factory, const char *target, grpc_client_channel_type type, const grpc_channel_args *args) { - return grpc_channel_create(exec_ctx, target, args, GRPC_CLIENT_CHANNEL, NULL); + // Add channel arg containing the server URI. + grpc_arg arg; + arg.type = GRPC_ARG_STRING; + arg.key = GRPC_ARG_SERVER_URI; + arg.value.string = (char *)target; + grpc_channel_args *new_args = grpc_channel_args_copy_and_add(args, &arg, 1); + grpc_channel *channel = grpc_channel_create(exec_ctx, target, new_args, + GRPC_CLIENT_CHANNEL, NULL); + grpc_channel_args_destroy(new_args); + return channel; } static const grpc_client_channel_factory_vtable client_channel_factory_vtable = @@ -90,19 +99,14 @@ grpc_channel *grpc_insecure_channel_create(const char *target, GPR_ASSERT(reserved == NULL); grpc_client_channel_factory *factory = (grpc_client_channel_factory *)&client_channel_factory; - // Add channel args containing the server name and client channel factory. - grpc_arg new_args[2]; - new_args[0].type = GRPC_ARG_STRING; - new_args[0].key = GRPC_ARG_SERVER_URI; - new_args[0].value.string = (char *)target; - new_args[1] = grpc_client_channel_factory_create_channel_arg(factory); - grpc_channel_args *args_copy = - grpc_channel_args_copy_and_add(args, new_args, GPR_ARRAY_SIZE(new_args)); + // Add channel arg containing the client channel factory. + grpc_arg arg = grpc_client_channel_factory_create_channel_arg(factory); + grpc_channel_args *new_args = grpc_channel_args_copy_and_add(args, &arg, 1); // Create channel. grpc_channel *channel = client_channel_factory_create_channel( - &exec_ctx, factory, target, GRPC_CLIENT_CHANNEL_TYPE_REGULAR, args_copy); + &exec_ctx, factory, target, GRPC_CLIENT_CHANNEL_TYPE_REGULAR, new_args); // Clean up. - grpc_channel_args_destroy(args_copy); + grpc_channel_args_destroy(new_args); grpc_client_channel_factory_unref(&exec_ctx, factory); grpc_exec_ctx_finish(&exec_ctx); return channel != NULL ? channel : grpc_lame_client_channel_create( diff --git a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c index 2474f544cfdeb..9b6d3819b677e 100644 --- a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c +++ b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c @@ -89,7 +89,16 @@ static grpc_channel *client_channel_factory_create_channel( grpc_exec_ctx *exec_ctx, grpc_client_channel_factory *cc_factory, const char *target, grpc_client_channel_type type, const grpc_channel_args *args) { - return grpc_channel_create(exec_ctx, target, args, GRPC_CLIENT_CHANNEL, NULL); + // Add channel arg containing the server URI. + grpc_arg arg; + arg.type = GRPC_ARG_STRING; + arg.key = GRPC_ARG_SERVER_URI; + arg.value.string = (char *)target; + grpc_channel_args *new_args = grpc_channel_args_copy_and_add(args, &arg, 1); + grpc_channel *channel = grpc_channel_create(exec_ctx, target, new_args, + GRPC_CLIENT_CHANNEL, NULL); + grpc_channel_args_destroy(new_args); + return channel; } static const grpc_client_channel_factory_vtable client_channel_factory_vtable = @@ -137,14 +146,11 @@ grpc_channel *grpc_secure_channel_create(grpc_channel_credentials *creds, GRPC_SECURITY_CONNECTOR_REF(&security_connector->base, "grpc_secure_channel_create"); f->security_connector = security_connector; - // Add channel args containing the server name, client channel - // factory, and security connector. - grpc_arg new_args[3]; - new_args[0].type = GRPC_ARG_STRING; - new_args[0].key = GRPC_ARG_SERVER_URI; - new_args[0].value.string = (char *)target; - new_args[1] = grpc_client_channel_factory_create_channel_arg(&f->base); - new_args[2] = grpc_security_connector_to_arg(&security_connector->base); + // Add channel args containing the client channel factory and security + // connector. + grpc_arg new_args[2]; + new_args[0] = grpc_client_channel_factory_create_channel_arg(&f->base); + new_args[1] = grpc_security_connector_to_arg(&security_connector->base); grpc_channel_args *args_copy = grpc_channel_args_copy_and_add( new_args_from_connector != NULL ? new_args_from_connector : args, new_args, GPR_ARRAY_SIZE(new_args)); From 201db7d613e73d3324d541d0e87a860dd656ca9f Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 12 Dec 2016 09:36:02 -0800 Subject: [PATCH 10/13] Eliminate redundant places where server name was stored. --- include/grpc/impl/codegen/grpc_types.h | 2 -- .../client_channel/http_connect_handshaker.c | 30 ++++++++++------- .../client_channel/http_connect_handshaker.h | 5 ++- .../ext/client_channel/resolver_registry.c | 32 +++++++++++++------ .../ext/client_channel/resolver_registry.h | 4 +++ src/core/ext/client_channel/subchannel.h | 2 -- .../ext/client_channel/subchannel_index.c | 4 --- src/core/ext/lb_policy/grpclb/grpclb.c | 26 +++++++-------- .../ext/lb_policy/pick_first/pick_first.c | 12 ++----- .../ext/lb_policy/round_robin/round_robin.c | 12 ++----- .../ext/resolver/dns/native/dns_resolver.c | 7 +--- .../ext/resolver/sockaddr/sockaddr_resolver.c | 7 +--- .../chttp2/client/chttp2_connector.c | 11 ++----- .../chttp2/client/chttp2_connector.h | 3 +- .../chttp2/client/insecure/channel_create.c | 3 +- .../client/secure/secure_channel_create.c | 2 +- test/core/end2end/fake_resolver.c | 7 +--- 17 files changed, 74 insertions(+), 95 deletions(-) diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index c86f7ecd7d0d5..f2ee5af78c957 100644 --- a/include/grpc/impl/codegen/grpc_types.h +++ b/include/grpc/impl/codegen/grpc_types.h @@ -215,8 +215,6 @@ typedef struct { #define GRPC_ARG_LB_POLICY_NAME "grpc.lb_policy_name" /** Server URI. Not intended for external use. */ #define GRPC_ARG_SERVER_URI "grpc.server_uri" -/** Server name. Not intended for external use. */ -#define GRPC_ARG_SERVER_NAME "grpc.server_name" /** Resolved addresses in a form used by the LB policy. Not intended for external use. */ #define GRPC_ARG_LB_ADDRESSES "grpc.lb_addresses" diff --git a/src/core/ext/client_channel/http_connect_handshaker.c b/src/core/ext/client_channel/http_connect_handshaker.c index 572af52dfdb34..6dc68eaaeaf46 100644 --- a/src/core/ext/client_channel/http_connect_handshaker.c +++ b/src/core/ext/client_channel/http_connect_handshaker.c @@ -40,6 +40,7 @@ #include #include +#include "src/core/ext/client_channel/resolver_registry.h" #include "src/core/ext/client_channel/uri_parser.h" #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/http/format_request.h" @@ -51,7 +52,6 @@ typedef struct http_connect_handshaker { grpc_handshaker base; char* proxy_server; - char* server_name; gpr_refcount refcount; gpr_mu mu; @@ -86,7 +86,6 @@ static void http_connect_handshaker_unref(grpc_exec_ctx* exec_ctx, gpr_free(handshaker->read_buffer_to_destroy); } gpr_free(handshaker->proxy_server); - gpr_free(handshaker->server_name); grpc_slice_buffer_destroy(&handshaker->write_buffer); grpc_http_parser_destroy(&handshaker->http_parser); grpc_http_response_destroy(&handshaker->http_response); @@ -265,18 +264,27 @@ static void http_connect_handshaker_do_handshake( grpc_tcp_server_acceptor* acceptor, grpc_closure* on_handshake_done, grpc_handshaker_args* args) { http_connect_handshaker* handshaker = (http_connect_handshaker*)handshaker_in; - gpr_mu_lock(&handshaker->mu); + // Get server name from channel args. + const grpc_arg* arg = grpc_channel_args_find(args->args, GRPC_ARG_SERVER_URI); + GPR_ASSERT(arg != NULL); + GPR_ASSERT(arg->type == GRPC_ARG_STRING); + char *canonical_uri = + grpc_resolver_factory_add_default_prefix_if_needed(arg->value.string); + grpc_uri* uri = grpc_uri_parse(canonical_uri, 1); + char* server_name = uri->path; + if (server_name[0] == '/') ++server_name; // Save state in the handshaker object. + gpr_mu_lock(&handshaker->mu); handshaker->args = args; handshaker->on_handshake_done = on_handshake_done; // Send HTTP CONNECT request. - gpr_log(GPR_INFO, "Connecting to server %s via HTTP proxy %s", - handshaker->server_name, handshaker->proxy_server); + gpr_log(GPR_INFO, "Connecting to server %s via HTTP proxy %s", server_name, + handshaker->proxy_server); grpc_httpcli_request request; memset(&request, 0, sizeof(request)); - request.host = handshaker->proxy_server; + request.host = server_name; request.http.method = "CONNECT"; - request.http.path = handshaker->server_name; + request.http.path = server_name; request.handshaker = &grpc_httpcli_plaintext; grpc_slice request_slice = grpc_httpcli_format_connect_request(&request); grpc_slice_buffer_add(&handshaker->write_buffer, request_slice); @@ -285,23 +293,23 @@ static void http_connect_handshaker_do_handshake( grpc_endpoint_write(exec_ctx, args->endpoint, &handshaker->write_buffer, &handshaker->request_done_closure); gpr_mu_unlock(&handshaker->mu); + // Clean up. + gpr_free(canonical_uri); + grpc_uri_destroy(uri); } static const grpc_handshaker_vtable http_connect_handshaker_vtable = { http_connect_handshaker_destroy, http_connect_handshaker_shutdown, http_connect_handshaker_do_handshake}; -grpc_handshaker* grpc_http_connect_handshaker_create(const char* proxy_server, - const char* server_name) { +grpc_handshaker* grpc_http_connect_handshaker_create(const char* proxy_server) { GPR_ASSERT(proxy_server != NULL); - GPR_ASSERT(server_name != NULL); http_connect_handshaker* handshaker = gpr_malloc(sizeof(*handshaker)); memset(handshaker, 0, sizeof(*handshaker)); grpc_handshaker_init(&http_connect_handshaker_vtable, &handshaker->base); gpr_mu_init(&handshaker->mu); gpr_ref_init(&handshaker->refcount, 1); handshaker->proxy_server = gpr_strdup(proxy_server); - handshaker->server_name = gpr_strdup(server_name); grpc_slice_buffer_init(&handshaker->write_buffer); grpc_closure_init(&handshaker->request_done_closure, on_write_done, handshaker); diff --git a/src/core/ext/client_channel/http_connect_handshaker.h b/src/core/ext/client_channel/http_connect_handshaker.h index c689df2b2b52e..ea293852e60bc 100644 --- a/src/core/ext/client_channel/http_connect_handshaker.h +++ b/src/core/ext/client_channel/http_connect_handshaker.h @@ -36,9 +36,8 @@ #include "src/core/lib/channel/handshaker.h" -/// Does NOT take ownership of \a proxy_server or \a server_name. -grpc_handshaker* grpc_http_connect_handshaker_create(const char* proxy_server, - const char* server_name); +/// Does NOT take ownership of \a proxy_server. +grpc_handshaker* grpc_http_connect_handshaker_create(const char* proxy_server); /// Returns the name of the proxy to use, or NULL if no proxy is configured. /// Caller takes ownership of result. diff --git a/src/core/ext/client_channel/resolver_registry.c b/src/core/ext/client_channel/resolver_registry.c index d0f0fc3f332f0..2b62b976a9575 100644 --- a/src/core/ext/client_channel/resolver_registry.c +++ b/src/core/ext/client_channel/resolver_registry.c @@ -109,8 +109,8 @@ static grpc_resolver_factory *lookup_factory_by_uri(grpc_uri *uri) { } static grpc_resolver_factory *resolve_factory(const char *target, - grpc_uri **uri) { - char *tmp; + grpc_uri **uri, + char **canonical_target) { grpc_resolver_factory *factory = NULL; GPR_ASSERT(uri != NULL); @@ -118,15 +118,15 @@ static grpc_resolver_factory *resolve_factory(const char *target, factory = lookup_factory_by_uri(*uri); if (factory == NULL) { grpc_uri_destroy(*uri); - gpr_asprintf(&tmp, "%s%s", g_default_resolver_prefix, target); - *uri = grpc_uri_parse(tmp, 1); + gpr_asprintf(canonical_target, "%s%s", g_default_resolver_prefix, target); + *uri = grpc_uri_parse(*canonical_target, 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); + grpc_uri_destroy(grpc_uri_parse(*canonical_target, 0)); + gpr_log(GPR_ERROR, "don't know how to resolve '%s' or '%s'", target, + *canonical_target); } - gpr_free(tmp); } return factory; } @@ -134,7 +134,9 @@ static grpc_resolver_factory *resolve_factory(const char *target, grpc_resolver *grpc_resolver_create(const char *target, const grpc_channel_args *args) { grpc_uri *uri = NULL; - grpc_resolver_factory *factory = resolve_factory(target, &uri); + char *canonical_target = NULL; + grpc_resolver_factory *factory = + resolve_factory(target, &uri, &canonical_target); grpc_resolver *resolver; grpc_resolver_args resolver_args; memset(&resolver_args, 0, sizeof(resolver_args)); @@ -142,13 +144,25 @@ grpc_resolver *grpc_resolver_create(const char *target, resolver_args.args = args; resolver = grpc_resolver_factory_create_resolver(factory, &resolver_args); grpc_uri_destroy(uri); + gpr_free(canonical_target); return resolver; } char *grpc_get_default_authority(const char *target) { grpc_uri *uri = NULL; - grpc_resolver_factory *factory = resolve_factory(target, &uri); + char *canonical_target = NULL; + grpc_resolver_factory *factory = + resolve_factory(target, &uri, &canonical_target); char *authority = grpc_resolver_factory_get_default_authority(factory, uri); grpc_uri_destroy(uri); + gpr_free(canonical_target); return authority; } + +char *grpc_resolver_factory_add_default_prefix_if_needed(const char *target) { + grpc_uri *uri = NULL; + char *canonical_target = NULL; + resolve_factory(target, &uri, &canonical_target); + grpc_uri_destroy(uri); + return canonical_target == NULL ? gpr_strdup(target) : canonical_target; +} diff --git a/src/core/ext/client_channel/resolver_registry.h b/src/core/ext/client_channel/resolver_registry.h index 2a95a669f0d47..24678bc05f010 100644 --- a/src/core/ext/client_channel/resolver_registry.h +++ b/src/core/ext/client_channel/resolver_registry.h @@ -71,4 +71,8 @@ grpc_resolver_factory *grpc_resolver_factory_lookup(const char *name); representing the default authority to pass from a client. */ char *grpc_get_default_authority(const char *target); +/** Returns a newly allocated string containing \a target, adding the + default prefix if needed. */ +char *grpc_resolver_factory_add_default_prefix_if_needed(const char *target); + #endif /* GRPC_CORE_EXT_CLIENT_CHANNEL_RESOLVER_REGISTRY_H */ diff --git a/src/core/ext/client_channel/subchannel.h b/src/core/ext/client_channel/subchannel.h index 10bae620dfa6e..24aa9f73dca80 100644 --- a/src/core/ext/client_channel/subchannel.h +++ b/src/core/ext/client_channel/subchannel.h @@ -164,8 +164,6 @@ struct grpc_subchannel_args { size_t filter_count; /** Channel arguments to be supplied to the newly created channel */ const grpc_channel_args *args; - /** Server name */ - const char *server_name; /** Address to connect to */ grpc_resolved_address *addr; }; diff --git a/src/core/ext/client_channel/subchannel_index.c b/src/core/ext/client_channel/subchannel_index.c index 227013a7d78e1..a1ba5e945c2ab 100644 --- a/src/core/ext/client_channel/subchannel_index.c +++ b/src/core/ext/client_channel/subchannel_index.c @@ -86,7 +86,6 @@ static grpc_subchannel_key *create_key( } else { k->args.filters = NULL; } - k->args.server_name = gpr_strdup(args->server_name); k->args.addr = gpr_malloc(sizeof(grpc_resolved_address)); k->args.addr->len = args->addr->len; if (k->args.addr->len > 0) { @@ -113,8 +112,6 @@ static int subchannel_key_compare(grpc_subchannel_key *a, if (c != 0) return c; c = GPR_ICMP(a->args.filter_count, b->args.filter_count); if (c != 0) return c; - c = strcmp(a->args.server_name, b->args.server_name); - if (c != 0) return c; if (a->args.addr->len) { c = memcmp(a->args.addr->addr, b->args.addr->addr, a->args.addr->len); if (c != 0) return c; @@ -132,7 +129,6 @@ void grpc_subchannel_key_destroy(grpc_exec_ctx *exec_ctx, grpc_connector_unref(exec_ctx, k->connector); gpr_free((grpc_channel_args *)k->args.filters); grpc_channel_args_destroy((grpc_channel_args *)k->args.args); - gpr_free((void *)k->args.server_name); gpr_free(k->args.addr); gpr_free(k); } diff --git a/src/core/ext/lb_policy/grpclb/grpclb.c b/src/core/ext/lb_policy/grpclb/grpclb.c index da1cf65060ac7..b7c06a55bb3f8 100644 --- a/src/core/ext/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/lb_policy/grpclb/grpclb.c @@ -743,12 +743,6 @@ static void glb_rr_connectivity_changed(grpc_exec_ctx *exec_ctx, void *arg, static grpc_lb_policy *glb_create(grpc_exec_ctx *exec_ctx, grpc_lb_policy_factory *factory, grpc_lb_policy_args *args) { - /* Get server name. */ - const grpc_arg *arg = - grpc_channel_args_find(args->args, GRPC_ARG_SERVER_NAME); - const char *server_name = - arg != NULL && arg->type == GRPC_ARG_STRING ? arg->value.string : NULL; - /* Count the number of gRPC-LB addresses. There must be at least one. * TODO(roth): For now, we ignore non-balancer addresses, but in the * future, we may change the behavior such that we fall back to using @@ -756,7 +750,8 @@ static grpc_lb_policy *glb_create(grpc_exec_ctx *exec_ctx, * time, this should be changed to allow a list with no balancer addresses, * since the resolver might fail to return a balancer address even when * this is the right LB policy to use. */ - arg = grpc_channel_args_find(args->args, GRPC_ARG_LB_ADDRESSES); + const grpc_arg *arg = + grpc_channel_args_find(args->args, GRPC_ARG_LB_ADDRESSES); GPR_ASSERT(arg != NULL && arg->type == GRPC_ARG_POINTER); grpc_lb_addresses *addresses = arg->value.pointer.p; size_t num_grpclb_addrs = 0; @@ -768,13 +763,19 @@ static grpc_lb_policy *glb_create(grpc_exec_ctx *exec_ctx, glb_lb_policy *glb_policy = gpr_malloc(sizeof(*glb_policy)); memset(glb_policy, 0, sizeof(*glb_policy)); + /* Get server name. */ + arg = grpc_channel_args_find(args->args, GRPC_ARG_SERVER_URI); + GPR_ASSERT(arg != NULL); + GPR_ASSERT(arg->type == GRPC_ARG_STRING); + grpc_uri *uri = grpc_uri_parse(arg->value.string, 1); + glb_policy->server_name = gpr_strdup(uri->path); + grpc_uri_destroy(uri); + /* All input addresses in addresses come from a resolver that claims * they are LB services. It's the resolver's responsibility to make sure - * this - * policy is only instantiated and used in that case. + * this policy is only instantiated and used in that case. * * Create a client channel over them to communicate with a LB service */ - glb_policy->server_name = gpr_strdup(server_name); glb_policy->cc_factory = args->client_channel_factory; glb_policy->args = grpc_channel_args_copy(args->args); GPR_ASSERT(glb_policy->cc_factory != NULL); @@ -824,9 +825,8 @@ static grpc_lb_policy *glb_create(grpc_exec_ctx *exec_ctx, * channel. (The client channel factory will re-add this arg with * the right value.) */ - static const char *keys_to_remove[] = {GRPC_ARG_LB_POLICY_NAME, - GRPC_ARG_LB_ADDRESSES, - GRPC_ARG_SERVER_URI}; + static const char *keys_to_remove[] = { + GRPC_ARG_LB_POLICY_NAME, GRPC_ARG_LB_ADDRESSES, GRPC_ARG_SERVER_URI}; grpc_channel_args *new_args = grpc_channel_args_copy_and_remove( args->args, keys_to_remove, GPR_ARRAY_SIZE(keys_to_remove)); glb_policy->lb_channel = grpc_client_channel_factory_create_channel( diff --git a/src/core/ext/lb_policy/pick_first/pick_first.c b/src/core/ext/lb_policy/pick_first/pick_first.c index c69f773e7828a..b9cfe6b5c076f 100644 --- a/src/core/ext/lb_policy/pick_first/pick_first.c +++ b/src/core/ext/lb_policy/pick_first/pick_first.c @@ -438,15 +438,10 @@ static grpc_lb_policy *create_pick_first(grpc_exec_ctx *exec_ctx, grpc_lb_policy_args *args) { GPR_ASSERT(args->client_channel_factory != NULL); - /* Get server name. */ - const grpc_arg *arg = - grpc_channel_args_find(args->args, GRPC_ARG_SERVER_NAME); - const char *server_name = - arg != NULL && arg->type == GRPC_ARG_STRING ? arg->value.string : NULL; - /* Find the number of backend addresses. We ignore balancer * addresses, since we don't know how to handle them. */ - arg = grpc_channel_args_find(args->args, GRPC_ARG_LB_ADDRESSES); + const grpc_arg *arg = + grpc_channel_args_find(args->args, GRPC_ARG_LB_ADDRESSES); GPR_ASSERT(arg != NULL && arg->type == GRPC_ARG_POINTER); grpc_lb_addresses *addresses = arg->value.pointer.p; size_t num_addrs = 0; @@ -472,9 +467,6 @@ static grpc_lb_policy *create_pick_first(grpc_exec_ctx *exec_ctx, } memset(&sc_args, 0, sizeof(grpc_subchannel_args)); - /* server_name will be copied as part of the subchannel creation. This makes - * the copying of server_name (a borrowed pointer) OK. */ - sc_args.server_name = server_name; sc_args.addr = &addresses->addresses[i].address; sc_args.args = args->args; diff --git a/src/core/ext/lb_policy/round_robin/round_robin.c b/src/core/ext/lb_policy/round_robin/round_robin.c index 59f84054c4073..f0305473d2c3e 100644 --- a/src/core/ext/lb_policy/round_robin/round_robin.c +++ b/src/core/ext/lb_policy/round_robin/round_robin.c @@ -703,15 +703,10 @@ static grpc_lb_policy *round_robin_create(grpc_exec_ctx *exec_ctx, grpc_lb_policy_args *args) { GPR_ASSERT(args->client_channel_factory != NULL); - /* Get server name. */ - const grpc_arg *arg = - grpc_channel_args_find(args->args, GRPC_ARG_SERVER_NAME); - const char *server_name = - arg != NULL && arg->type == GRPC_ARG_STRING ? arg->value.string : NULL; - /* Find the number of backend addresses. We ignore balancer * addresses, since we don't know how to handle them. */ - arg = grpc_channel_args_find(args->args, GRPC_ARG_LB_ADDRESSES); + const grpc_arg *arg = + grpc_channel_args_find(args->args, GRPC_ARG_LB_ADDRESSES); GPR_ASSERT(arg != NULL && arg->type == GRPC_ARG_POINTER); grpc_lb_addresses *addresses = arg->value.pointer.p; size_t num_addrs = 0; @@ -734,9 +729,6 @@ static grpc_lb_policy *round_robin_create(grpc_exec_ctx *exec_ctx, if (addresses->addresses[i].is_balancer) continue; memset(&sc_args, 0, sizeof(grpc_subchannel_args)); - /* server_name will be copied as part of the subchannel creation. This makes - * the copying of server_name (a borrowed pointer) OK. */ - sc_args.server_name = server_name; sc_args.addr = &addresses->addresses[i].address; sc_args.args = args->args; diff --git a/src/core/ext/resolver/dns/native/dns_resolver.c b/src/core/ext/resolver/dns/native/dns_resolver.c index 15476f5792b4b..a7392e14ad7ae 100644 --- a/src/core/ext/resolver/dns/native/dns_resolver.c +++ b/src/core/ext/resolver/dns/native/dns_resolver.c @@ -264,12 +264,7 @@ static grpc_resolver *dns_create(grpc_resolver_args *args, grpc_resolver_init(&r->base, &dns_resolver_vtable); r->name_to_resolve = proxy_name == NULL ? gpr_strdup(path) : proxy_name; r->default_port = gpr_strdup(default_port); - grpc_arg server_name_arg; - server_name_arg.type = GRPC_ARG_STRING; - server_name_arg.key = GRPC_ARG_SERVER_NAME; - server_name_arg.value.string = (char *)path; - r->channel_args = - grpc_channel_args_copy_and_add(args->args, &server_name_arg, 1); + r->channel_args = grpc_channel_args_copy(args->args); gpr_backoff_init(&r->backoff_state, GRPC_DNS_INITIAL_CONNECT_BACKOFF_SECONDS, GRPC_DNS_RECONNECT_BACKOFF_MULTIPLIER, GRPC_DNS_RECONNECT_JITTER, diff --git a/src/core/ext/resolver/sockaddr/sockaddr_resolver.c b/src/core/ext/resolver/sockaddr/sockaddr_resolver.c index 26a650aaddeec..0a9b1aa49ad73 100644 --- a/src/core/ext/resolver/sockaddr/sockaddr_resolver.c +++ b/src/core/ext/resolver/sockaddr/sockaddr_resolver.c @@ -198,12 +198,7 @@ static grpc_resolver *sockaddr_create(grpc_resolver_args *args, sockaddr_resolver *r = gpr_malloc(sizeof(sockaddr_resolver)); memset(r, 0, sizeof(*r)); r->addresses = addresses; - grpc_arg server_name_arg; - server_name_arg.type = GRPC_ARG_STRING; - server_name_arg.key = GRPC_ARG_SERVER_NAME; - server_name_arg.value.string = args->uri->path; - r->channel_args = - grpc_channel_args_copy_and_add(args->args, &server_name_arg, 1); + r->channel_args = grpc_channel_args_copy(args->args); gpr_mu_init(&r->mu); grpc_resolver_init(&r->base, &sockaddr_resolver_vtable); return &r->base; diff --git a/src/core/ext/transport/chttp2/client/chttp2_connector.c b/src/core/ext/transport/chttp2/client/chttp2_connector.c index 58a6877b9b4b9..114bb07222f77 100644 --- a/src/core/ext/transport/chttp2/client/chttp2_connector.c +++ b/src/core/ext/transport/chttp2/client/chttp2_connector.c @@ -58,7 +58,6 @@ typedef struct { bool shutdown; bool connecting; - char *server_name; grpc_chttp2_add_handshakers_func add_handshakers; void *add_handshakers_user_data; @@ -89,7 +88,6 @@ static void chttp2_connector_unref(grpc_exec_ctx *exec_ctx, // If handshaking is not yet in progress, destroy the endpoint. // Otherwise, the handshaker will do this for us. if (c->endpoint != NULL) grpc_endpoint_destroy(exec_ctx, c->endpoint); - gpr_free(c->server_name); gpr_free(c); } } @@ -155,9 +153,8 @@ static void start_handshake_locked(grpc_exec_ctx *exec_ctx, c->handshake_mgr = grpc_handshake_manager_create(); char *proxy_name = grpc_get_http_proxy_server(); if (proxy_name != NULL) { - grpc_handshake_manager_add( - c->handshake_mgr, - grpc_http_connect_handshaker_create(proxy_name, c->server_name)); + grpc_handshake_manager_add(c->handshake_mgr, + grpc_http_connect_handshaker_create(proxy_name)); gpr_free(proxy_name); } if (c->add_handshakers != NULL) { @@ -254,15 +251,13 @@ static const grpc_connector_vtable chttp2_connector_vtable = { chttp2_connector_connect}; grpc_connector *grpc_chttp2_connector_create( - grpc_exec_ctx *exec_ctx, const char *server_name, - grpc_chttp2_add_handshakers_func add_handshakers, + grpc_exec_ctx *exec_ctx, grpc_chttp2_add_handshakers_func add_handshakers, void *add_handshakers_user_data) { chttp2_connector *c = gpr_malloc(sizeof(*c)); memset(c, 0, sizeof(*c)); c->base.vtable = &chttp2_connector_vtable; gpr_mu_init(&c->mu); gpr_ref_init(&c->refs, 1); - c->server_name = gpr_strdup(server_name); c->add_handshakers = add_handshakers; c->add_handshakers_user_data = add_handshakers_user_data; return &c->base; diff --git a/src/core/ext/transport/chttp2/client/chttp2_connector.h b/src/core/ext/transport/chttp2/client/chttp2_connector.h index c57fb1a9a0d81..58eba22417847 100644 --- a/src/core/ext/transport/chttp2/client/chttp2_connector.h +++ b/src/core/ext/transport/chttp2/client/chttp2_connector.h @@ -45,8 +45,7 @@ typedef void (*grpc_chttp2_add_handshakers_func)( /// If \a add_handshakers is non-NULL, it will be called with /// \a add_handshakers_user_data to add handshakers. grpc_connector* grpc_chttp2_connector_create( - grpc_exec_ctx* exec_ctx, const char* server_name, - grpc_chttp2_add_handshakers_func add_handshakers, + grpc_exec_ctx* exec_ctx, grpc_chttp2_add_handshakers_func add_handshakers, void* add_handshakers_user_data); #endif /* GRPC_CORE_EXT_TRANSPORT_CHTTP2_CLIENT_CHTTP2_CONNECTOR_H */ diff --git a/src/core/ext/transport/chttp2/client/insecure/channel_create.c b/src/core/ext/transport/chttp2/client/insecure/channel_create.c index 90d3503959226..a0d0652ce7566 100644 --- a/src/core/ext/transport/chttp2/client/insecure/channel_create.c +++ b/src/core/ext/transport/chttp2/client/insecure/channel_create.c @@ -54,8 +54,7 @@ static grpc_subchannel *client_channel_factory_create_subchannel( grpc_exec_ctx *exec_ctx, grpc_client_channel_factory *cc_factory, const grpc_subchannel_args *args) { grpc_connector *connector = grpc_chttp2_connector_create( - exec_ctx, args->server_name, NULL /* add_handshakers */, - NULL /* user_data */); + exec_ctx, NULL /* add_handshakers */, NULL /* user_data */); grpc_subchannel *s = grpc_subchannel_create(exec_ctx, connector, args); grpc_connector_unref(exec_ctx, connector); return s; diff --git a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c index 9b6d3819b677e..f35439cd445b5 100644 --- a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c +++ b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c @@ -79,7 +79,7 @@ static grpc_subchannel *client_channel_factory_create_subchannel( const grpc_subchannel_args *args) { client_channel_factory *f = (client_channel_factory *)cc_factory; grpc_connector *connector = grpc_chttp2_connector_create( - exec_ctx, args->server_name, add_handshakers, f->security_connector); + exec_ctx, add_handshakers, f->security_connector); grpc_subchannel *s = grpc_subchannel_create(exec_ctx, connector, args); grpc_connector_unref(exec_ctx, connector); return s; diff --git a/test/core/end2end/fake_resolver.c b/test/core/end2end/fake_resolver.c index 865b55de4d659..7380dccd809f6 100644 --- a/test/core/end2end/fake_resolver.c +++ b/test/core/end2end/fake_resolver.c @@ -181,12 +181,7 @@ static grpc_resolver* fake_resolver_create(grpc_resolver_factory* factory, // Instantiate resolver. fake_resolver* r = gpr_malloc(sizeof(fake_resolver)); memset(r, 0, sizeof(*r)); - grpc_arg server_name_arg; - server_name_arg.type = GRPC_ARG_STRING; - server_name_arg.key = GRPC_ARG_SERVER_NAME; - server_name_arg.value.string = args->uri->path; - r->channel_args = - grpc_channel_args_copy_and_add(args->args, &server_name_arg, 1); + r->channel_args = grpc_channel_args_copy(args->args); r->addresses = addresses; gpr_mu_init(&r->mu); grpc_resolver_init(&r->base, &fake_resolver_vtable); From be5e3ca5057b93e7958c9bc757c5299b9debacee Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 12 Dec 2016 09:58:20 -0800 Subject: [PATCH 11/13] Move internal channel arg definitions out of public headers. --- include/grpc/impl/codegen/grpc_types.h | 12 ++---------- src/core/ext/client_channel/client_channel.h | 3 +++ src/core/ext/client_channel/client_channel_factory.h | 3 +++ .../ext/client_channel/http_connect_handshaker.c | 1 + src/core/ext/client_channel/lb_policy_factory.h | 3 +++ src/core/ext/lb_policy/grpclb/grpclb.c | 1 + 6 files changed, 13 insertions(+), 10 deletions(-) diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index f2ee5af78c957..4471ccf745687 100644 --- a/include/grpc/impl/codegen/grpc_types.h +++ b/include/grpc/impl/codegen/grpc_types.h @@ -206,22 +206,14 @@ typedef struct { /** If non-zero, allow the use of SO_REUSEPORT if it's available (default 1) */ #define GRPC_ARG_ALLOW_REUSEPORT "grpc.so_reuseport" /** If non-zero, a pointer to a buffer pool (use grpc_resource_quota_arg_vtable - to fetch an appropriate pointer arg vtable */ + to fetch an appropriate pointer arg vtable) */ #define GRPC_ARG_RESOURCE_QUOTA "grpc.resource_quota" -/** Service config data, to be passed to subchannels. - Not intended for external use. */ +/** Service config data in JSON form. Not intended for use outside of tests. */ #define GRPC_ARG_SERVICE_CONFIG "grpc.service_config" /** LB policy name. */ #define GRPC_ARG_LB_POLICY_NAME "grpc.lb_policy_name" -/** Server URI. Not intended for external use. */ -#define GRPC_ARG_SERVER_URI "grpc.server_uri" -/** Resolved addresses in a form used by the LB policy. - Not intended for external use. */ -#define GRPC_ARG_LB_ADDRESSES "grpc.lb_addresses" /** The grpc_socket_mutator instance that set the socket options. A pointer. */ #define GRPC_ARG_SOCKET_MUTATOR "grpc.socket_mutator" -/** Client channel factory. Not intended for external use. */ -#define GRPC_ARG_CLIENT_CHANNEL_FACTORY "grpc.client_channel_factory" /** \} */ /** Result of a grpc call. If the caller satisfies the prerequisites of a diff --git a/src/core/ext/client_channel/client_channel.h b/src/core/ext/client_channel/client_channel.h index 9ba012865cfac..f02587d0c1e69 100644 --- a/src/core/ext/client_channel/client_channel.h +++ b/src/core/ext/client_channel/client_channel.h @@ -38,6 +38,9 @@ #include "src/core/ext/client_channel/resolver.h" #include "src/core/lib/channel/channel_stack.h" +// Channel arg key for server URI string. +#define GRPC_ARG_SERVER_URI "grpc.server_uri" + /* A client channel is a channel that begins disconnected, and can connect to some endpoint on demand. If that endpoint disconnects, it will be connected to again later. diff --git a/src/core/ext/client_channel/client_channel_factory.h b/src/core/ext/client_channel/client_channel_factory.h index e7ad9188813a0..bf2764b5370f8 100644 --- a/src/core/ext/client_channel/client_channel_factory.h +++ b/src/core/ext/client_channel/client_channel_factory.h @@ -39,6 +39,9 @@ #include "src/core/ext/client_channel/subchannel.h" #include "src/core/lib/channel/channel_stack.h" +// Channel arg key for client channel factory. +#define GRPC_ARG_CLIENT_CHANNEL_FACTORY "grpc.client_channel_factory" + typedef struct grpc_client_channel_factory grpc_client_channel_factory; typedef struct grpc_client_channel_factory_vtable grpc_client_channel_factory_vtable; diff --git a/src/core/ext/client_channel/http_connect_handshaker.c b/src/core/ext/client_channel/http_connect_handshaker.c index 6dc68eaaeaf46..cf10dfb3e942e 100644 --- a/src/core/ext/client_channel/http_connect_handshaker.c +++ b/src/core/ext/client_channel/http_connect_handshaker.c @@ -40,6 +40,7 @@ #include #include +#include "src/core/ext/client_channel/client_channel.h" #include "src/core/ext/client_channel/resolver_registry.h" #include "src/core/ext/client_channel/uri_parser.h" #include "src/core/lib/channel/channel_args.h" diff --git a/src/core/ext/client_channel/lb_policy_factory.h b/src/core/ext/client_channel/lb_policy_factory.h index e2b8080a32917..79b3dee259254 100644 --- a/src/core/ext/client_channel/lb_policy_factory.h +++ b/src/core/ext/client_channel/lb_policy_factory.h @@ -40,6 +40,9 @@ #include "src/core/lib/iomgr/exec_ctx.h" #include "src/core/lib/iomgr/resolve_address.h" +// Channel arg key for grpc_lb_addresses. +#define GRPC_ARG_LB_ADDRESSES "grpc.lb_addresses" + typedef struct grpc_lb_policy_factory grpc_lb_policy_factory; typedef struct grpc_lb_policy_factory_vtable grpc_lb_policy_factory_vtable; diff --git a/src/core/ext/lb_policy/grpclb/grpclb.c b/src/core/ext/lb_policy/grpclb/grpclb.c index b7c06a55bb3f8..38eebdc758033 100644 --- a/src/core/ext/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/lb_policy/grpclb/grpclb.c @@ -106,6 +106,7 @@ #include #include +#include "src/core/ext/client_channel/client_channel.h" #include "src/core/ext/client_channel/client_channel_factory.h" #include "src/core/ext/client_channel/lb_policy_factory.h" #include "src/core/ext/client_channel/lb_policy_registry.h" From bcd54cdf7d47decc8aebfc469fe1bd46ec6e9d55 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 12 Dec 2016 10:02:35 -0800 Subject: [PATCH 12/13] Fix sockaddr_resolver_test. --- test/core/client_channel/resolvers/sockaddr_resolver_test.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/test/core/client_channel/resolvers/sockaddr_resolver_test.c b/test/core/client_channel/resolvers/sockaddr_resolver_test.c index ebf311ab83f8e..5ef248f036d12 100644 --- a/test/core/client_channel/resolvers/sockaddr_resolver_test.c +++ b/test/core/client_channel/resolvers/sockaddr_resolver_test.c @@ -49,11 +49,6 @@ typedef struct on_resolution_arg { void on_resolution_cb(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { on_resolution_arg *res = arg; - const grpc_arg *channel_arg = - grpc_channel_args_find(res->resolver_result, GRPC_ARG_SERVER_NAME); - GPR_ASSERT(channel_arg != NULL); - GPR_ASSERT(channel_arg->type == GRPC_ARG_STRING); - GPR_ASSERT(strcmp(res->expected_server_name, channel_arg->value.string) == 0); grpc_channel_args_destroy(res->resolver_result); } From f8439141a0727d8ccb7b3ce61c368aa2fdb4ca68 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 14 Dec 2016 12:36:39 -0800 Subject: [PATCH 13/13] clang-format --- src/core/ext/client_channel/client_channel_factory.c | 8 ++++---- src/core/ext/client_channel/http_connect_handshaker.c | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/core/ext/client_channel/client_channel_factory.c b/src/core/ext/client_channel/client_channel_factory.c index 01eee02979256..4eb35dfcf7c4c 100644 --- a/src/core/ext/client_channel/client_channel_factory.c +++ b/src/core/ext/client_channel/client_channel_factory.c @@ -56,12 +56,12 @@ grpc_channel* grpc_client_channel_factory_create_channel( args); } -static void *factory_arg_copy(void *factory) { +static void* factory_arg_copy(void* factory) { grpc_client_channel_factory_ref(factory); return factory; } -static void factory_arg_destroy(void *factory) { +static void factory_arg_destroy(void* factory) { // TODO(roth): Remove local exec_ctx when // https://github.com/grpc/grpc/pull/8705 is merged. grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; @@ -69,7 +69,7 @@ static void factory_arg_destroy(void *factory) { grpc_exec_ctx_finish(&exec_ctx); } -static int factory_arg_cmp(void *factory1, void *factory2) { +static int factory_arg_cmp(void* factory1, void* factory2) { if (factory1 < factory2) return -1; if (factory1 > factory2) return 1; return 0; @@ -79,7 +79,7 @@ static const grpc_arg_pointer_vtable factory_arg_vtable = { factory_arg_copy, factory_arg_destroy, factory_arg_cmp}; grpc_arg grpc_client_channel_factory_create_channel_arg( - grpc_client_channel_factory *factory) { + grpc_client_channel_factory* factory) { grpc_arg arg; arg.type = GRPC_ARG_POINTER; arg.key = GRPC_ARG_CLIENT_CHANNEL_FACTORY; diff --git a/src/core/ext/client_channel/http_connect_handshaker.c b/src/core/ext/client_channel/http_connect_handshaker.c index cf10dfb3e942e..76c78ee853338 100644 --- a/src/core/ext/client_channel/http_connect_handshaker.c +++ b/src/core/ext/client_channel/http_connect_handshaker.c @@ -269,7 +269,7 @@ static void http_connect_handshaker_do_handshake( const grpc_arg* arg = grpc_channel_args_find(args->args, GRPC_ARG_SERVER_URI); GPR_ASSERT(arg != NULL); GPR_ASSERT(arg->type == GRPC_ARG_STRING); - char *canonical_uri = + char* canonical_uri = grpc_resolver_factory_add_default_prefix_if_needed(arg->value.string); grpc_uri* uri = grpc_uri_parse(canonical_uri, 1); char* server_name = uri->path;