From fdee620e0146253956adf795cfccac9f516ca347 Mon Sep 17 00:00:00 2001 From: Taylor Cramer Date: Wed, 26 Jul 2017 10:25:02 -0700 Subject: [PATCH] GRPC metadata review cleanup --- src/daemon/utils_cache.c | 5 - src/grpc.cc | 237 +++++++++++++++++++++++++---------------------- 2 files changed, 127 insertions(+), 115 deletions(-) diff --git a/src/daemon/utils_cache.c b/src/daemon/utils_cache.c index 3534c3d5..ea7c3e33 100644 --- a/src/daemon/utils_cache.c +++ b/src/daemon/utils_cache.c @@ -917,11 +917,6 @@ int uc_iterator_get_meta(uc_iter_t *iter, meta_data_t **ret_meta) { if ((iter == NULL) || (iter->entry == NULL) || (ret_meta == NULL)) return -1; - if (iter->entry->meta == NULL) { - *ret_meta = NULL; - return 0; - } - *ret_meta = meta_data_clone(iter->entry->meta); return 0; diff --git a/src/grpc.cc b/src/grpc.cc index d465ffe0..0f5cfec0 100644 --- a/src/grpc.cc +++ b/src/grpc.cc @@ -56,6 +56,8 @@ using collectd::QueryValuesResponse; using google::protobuf::util::TimeUtil; +typedef google::protobuf::Map grpcMetadata; + /* * private types */ @@ -154,6 +156,124 @@ static grpc::Status unmarshal_ident(const collectd::types::Identifier &msg, return grpc::Status::OK; } /* unmarshal_ident() */ +static grpc::Status marshal_meta_data(meta_data_t *meta, + grpcMetadata *mutable_meta_data) { + char **meta_data_keys = nullptr; + int meta_data_keys_len = meta_data_toc(meta, &meta_data_keys); + if (meta_data_keys_len < 0) { + return grpc::Status(grpc::StatusCode::INTERNAL, + grpc::string("error getting metadata keys")); + } + + for (int i = 0; i < meta_data_keys_len; i++) { + char *key = meta_data_keys[i]; + int md_type = meta_data_type(meta, key); + + collectd::types::MetadataValue md_value; + md_value.Clear(); + + switch (md_type) { + case MD_TYPE_STRING: + char *md_string; + if (meta_data_get_string(meta, key, &md_string) != 0 || md_string == nullptr) { + strarray_free(meta_data_keys, meta_data_keys_len); + return grpc::Status(grpc::StatusCode::INTERNAL, + grpc::string("missing metadata")); + } + md_value.set_string_value(md_string); + free(md_string); + break; + case MD_TYPE_SIGNED_INT: + int64_t int64_value; + if (meta_data_get_signed_int(meta, key, &int64_value) != 0) { + strarray_free(meta_data_keys, meta_data_keys_len); + return grpc::Status(grpc::StatusCode::INTERNAL, + grpc::string("missing metadata")); + } + md_value.set_int64_value(int64_value); + break; + case MD_TYPE_UNSIGNED_INT: + uint64_t uint64_value; + if (meta_data_get_unsigned_int(meta, key, &uint64_value) != 0) { + strarray_free(meta_data_keys, meta_data_keys_len); + return grpc::Status(grpc::StatusCode::INTERNAL, + grpc::string("missing metadata")); + } + md_value.set_uint64_value(uint64_value); + break; + case MD_TYPE_DOUBLE: + double double_value; + if (meta_data_get_double(meta, key, &double_value) != 0) { + strarray_free(meta_data_keys, meta_data_keys_len); + return grpc::Status(grpc::StatusCode::INTERNAL, + grpc::string("missing metadata")); + } + md_value.set_double_value(double_value); + break; + case MD_TYPE_BOOLEAN: + bool bool_value; + if (meta_data_get_boolean(meta, key, &bool_value) != 0) { + strarray_free(meta_data_keys, meta_data_keys_len); + return grpc::Status(grpc::StatusCode::INTERNAL, + grpc::string("missing metadata")); + } + md_value.set_bool_value(bool_value); + break; + default: + strarray_free(meta_data_keys, meta_data_keys_len); + ERROR("grpc: invalid metadata type (%d)", md_type); + return grpc::Status(grpc::StatusCode::INTERNAL, + grpc::string("unknown metadata type")); + } + + (*mutable_meta_data)[grpc::string(key)] = md_value; + + strarray_free(meta_data_keys, meta_data_keys_len); + } + + return grpc::Status::OK; +} + +static grpc::Status unmarshal_meta_data(const grpcMetadata &rpc_metadata, + meta_data_t **md_out) { + *md_out = meta_data_create(); + if (*md_out == nullptr) { + return grpc::Status(grpc::StatusCode::RESOURCE_EXHAUSTED, + grpc::string("failed to metadata list")); + } + for (auto kv: rpc_metadata) { + auto k = kv.first.c_str(); + auto v = kv.second; + + // The meta_data collection individually allocates copies of the keys and + // string values for each entry, so it's safe for us to pass a reference + // to our short-lived strings. + + switch (v.value_case()) { + case collectd::types::MetadataValue::ValueCase::kStringValue: + meta_data_add_string(*md_out, k, v.string_value().c_str()); + break; + case collectd::types::MetadataValue::ValueCase::kInt64Value: + meta_data_add_signed_int(*md_out, k, v.int64_value()); + break; + case collectd::types::MetadataValue::ValueCase::kUint64Value: + meta_data_add_unsigned_int(*md_out, k, v.uint64_value()); + break; + case collectd::types::MetadataValue::ValueCase::kDoubleValue: + meta_data_add_double(*md_out, k, v.double_value()); + break; + case collectd::types::MetadataValue::ValueCase::kBoolValue: + meta_data_add_boolean(*md_out, k, v.bool_value()); + break; + default: + meta_data_destroy(*md_out); + return grpc::Status(grpc::StatusCode::INVALID_ARGUMENT, + grpc::string("Metadata of unknown type")); + } + } + return grpc::Status::OK; +} + static grpc::Status marshal_value_list(const value_list_t *vl, collectd::types::ValueList *msg) { auto id = msg->mutable_identifier(); @@ -172,78 +292,9 @@ static grpc::Status marshal_value_list(const value_list_t *vl, msg->clear_meta_data(); if (vl->meta != nullptr) { - char **meta_data_keys = nullptr; - int meta_data_keys_len = meta_data_toc(vl->meta, &meta_data_keys); - if (meta_data_keys_len < 0) { - return grpc::Status(grpc::StatusCode::INTERNAL, - grpc::string("error getting metadata keys")); - } - - for (int i = 0; i < meta_data_keys_len; i++) { - char *key = meta_data_keys[i]; - int md_type = meta_data_type(vl->meta, key); - - collectd::types::MetadataValue md_value; - md_value.Clear(); - - switch (md_type) { - case MD_TYPE_STRING: - char *md_string; - if (meta_data_get_string(vl->meta, key, &md_string) != 0 || md_string == nullptr) { - strarray_free(meta_data_keys, meta_data_keys_len); - return grpc::Status(grpc::StatusCode::INTERNAL, - grpc::string("missing metadata")); - } - md_value.set_string_value(md_string); - free(md_string); - break; - case MD_TYPE_SIGNED_INT: - int64_t int64_value; - if (meta_data_get_signed_int(vl->meta, key, &int64_value) != 0) { - strarray_free(meta_data_keys, meta_data_keys_len); - return grpc::Status(grpc::StatusCode::INTERNAL, - grpc::string("missing metadata")); - } - md_value.set_int64_value(int64_value); - break; - case MD_TYPE_UNSIGNED_INT: - uint64_t uint64_value; - if (meta_data_get_unsigned_int(vl->meta, key, &uint64_value) != 0) { - strarray_free(meta_data_keys, meta_data_keys_len); - return grpc::Status(grpc::StatusCode::INTERNAL, - grpc::string("missing metadata")); - } - md_value.set_uint64_value(uint64_value); - break; - case MD_TYPE_DOUBLE: - double double_value; - if (meta_data_get_double(vl->meta, key, &double_value) != 0) { - strarray_free(meta_data_keys, meta_data_keys_len); - return grpc::Status(grpc::StatusCode::INTERNAL, - grpc::string("missing metadata")); - } - md_value.set_double_value(double_value); - break; - case MD_TYPE_BOOLEAN: - bool bool_value; - if (meta_data_get_boolean(vl->meta, key, &bool_value) != 0) { - strarray_free(meta_data_keys, meta_data_keys_len); - return grpc::Status(grpc::StatusCode::INTERNAL, - grpc::string("missing metadata")); - } - md_value.set_bool_value(bool_value); - break; - default: - strarray_free(meta_data_keys, meta_data_keys_len); - return grpc::Status(grpc::StatusCode::INTERNAL, - grpc::string("unknown metadata type")); - } - - (*msg->mutable_meta_data())[grpc::string(key)] = md_value; - - if (meta_data_keys != nullptr) { - strarray_free(meta_data_keys, meta_data_keys_len); - } + grpc::Status status = marshal_meta_data(vl->meta, msg->mutable_meta_data()); + if (!status.ok()) { + return status; } } @@ -286,41 +337,9 @@ static grpc::Status unmarshal_value_list(const collectd::types::ValueList &msg, if (!status.ok()) return status; - vl->meta = meta_data_create(); - if (vl->meta == nullptr) { - return grpc::Status(grpc::StatusCode::RESOURCE_EXHAUSTED, - grpc::string("failed to metadata list")); - } - for (auto kv: msg.meta_data()) { - auto k = kv.first.c_str(); - auto v = kv.second; - - // The meta_data collection individually allocates copies of the keys and - // string values for each entry, so it's safe for us to pass a reference - // to our short-lived strings. - - switch (v.value_case()) { - case collectd::types::MetadataValue::ValueCase::kStringValue: - meta_data_add_string(vl->meta, k, v.string_value().c_str()); - break; - case collectd::types::MetadataValue::ValueCase::kInt64Value: - meta_data_add_signed_int(vl->meta, k, v.int64_value()); - break; - case collectd::types::MetadataValue::ValueCase::kUint64Value: - meta_data_add_unsigned_int(vl->meta, k, v.uint64_value()); - break; - case collectd::types::MetadataValue::ValueCase::kDoubleValue: - meta_data_add_double(vl->meta, k, v.double_value()); - break; - case collectd::types::MetadataValue::ValueCase::kBoolValue: - meta_data_add_boolean(vl->meta, k, v.bool_value()); - break; - default: - meta_data_destroy(vl->meta); - return grpc::Status(grpc::StatusCode::INVALID_ARGUMENT, - grpc::string("Metadata of unknown type")); - } - } + status = unmarshal_meta_data(msg.meta_data(), &vl->meta); + if (!status.ok()) + return status; value_t *values = NULL; size_t values_len = 0; @@ -366,9 +385,7 @@ static grpc::Status unmarshal_value_list(const collectd::types::ValueList &msg, vl->values_len = values_len; } else { meta_data_destroy(vl->meta); - if (values) { - free(values); - } + free(values); } return status; -- 2.11.0