Skip to content

Commit

Permalink
Merge pull request grpc#15758 from grpc/revert-15678-channel-arg-sanity
Browse files Browse the repository at this point in the history
Revert "Add Type Checking On Channel Args"
  • Loading branch information
nicolasnoble authored Jun 14, 2018
2 parents 49d229b + 7ea8a60 commit 33b77ee
Show file tree
Hide file tree
Showing 43 changed files with 399 additions and 280 deletions.
71 changes: 42 additions & 29 deletions src/core/ext/filters/client_channel/client_channel.cc
Original file line number Diff line number Diff line change
Expand Up @@ -327,14 +327,16 @@ static void on_resolver_result_changed_locked(void* arg, grpc_error* error) {
if (chand->resolver_result != nullptr) {
if (chand->resolver != nullptr) {
// Find LB policy name.
const char* lb_policy_name = grpc_channel_args_get_string(
const grpc_arg* channel_arg = grpc_channel_args_find(
chand->resolver_result, GRPC_ARG_LB_POLICY_NAME);
const char* lb_policy_name = grpc_channel_arg_get_string(channel_arg);
// Special case: If at least one balancer address is present, we use
// the grpclb policy, regardless of what the resolver actually specified.
grpc_lb_addresses* addresses =
grpc_channel_args_get_pointer<grpc_lb_addresses>(
chand->resolver_result, GRPC_ARG_LB_ADDRESSES);
if (addresses != nullptr) {
channel_arg =
grpc_channel_args_find(chand->resolver_result, GRPC_ARG_LB_ADDRESSES);
if (channel_arg != nullptr && channel_arg->type == GRPC_ARG_POINTER) {
grpc_lb_addresses* addresses =
static_cast<grpc_lb_addresses*>(channel_arg->value.pointer.p);
bool found_balancer_address = false;
for (size_t i = 0; i < addresses->num_addresses; ++i) {
if (addresses->addresses[i].is_balancer) {
Expand Down Expand Up @@ -398,15 +400,18 @@ static void on_resolver_result_changed_locked(void* arg, grpc_error* error) {
// The copy will be saved in chand->lb_policy_name below.
lb_policy_name_dup = gpr_strdup(lb_policy_name);
// Find service config.
service_config_json = gpr_strdup(grpc_channel_args_get_string(
chand->resolver_result, GRPC_ARG_SERVICE_CONFIG));
channel_arg = grpc_channel_args_find(chand->resolver_result,
GRPC_ARG_SERVICE_CONFIG);
service_config_json =
gpr_strdup(grpc_channel_arg_get_string(channel_arg));
if (service_config_json != nullptr) {
grpc_core::UniquePtr<grpc_core::ServiceConfig> service_config =
grpc_core::ServiceConfig::Create(service_config_json);
if (service_config != nullptr) {
if (chand->enable_retries) {
const char* server_uri = grpc_channel_args_get_string(
chand->resolver_result, GRPC_ARG_SERVER_URI);
channel_arg = grpc_channel_args_find(chand->resolver_result,
GRPC_ARG_SERVER_URI);
const char* server_uri = grpc_channel_arg_get_string(channel_arg);
GPR_ASSERT(server_uri != nullptr);
grpc_uri* uri = grpc_uri_parse(server_uri, true);
GPR_ASSERT(uri->path[0] != '\0');
Expand Down Expand Up @@ -643,37 +648,45 @@ static grpc_error* cc_init_channel_elem(grpc_channel_element* elem,
"client_channel");
grpc_client_channel_start_backup_polling(chand->interested_parties);
// Record max per-RPC retry buffer size.
chand->per_rpc_retry_buffer_size = (size_t)grpc_channel_args_get_integer(
args->channel_args, GRPC_ARG_PER_RPC_RETRY_BUFFER_SIZE,
{DEFAULT_PER_RPC_RETRY_BUFFER_SIZE, 0, INT_MAX});
const grpc_arg* arg = grpc_channel_args_find(
args->channel_args, GRPC_ARG_PER_RPC_RETRY_BUFFER_SIZE);
chand->per_rpc_retry_buffer_size = (size_t)grpc_channel_arg_get_integer(
arg, {DEFAULT_PER_RPC_RETRY_BUFFER_SIZE, 0, INT_MAX});
// Record enable_retries.
chand->enable_retries = grpc_channel_args_get_bool(
args->channel_args, GRPC_ARG_ENABLE_RETRIES, true);
arg = grpc_channel_args_find(args->channel_args, GRPC_ARG_ENABLE_RETRIES);
chand->enable_retries = grpc_channel_arg_get_bool(arg, true);
// Record client channel factory.
grpc_client_channel_factory* client_channel_factory =
grpc_channel_args_get_pointer<grpc_client_channel_factory>(
args->channel_args, GRPC_ARG_CLIENT_CHANNEL_FACTORY);
if (client_channel_factory == nullptr) {
arg = grpc_channel_args_find(args->channel_args,
GRPC_ARG_CLIENT_CHANNEL_FACTORY);
if (arg == nullptr) {
return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
"Missing or malformed client channel factory in args for client "
"channel filter");
"Missing client channel factory in args for client channel filter");
}
grpc_client_channel_factory_ref(client_channel_factory);
chand->client_channel_factory = client_channel_factory;
if (arg->type != GRPC_ARG_POINTER) {
return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
"client channel factory arg must be a pointer");
}
grpc_client_channel_factory_ref(
static_cast<grpc_client_channel_factory*>(arg->value.pointer.p));
chand->client_channel_factory =
static_cast<grpc_client_channel_factory*>(arg->value.pointer.p);
// Get server name to resolve, using proxy mapper if needed.
char* server_uri =
grpc_channel_args_get_string(args->channel_args, GRPC_ARG_SERVER_URI);
if (server_uri == nullptr) {
arg = grpc_channel_args_find(args->channel_args, GRPC_ARG_SERVER_URI);
if (arg == nullptr) {
return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
"Missing server uri in args for client channel filter");
}
if (arg->type != GRPC_ARG_STRING) {
return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
"Missing or malformed server uri in args for client channel filter");
"server uri arg must be a string");
}
char* proxy_name = nullptr;
grpc_channel_args* new_args = nullptr;
grpc_proxy_mappers_map_name(server_uri, args->channel_args, &proxy_name,
&new_args);
grpc_proxy_mappers_map_name(arg->value.string, args->channel_args,
&proxy_name, &new_args);
// Instantiate resolver.
chand->resolver = grpc_core::ResolverRegistry::CreateResolver(
proxy_name != nullptr ? proxy_name : server_uri,
proxy_name != nullptr ? proxy_name : arg->value.string,
new_args != nullptr ? new_args : args->channel_args,
chand->interested_parties, chand->combiner);
if (proxy_name != nullptr) gpr_free(proxy_name);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -254,8 +254,9 @@ static void http_connect_handshaker_do_handshake(
reinterpret_cast<http_connect_handshaker*>(handshaker_in);
// Check for HTTP CONNECT channel arg.
// If not found, invoke on_handshake_done without doing anything.
char* server_name =
grpc_channel_args_get_string(args->args, GRPC_ARG_HTTP_CONNECT_SERVER);
const grpc_arg* arg =
grpc_channel_args_find(args->args, GRPC_ARG_HTTP_CONNECT_SERVER);
char* server_name = grpc_channel_arg_get_string(arg);
if (server_name == nullptr) {
// Set shutdown to true so that subsequent calls to
// http_connect_handshaker_shutdown() do nothing.
Expand All @@ -266,8 +267,8 @@ static void http_connect_handshaker_do_handshake(
return;
}
// Get headers from channel args.
char* arg_header_string =
grpc_channel_args_get_string(args->args, GRPC_ARG_HTTP_CONNECT_HEADERS);
arg = grpc_channel_args_find(args->args, GRPC_ARG_HTTP_CONNECT_HEADERS);
char* arg_header_string = grpc_channel_arg_get_string(arg);
grpc_http_header* headers = nullptr;
size_t num_headers = 0;
char** header_strings = nullptr;
Expand Down
4 changes: 3 additions & 1 deletion src/core/ext/filters/client_channel/http_proxy.cc
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,9 @@ static char* get_http_proxy_server(char** user_cred) {
* should be used.
*/
bool http_proxy_enabled(const grpc_channel_args* args) {
return grpc_channel_args_get_bool(args, GRPC_ARG_ENABLE_HTTP_PROXY, true);
const grpc_arg* arg =
grpc_channel_args_find(args, GRPC_ARG_ENABLE_HTTP_PROXY);
return grpc_channel_arg_get_bool(arg, true);
}

static bool proxy_mapper_map_name(grpc_proxy_mapper* mapper,
Expand Down
38 changes: 20 additions & 18 deletions src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1045,8 +1045,8 @@ GrpcLb::GrpcLb(const grpc_lb_addresses* addresses,
grpc_combiner_scheduler(args.combiner));
grpc_connectivity_state_init(&state_tracker_, GRPC_CHANNEL_IDLE, "grpclb");
// Record server name.
const char* server_uri =
grpc_channel_args_get_string(args.args, GRPC_ARG_SERVER_URI);
const grpc_arg* arg = grpc_channel_args_find(args.args, GRPC_ARG_SERVER_URI);
const char* server_uri = grpc_channel_arg_get_string(arg);
GPR_ASSERT(server_uri != nullptr);
grpc_uri* uri = grpc_uri_parse(server_uri, true);
GPR_ASSERT(uri->path[0] != '\0');
Expand All @@ -1058,12 +1058,12 @@ GrpcLb::GrpcLb(const grpc_lb_addresses* addresses,
}
grpc_uri_destroy(uri);
// Record LB call timeout.
lb_call_timeout_ms_ = grpc_channel_args_get_integer(
args.args, GRPC_ARG_GRPCLB_CALL_TIMEOUT_MS, {0, 0, INT_MAX});
arg = grpc_channel_args_find(args.args, GRPC_ARG_GRPCLB_CALL_TIMEOUT_MS);
lb_call_timeout_ms_ = grpc_channel_arg_get_integer(arg, {0, 0, INT_MAX});
// Record fallback timeout.
lb_fallback_timeout_ms_ = grpc_channel_args_get_integer(
args.args, GRPC_ARG_GRPCLB_FALLBACK_TIMEOUT_MS,
{GRPC_GRPCLB_DEFAULT_FALLBACK_TIMEOUT_MS, 0, INT_MAX});
arg = grpc_channel_args_find(args.args, GRPC_ARG_GRPCLB_FALLBACK_TIMEOUT_MS);
lb_fallback_timeout_ms_ = grpc_channel_arg_get_integer(
arg, {GRPC_GRPCLB_DEFAULT_FALLBACK_TIMEOUT_MS, 0, INT_MAX});
// Process channel args.
ProcessChannelArgsLocked(*args.args);
}
Expand Down Expand Up @@ -1284,17 +1284,17 @@ void GrpcLb::NotifyOnStateChangeLocked(grpc_connectivity_state* current,
}

void GrpcLb::ProcessChannelArgsLocked(const grpc_channel_args& args) {
const grpc_lb_addresses* addresses =
grpc_channel_args_get_pointer<grpc_lb_addresses>(&args,
GRPC_ARG_LB_ADDRESSES);
if (GPR_UNLIKELY(addresses == nullptr)) {
const grpc_arg* arg = grpc_channel_args_find(&args, GRPC_ARG_LB_ADDRESSES);
if (GPR_UNLIKELY(arg == nullptr || arg->type != GRPC_ARG_POINTER)) {
// Ignore this update.
gpr_log(
GPR_ERROR,
"[grpclb %p] No valid LB addresses channel arg in update, ignoring.",
this);
return;
}
const grpc_lb_addresses* addresses =
static_cast<const grpc_lb_addresses*>(arg->value.pointer.p);
// Update fallback address list.
if (fallback_backend_addresses_ != nullptr) {
grpc_lb_addresses_destroy(fallback_backend_addresses_);
Expand Down Expand Up @@ -1860,12 +1860,13 @@ class GrpcLbFactory : public LoadBalancingPolicyFactory {
OrphanablePtr<LoadBalancingPolicy> CreateLoadBalancingPolicy(
const LoadBalancingPolicy::Args& args) const override {
/* Count the number of gRPC-LB addresses. There must be at least one. */
grpc_lb_addresses* addresses =
grpc_channel_args_get_pointer<grpc_lb_addresses>(args.args,
GRPC_ARG_LB_ADDRESSES);
if (addresses == nullptr) {
const grpc_arg* arg =
grpc_channel_args_find(args.args, GRPC_ARG_LB_ADDRESSES);
if (arg == nullptr || arg->type != GRPC_ARG_POINTER) {
return nullptr;
}
grpc_lb_addresses* addresses =
static_cast<grpc_lb_addresses*>(arg->value.pointer.p);
size_t num_grpclb_addrs = 0;
for (size_t i = 0; i < addresses->num_addresses; ++i) {
if (addresses->addresses[i].is_balancer) ++num_grpclb_addrs;
Expand All @@ -1892,9 +1893,10 @@ bool maybe_add_client_load_reporting_filter(grpc_channel_stack_builder* builder,
void* arg) {
const grpc_channel_args* args =
grpc_channel_stack_builder_get_channel_arguments(builder);
const char* lb_policy =
grpc_channel_args_get_string(args, GRPC_ARG_LB_POLICY_NAME);
if (lb_policy != nullptr && strcmp(lb_policy, "grpclb") == 0) {
const grpc_arg* channel_arg =
grpc_channel_args_find(args, GRPC_ARG_LB_POLICY_NAME);
if (channel_arg != nullptr && channel_arg->type == GRPC_ARG_STRING &&
strcmp(channel_arg->value.string, "grpclb") == 0) {
return grpc_channel_stack_builder_append_filter(
builder, (const grpc_channel_filter*)arg, nullptr, nullptr);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,11 @@ grpc_channel_args* grpc_lb_policy_grpclb_modify_lb_channel_args(
grpc_arg args_to_add[2];
size_t num_args_to_add = 0;
// Add arg for targets info table.
const grpc_arg* arg = grpc_channel_args_find(args, GRPC_ARG_LB_ADDRESSES);
GPR_ASSERT(arg != nullptr);
GPR_ASSERT(arg->type == GRPC_ARG_POINTER);
grpc_lb_addresses* addresses =
grpc_channel_args_get_pointer<grpc_lb_addresses>(args,
GRPC_ARG_LB_ADDRESSES);
GPR_ASSERT(addresses != nullptr);
static_cast<grpc_lb_addresses*>(arg->value.pointer.p);
grpc_core::RefCountedPtr<grpc_core::TargetAuthorityTable>
target_authority_table = grpc_core::CreateTargetAuthorityTable(addresses);
args_to_add[num_args_to_add++] =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -281,10 +281,8 @@ void PickFirst::PingOneLocked(grpc_closure* on_initiate, grpc_closure* on_ack) {
}

void PickFirst::UpdateLocked(const grpc_channel_args& args) {
const grpc_lb_addresses* addresses =
grpc_channel_args_get_pointer<const grpc_lb_addresses>(
&args, GRPC_ARG_LB_ADDRESSES);
if (addresses == nullptr) {
const grpc_arg* arg = grpc_channel_args_find(&args, GRPC_ARG_LB_ADDRESSES);
if (arg == nullptr || arg->type != GRPC_ARG_POINTER) {
if (subchannel_list_ == nullptr) {
// If we don't have a current subchannel list, go into TRANSIENT FAILURE.
grpc_connectivity_state_set(
Expand All @@ -300,6 +298,8 @@ void PickFirst::UpdateLocked(const grpc_channel_args& args) {
}
return;
}
const grpc_lb_addresses* addresses =
static_cast<const grpc_lb_addresses*>(arg->value.pointer.p);
if (grpc_lb_pick_first_trace.enabled()) {
gpr_log(GPR_INFO,
"Pick First %p received update with %" PRIuPTR " addresses", this,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -607,10 +607,8 @@ void RoundRobin::PingOneLocked(grpc_closure* on_initiate,
}

void RoundRobin::UpdateLocked(const grpc_channel_args& args) {
grpc_lb_addresses* addresses =
grpc_channel_args_get_pointer<grpc_lb_addresses>(&args,
GRPC_ARG_LB_ADDRESSES);
if (GPR_UNLIKELY(addresses == nullptr)) {
const grpc_arg* arg = grpc_channel_args_find(&args, GRPC_ARG_LB_ADDRESSES);
if (GPR_UNLIKELY(arg == nullptr || arg->type != GRPC_ARG_POINTER)) {
gpr_log(GPR_ERROR, "[RR %p] update provided no addresses; ignoring", this);
// If we don't have a current subchannel list, go into TRANSIENT_FAILURE.
// Otherwise, keep using the current subchannel list (ignore this update).
Expand All @@ -622,6 +620,8 @@ void RoundRobin::UpdateLocked(const grpc_channel_args& args) {
}
return;
}
grpc_lb_addresses* addresses =
static_cast<grpc_lb_addresses*>(arg->value.pointer.p);
if (grpc_lb_round_robin_trace.enabled()) {
gpr_log(GPR_INFO, "[RR %p] received update with %" PRIuPTR " addresses",
this, addresses->num_addresses);
Expand Down
7 changes: 5 additions & 2 deletions src/core/ext/filters/client_channel/lb_policy_factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,9 @@ grpc_arg grpc_lb_addresses_create_channel_arg(

grpc_lb_addresses* grpc_lb_addresses_find_channel_arg(
const grpc_channel_args* channel_args) {
return grpc_channel_args_get_pointer<grpc_lb_addresses>(
channel_args, GRPC_ARG_LB_ADDRESSES);
const grpc_arg* lb_addresses_arg =
grpc_channel_args_find(channel_args, GRPC_ARG_LB_ADDRESSES);
if (lb_addresses_arg == nullptr || lb_addresses_arg->type != GRPC_ARG_POINTER)
return nullptr;
return static_cast<grpc_lb_addresses*>(lb_addresses_arg->value.pointer.p);
}
Original file line number Diff line number Diff line change
Expand Up @@ -140,11 +140,14 @@ AresDnsResolver::AresDnsResolver(const ResolverArgs& args)
dns_server_ = gpr_strdup(args.uri->authority);
}
channel_args_ = grpc_channel_args_copy(args.args);
request_service_config_ = !grpc_channel_args_get_bool(
channel_args_, GRPC_ARG_SERVICE_CONFIG_DISABLE_RESOLUTION, false);
min_time_between_resolutions_ = grpc_channel_args_get_integer(
channel_args_, GRPC_ARG_DNS_MIN_TIME_BETWEEN_RESOLUTIONS_MS,
{1000, 0, INT_MAX});
const grpc_arg* arg = grpc_channel_args_find(
channel_args_, GRPC_ARG_SERVICE_CONFIG_DISABLE_RESOLUTION);
request_service_config_ = !grpc_channel_arg_get_integer(
arg, (grpc_integer_options){false, false, true});
arg = grpc_channel_args_find(channel_args_,
GRPC_ARG_DNS_MIN_TIME_BETWEEN_RESOLUTIONS_MS);
min_time_between_resolutions_ =
grpc_channel_arg_get_integer(arg, {1000, 0, INT_MAX});
interested_parties_ = grpc_pollset_set_create();
if (args.pollset_set != nullptr) {
grpc_pollset_set_add_pollset_set(interested_parties_, args.pollset_set);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,10 @@ NativeDnsResolver::NativeDnsResolver(const ResolverArgs& args)
if (path[0] == '/') ++path;
name_to_resolve_ = gpr_strdup(path);
channel_args_ = grpc_channel_args_copy(args.args);
min_time_between_resolutions_ = grpc_channel_args_get_integer(
args.args, GRPC_ARG_DNS_MIN_TIME_BETWEEN_RESOLUTIONS_MS,
{1000, 0, INT_MAX});
const grpc_arg* arg = grpc_channel_args_find(
args.args, GRPC_ARG_DNS_MIN_TIME_BETWEEN_RESOLUTIONS_MS);
min_time_between_resolutions_ =
grpc_channel_arg_get_integer(arg, {1000, 0, INT_MAX});
interested_parties_ = grpc_pollset_set_create();
if (args.pollset_set != nullptr) {
grpc_pollset_set_add_pollset_set(interested_parties_, args.pollset_set);
Expand Down
15 changes: 10 additions & 5 deletions src/core/ext/filters/client_channel/resolver/fake/fake_resolver.cc
Original file line number Diff line number Diff line change
Expand Up @@ -252,15 +252,20 @@ static const grpc_arg_pointer_vtable response_generator_arg_vtable = {

grpc_arg FakeResolverResponseGenerator::MakeChannelArg(
FakeResolverResponseGenerator* generator) {
return grpc_channel_arg_pointer_create(
const_cast<char*>(GRPC_ARG_FAKE_RESOLVER_RESPONSE_GENERATOR), generator,
&response_generator_arg_vtable);
grpc_arg arg;
arg.type = GRPC_ARG_POINTER;
arg.key = (char*)GRPC_ARG_FAKE_RESOLVER_RESPONSE_GENERATOR;
arg.value.pointer.p = generator;
arg.value.pointer.vtable = &response_generator_arg_vtable;
return arg;
}

FakeResolverResponseGenerator* FakeResolverResponseGenerator::GetFromArgs(
const grpc_channel_args* args) {
return grpc_channel_args_get_pointer<FakeResolverResponseGenerator>(
args, GRPC_ARG_FAKE_RESOLVER_RESPONSE_GENERATOR);
const grpc_arg* arg =
grpc_channel_args_find(args, GRPC_ARG_FAKE_RESOLVER_RESPONSE_GENERATOR);
if (arg == nullptr || arg->type != GRPC_ARG_POINTER) return nullptr;
return static_cast<FakeResolverResponseGenerator*>(arg->value.pointer.p);
}

//
Expand Down
5 changes: 3 additions & 2 deletions src/core/ext/filters/client_channel/subchannel.cc
Original file line number Diff line number Diff line change
Expand Up @@ -736,8 +736,9 @@ void grpc_get_subchannel_address_arg(const grpc_channel_args* args,
}

const char* grpc_get_subchannel_address_uri_arg(const grpc_channel_args* args) {
const char* addr_str =
grpc_channel_args_get_string(args, GRPC_ARG_SUBCHANNEL_ADDRESS);
const grpc_arg* addr_arg =
grpc_channel_args_find(args, GRPC_ARG_SUBCHANNEL_ADDRESS);
const char* addr_str = grpc_channel_arg_get_string(addr_arg);
GPR_ASSERT(addr_str != nullptr); // Should have been set by LB policy.
return addr_str;
}
Expand Down
4 changes: 2 additions & 2 deletions src/core/ext/filters/deadline/deadline_filter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -358,8 +358,8 @@ const grpc_channel_filter grpc_server_deadline_filter = {
};

bool grpc_deadline_checking_enabled(const grpc_channel_args* channel_args) {
return grpc_channel_args_get_bool(
channel_args, GRPC_ARG_ENABLE_DEADLINE_CHECKS,
return grpc_channel_arg_get_bool(
grpc_channel_args_find(channel_args, GRPC_ARG_ENABLE_DEADLINE_CHECKS),
!grpc_channel_args_want_minimal_stack(channel_args));
}

Expand Down
Loading

0 comments on commit 33b77ee

Please sign in to comment.