From f79ce7d2016ecdd8e7b063de4a8fa9914e7148e6 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 4 Nov 2016 08:43:36 -0700 Subject: [PATCH] Code review changes. --- include/grpc/grpc.h | 7 +++++-- src/core/ext/client_channel/client_channel.c | 13 ++++++++----- src/core/lib/channel/channel_stack.c | 2 +- src/core/lib/channel/channel_stack.h | 4 ++-- src/core/lib/channel/connected_channel.c | 2 +- src/core/lib/surface/channel.c | 2 +- src/core/lib/surface/lame_client.c | 2 +- test/core/client_channel/lb_policies_test.c | 5 +++++ 8 files changed, 24 insertions(+), 13 deletions(-) diff --git a/include/grpc/grpc.h b/include/grpc/grpc.h index 9dfcca97667..a228683d410 100644 --- a/include/grpc/grpc.h +++ b/include/grpc/grpc.h @@ -237,9 +237,12 @@ GRPCAPI struct census_context *grpc_census_call_get_context(grpc_call *call); created for. */ GRPCAPI char *grpc_channel_get_target(grpc_channel *channel); -/** Request info about the channel. */ +/** Request info about the channel. + \a channel_info indicates what information is being requested and + how that information will be returned. + \a channel_info is owned by the caller. */ GRPCAPI void grpc_channel_get_info(grpc_channel *channel, - grpc_channel_info *channel_info); + const grpc_channel_info *channel_info); /** Create a client channel to 'target'. Additional channel level configuration MAY be provided by grpc_channel_args, though the expectation is that most diff --git a/src/core/ext/client_channel/client_channel.c b/src/core/ext/client_channel/client_channel.c index 3b89f938c2e..1bed965ed1f 100644 --- a/src/core/ext/client_channel/client_channel.c +++ b/src/core/ext/client_channel/client_channel.c @@ -243,7 +243,7 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg, grpc_channel_args_find(lb_policy_args.args, GRPC_ARG_LB_POLICY_NAME); if (channel_arg != NULL) { GPR_ASSERT(channel_arg->type == GRPC_ARG_STRING); - lb_policy_name = gpr_strdup(channel_arg->value.string); + lb_policy_name = channel_arg->value.string; } // Special case: If all of the addresses are balancer addresses, // assume that we should use the grpclb policy, regardless of what the @@ -267,14 +267,13 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg, "addresses, no backend addresses -- forcing use of grpclb LB " "policy", lb_policy_name); - gpr_free(lb_policy_name); } - lb_policy_name = gpr_strdup("grpclb"); + lb_policy_name = "grpclb"; } } // Use pick_first if nothing was specified and we didn't select grpclb // above. - if (lb_policy_name == NULL) lb_policy_name = gpr_strdup("pick_first"); + if (lb_policy_name == NULL) lb_policy_name = "pick_first"; lb_policy = grpc_lb_policy_create(exec_ctx, lb_policy_name, &lb_policy_args); @@ -292,6 +291,10 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg, (grpc_method_config_table *)channel_arg->value.pointer.p, method_config_convert_value, &method_parameters_vtable); } + // Before we clean up, save a copy of lb_policy_name, since it might + // be pointing to data inside chand->resolver_result. + // The copy will be saved in chand->lb_policy_name below. + lb_policy_name = gpr_strdup(lb_policy_name); grpc_channel_args_destroy(chand->resolver_result); chand->resolver_result = NULL; } @@ -435,7 +438,7 @@ static void cc_start_transport_op(grpc_exec_ctx *exec_ctx, static void cc_get_channel_info(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, - grpc_channel_info *info) { + const grpc_channel_info *info) { channel_data *chand = elem->channel_data; gpr_mu_lock(&chand->mu); if (info->lb_policy_name != NULL) { diff --git a/src/core/lib/channel/channel_stack.c b/src/core/lib/channel/channel_stack.c index 3370f0ef459..c6c90d8c16d 100644 --- a/src/core/lib/channel/channel_stack.c +++ b/src/core/lib/channel/channel_stack.c @@ -257,7 +257,7 @@ char *grpc_call_next_get_peer(grpc_exec_ctx *exec_ctx, void grpc_channel_next_get_info(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, - grpc_channel_info *channel_info) { + const grpc_channel_info *channel_info) { grpc_channel_element *next_elem = elem + 1; return next_elem->filter->get_channel_info(exec_ctx, next_elem, channel_info); } diff --git a/src/core/lib/channel/channel_stack.h b/src/core/lib/channel/channel_stack.h index 6fbcd870641..9b9ded0aeeb 100644 --- a/src/core/lib/channel/channel_stack.h +++ b/src/core/lib/channel/channel_stack.h @@ -158,7 +158,7 @@ typedef struct { /* Implement grpc_channel_get_info() */ void (*get_channel_info)(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, - grpc_channel_info *channel_info); + const grpc_channel_info *channel_info); /* The name of this filter */ const char *name; @@ -280,7 +280,7 @@ char *grpc_call_next_get_peer(grpc_exec_ctx *exec_ctx, grpc_call_element *elem); /* Pass through a request to get_channel_info() to the next child element */ void grpc_channel_next_get_info(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, - grpc_channel_info *channel_info); + const grpc_channel_info *channel_info); /* Given the top element of a channel stack, get the channel stack itself */ grpc_channel_stack *grpc_channel_stack_from_top_element( diff --git a/src/core/lib/channel/connected_channel.c b/src/core/lib/channel/connected_channel.c index 6ae2b3b6b7b..bb6986438ee 100644 --- a/src/core/lib/channel/connected_channel.c +++ b/src/core/lib/channel/connected_channel.c @@ -137,7 +137,7 @@ static char *con_get_peer(grpc_exec_ctx *exec_ctx, grpc_call_element *elem) { /* No-op. */ static void con_get_channel_info(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, - grpc_channel_info *channel_info) {} + const grpc_channel_info *channel_info) {} static const grpc_channel_filter connected_channel_filter = { con_start_transport_stream_op, diff --git a/src/core/lib/surface/channel.c b/src/core/lib/surface/channel.c index 22bf85b1261..1389df68862 100644 --- a/src/core/lib/surface/channel.c +++ b/src/core/lib/surface/channel.c @@ -176,7 +176,7 @@ char *grpc_channel_get_target(grpc_channel *channel) { } void grpc_channel_get_info(grpc_channel *channel, - grpc_channel_info *channel_info) { + const grpc_channel_info *channel_info) { grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; grpc_channel_element *elem = grpc_channel_stack_element(CHANNEL_STACK_FROM_CHANNEL(channel), 0); diff --git a/src/core/lib/surface/lame_client.c b/src/core/lib/surface/lame_client.c index 6fd1dd921c8..d0df8e7e174 100644 --- a/src/core/lib/surface/lame_client.c +++ b/src/core/lib/surface/lame_client.c @@ -90,7 +90,7 @@ static char *lame_get_peer(grpc_exec_ctx *exec_ctx, grpc_call_element *elem) { static void lame_get_channel_info(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, - grpc_channel_info *channel_info) {} + const grpc_channel_info *channel_info) {} static void lame_start_transport_op(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, diff --git a/test/core/client_channel/lb_policies_test.c b/test/core/client_channel/lb_policies_test.c index 857ba6bd520..7c639cbeb91 100644 --- a/test/core/client_channel/lb_policies_test.c +++ b/test/core/client_channel/lb_policies_test.c @@ -643,6 +643,7 @@ static void test_get_channel_info() { "test:127.0.0.1:1234?lb_policy=round_robin", NULL, NULL); // Ensures that resolver returns. grpc_channel_check_connectivity_state(channel, true /* try_to_connect */); + // Use grpc_channel_get_info() to get LB policy name. char *lb_policy_name = NULL; grpc_channel_info channel_info; channel_info.lb_policy_name = &lb_policy_name; @@ -650,6 +651,10 @@ static void test_get_channel_info() { GPR_ASSERT(lb_policy_name != NULL); GPR_ASSERT(strcmp(lb_policy_name, "round_robin") == 0); gpr_free(lb_policy_name); + // Try again without requesting anything. This is a no-op. + channel_info.lb_policy_name = NULL; + grpc_channel_get_info(channel, &channel_info); + // Clean up. grpc_channel_destroy(channel); }