From 6670add667d9341f065550719a209f09923fce5f Mon Sep 17 00:00:00 2001 From: Alistair Veitch Date: Thu, 21 Jan 2016 17:21:53 -0800 Subject: [PATCH] keep status with tag set, eliminate tag_set_ntags --- include/grpc/census.h | 16 +++--- src/core/census/tag_set.c | 79 +++++++++++++----------------- test/core/census/tag_set_test.c | 86 +++++++++++++-------------------- 3 files changed, 75 insertions(+), 106 deletions(-) diff --git a/include/grpc/census.h b/include/grpc/census.h index 5443488fbe4..ae4bf36b1b2 100644 --- a/include/grpc/census.h +++ b/include/grpc/census.h @@ -392,16 +392,16 @@ typedef struct { creation. @return A new, valid census_tag_set. */ -census_tag_set *census_tag_set_create(const census_tag_set *base, - const census_tag *tags, int ntags, - census_tag_set_create_status *status); +census_tag_set *census_tag_set_create( + const census_tag_set *base, const census_tag *tags, int ntags, + census_tag_set_create_status const **status); /* Destroy a tag set created by census_tag_set_create(). Once this function has been called, the tag set cannot be reused. */ void census_tag_set_destroy(census_tag_set *tags); -/* Get the total number of tags in the tag set. */ -int census_tag_set_ntags(const census_tag_set *tags); +const census_tag_set_create_status *census_tag_set_get_create_status( + const census_tag_set *tags); /* Structure used for tag set iteration. API clients should not use or reference internal fields - neither their contents or presence/absence are @@ -445,11 +445,9 @@ size_t census_tag_set_encode_propagated_binary(const census_tag_set *tags, char *buffer, size_t buf_size); /* Decode tag set buffers encoded with census_tag_set_encode_*(). Returns NULL - if there is an error in parsing either buffer. The number of tags in the - decoded tag set will be returned in status, if it is non-NULL. */ + if there is an error in parsing either buffer. */ census_tag_set *census_tag_set_decode(const char *buffer, size_t size, - const char *bin_buffer, size_t bin_size, - census_tag_set_create_status *status); + const char *bin_buffer, size_t bin_size); /* Get a contexts tag set. */ census_tag_set *census_context_tag_set(census_context *context); diff --git a/src/core/census/tag_set.c b/src/core/census/tag_set.c index 5cc777b085f..88269cf406d 100644 --- a/src/core/census/tag_set.c +++ b/src/core/census/tag_set.c @@ -113,6 +113,7 @@ struct raw_tag { // encoding/decoding. struct census_tag_set { struct tag_set tags[3]; + census_tag_set_create_status status; }; // Indices into the tags member of census_tag_set @@ -202,8 +203,7 @@ static bool tag_set_add_tag(struct tag_set *tags, const census_tag *tag, // Add/modify/delete a tag to/in a census_tag_set. Caller must validate that // tag key etc. are valid. static void cts_modify_tag(census_tag_set *tags, const census_tag *tag, - size_t key_len, - census_tag_set_create_status *status) { + size_t key_len) { // First delete the tag if it is already present. bool deleted = cts_delete_tag(tags, tag, key_len); // Determine if we need to add it back. @@ -221,19 +221,17 @@ static void cts_modify_tag(census_tag_set *tags, const census_tag *tag, added = tag_set_add_tag(&tags->tags[LOCAL_TAGS], tag, key_len); } } - if (status) { - if (deleted) { - if (call_add) { - status->n_modified_tags++; - } else { - status->n_deleted_tags++; - } + if (deleted) { + if (call_add) { + tags->status.n_modified_tags++; } else { - if (added) { - status->n_added_tags++; - } else { - status->n_ignored_tags++; - } + tags->status.n_deleted_tags++; + } + } else { + if (added) { + tags->status.n_added_tags++; + } else { + tags->status.n_ignored_tags++; } } } @@ -280,13 +278,9 @@ static void tag_set_flatten(struct tag_set *tags) { tags->ntags_alloc = tags->ntags; } -census_tag_set *census_tag_set_create(const census_tag_set *base, - const census_tag *tags, int ntags, - census_tag_set_create_status *status) { - int n_invalid_tags = 0; - if (status) { - memset(status, 0, sizeof(*status)); - } +census_tag_set *census_tag_set_create( + const census_tag_set *base, const census_tag *tags, int ntags, + census_tag_set_create_status const **status) { census_tag_set *new_ts = gpr_malloc(sizeof(census_tag_set)); // If we are given a base, copy it into our new tag set. Otherwise set it // to zero/NULL everything. @@ -297,6 +291,7 @@ census_tag_set *census_tag_set_create(const census_tag_set *base, tag_set_copy(&new_ts->tags[PROPAGATED_BINARY_TAGS], &base->tags[PROPAGATED_BINARY_TAGS]); tag_set_copy(&new_ts->tags[LOCAL_TAGS], &base->tags[LOCAL_TAGS]); + memset(&new_ts->status, 0, sizeof(new_ts->status)); } // Walk over the additional tags and, for those that aren't invalid, modify // the tag set to add/replace/delete as required. @@ -306,25 +301,30 @@ census_tag_set *census_tag_set_create(const census_tag_set *base, // ignore the tag if it is too long/short. if (key_len != 1 && key_len <= CENSUS_MAX_TAG_KV_LEN && tag->value_len <= CENSUS_MAX_TAG_KV_LEN) { - cts_modify_tag(new_ts, tag, key_len, status); + cts_modify_tag(new_ts, tag, key_len); } else { - n_invalid_tags++; + new_ts->status.n_invalid_tags++; } } // Remove any deleted tags, update status if needed, and return. tag_set_flatten(&new_ts->tags[PROPAGATED_TAGS]); tag_set_flatten(&new_ts->tags[PROPAGATED_BINARY_TAGS]); tag_set_flatten(&new_ts->tags[LOCAL_TAGS]); - if (status != NULL) { - status->n_propagated_tags = new_ts->tags[PROPAGATED_TAGS].ntags; - status->n_propagated_binary_tags = - new_ts->tags[PROPAGATED_BINARY_TAGS].ntags; - status->n_local_tags = new_ts->tags[LOCAL_TAGS].ntags; - status->n_invalid_tags = n_invalid_tags; + new_ts->status.n_propagated_tags = new_ts->tags[PROPAGATED_TAGS].ntags; + new_ts->status.n_propagated_binary_tags = + new_ts->tags[PROPAGATED_BINARY_TAGS].ntags; + new_ts->status.n_local_tags = new_ts->tags[LOCAL_TAGS].ntags; + if (status) { + *status = &new_ts->status; } return new_ts; } +const census_tag_set_create_status *census_tag_set_get_create_status( + const census_tag_set *tags) { + return &tags->status; +} + void census_tag_set_destroy(census_tag_set *tags) { gpr_free(tags->tags[PROPAGATED_TAGS].kvm); gpr_free(tags->tags[PROPAGATED_BINARY_TAGS].kvm); @@ -332,12 +332,6 @@ void census_tag_set_destroy(census_tag_set *tags) { gpr_free(tags); } -int census_tag_set_ntags(const census_tag_set *tags) { - return tags->tags[PROPAGATED_TAGS].ntags + - tags->tags[PROPAGATED_BINARY_TAGS].ntags + - tags->tags[LOCAL_TAGS].ntags; -} - // Initialize a tag set iterator. Must be called before first use of the // iterator. void census_tag_set_initialize_iterator(const census_tag_set *tags, @@ -507,11 +501,7 @@ static void tag_set_decode(struct tag_set *tags, const char *buffer, } census_tag_set *census_tag_set_decode(const char *buffer, size_t size, - const char *bin_buffer, size_t bin_size, - census_tag_set_create_status *status) { - if (status) { - memset(status, 0, sizeof(*status)); - } + const char *bin_buffer, size_t bin_size) { census_tag_set *new_ts = gpr_malloc(sizeof(census_tag_set)); memset(&new_ts->tags[LOCAL_TAGS], 0, sizeof(struct tag_set)); if (buffer == NULL) { @@ -524,11 +514,10 @@ census_tag_set *census_tag_set_decode(const char *buffer, size_t size, } else { tag_set_decode(&new_ts->tags[PROPAGATED_BINARY_TAGS], bin_buffer, bin_size); } - if (status) { - status->n_propagated_tags = new_ts->tags[PROPAGATED_TAGS].ntags; - status->n_propagated_binary_tags = - new_ts->tags[PROPAGATED_BINARY_TAGS].ntags; - } + memset(&new_ts->status, 0, sizeof(new_ts->status)); + new_ts->status.n_propagated_tags = new_ts->tags[PROPAGATED_TAGS].ntags; + new_ts->status.n_propagated_binary_tags = + new_ts->tags[PROPAGATED_BINARY_TAGS].ntags; // TODO(aveitch): check that BINARY flag is correct for each type. return new_ts; } diff --git a/test/core/census/tag_set_test.c b/test/core/census/tag_set_test.c index bc084ec04bd..752a74d4016 100644 --- a/test/core/census/tag_set_test.c +++ b/test/core/census/tag_set_test.c @@ -109,18 +109,21 @@ static bool validate_tag(const census_tag_set *cts, const census_tag *tag) { // Create an empty tag_set. static void empty_test(void) { struct census_tag_set *cts = census_tag_set_create(NULL, NULL, 0, NULL); - GPR_ASSERT(census_tag_set_ntags(cts) == 0); + GPR_ASSERT(cts != NULL); + const census_tag_set_create_status *status = + census_tag_set_get_create_status(cts); + census_tag_set_create_status expected = {0, 0, 0, 0, 0, 0, 0, 0}; + GPR_ASSERT(memcmp(status, &expected, sizeof(expected)) == 0); census_tag_set_destroy(cts); } // Test create and iteration over basic tag set. static void basic_test(void) { - census_tag_set_create_status status; + const census_tag_set_create_status *status; struct census_tag_set *cts = census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, &status); - GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); census_tag_set_create_status expected = {2, 2, 4, 0, 8, 0, 0, 0}; - GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); + GPR_ASSERT(memcmp(status, &expected, sizeof(expected)) == 0); census_tag_set_iterator it; census_tag_set_initialize_iterator(cts, &it); census_tag tag; @@ -139,9 +142,8 @@ static void basic_test(void) { static void lookup_by_key_test(void) { struct census_tag_set *cts = census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); - GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); census_tag tag; - for (int i = 0; i < census_tag_set_ntags(cts); i++) { + for (int i = 0; i < BASIC_TAG_COUNT; i++) { GPR_ASSERT(census_tag_set_get_tag_by_key(cts, basic_tags[i].key, &tag) == 1); GPR_ASSERT(compare_tag(&tag, &basic_tags[i])); @@ -166,46 +168,39 @@ static void invalid_test(void) { // long keys, short value. Key lengths (including terminator) should be // <= 255 (CENSUS_MAX_TAG_KV_LEN) GPR_ASSERT(strlen(key) == 299); - census_tag_set_create_status status; + const census_tag_set_create_status *status; struct census_tag_set *cts = census_tag_set_create(NULL, &tag, 1, &status); - GPR_ASSERT(census_tag_set_ntags(cts) == 0); census_tag_set_create_status expected = {0, 0, 0, 0, 0, 0, 1, 0}; - GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); + GPR_ASSERT(memcmp(status, &expected, sizeof(expected)) == 0); census_tag_set_destroy(cts); key[CENSUS_MAX_TAG_KV_LEN] = 0; GPR_ASSERT(strlen(key) == CENSUS_MAX_TAG_KV_LEN); cts = census_tag_set_create(NULL, &tag, 1, &status); - GPR_ASSERT(census_tag_set_ntags(cts) == 0); - GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); + GPR_ASSERT(memcmp(status, &expected, sizeof(expected)) == 0); census_tag_set_destroy(cts); key[CENSUS_MAX_TAG_KV_LEN - 1] = 0; GPR_ASSERT(strlen(key) == CENSUS_MAX_TAG_KV_LEN - 1); cts = census_tag_set_create(NULL, &tag, 1, &status); - GPR_ASSERT(census_tag_set_ntags(cts) == 1); census_tag_set_create_status expected2 = {0, 0, 1, 0, 1, 0, 0, 0}; - GPR_ASSERT(memcmp(&status, &expected2, sizeof(status)) == 0); + GPR_ASSERT(memcmp(status, &expected2, sizeof(expected2)) == 0); census_tag_set_destroy(cts); // now try with long values tag.value_len = 300; cts = census_tag_set_create(NULL, &tag, 1, &status); - GPR_ASSERT(census_tag_set_ntags(cts) == 0); - GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); + GPR_ASSERT(memcmp(status, &expected, sizeof(expected)) == 0); census_tag_set_destroy(cts); tag.value_len = CENSUS_MAX_TAG_KV_LEN + 1; cts = census_tag_set_create(NULL, &tag, 1, &status); - GPR_ASSERT(census_tag_set_ntags(cts) == 0); - GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); + GPR_ASSERT(memcmp(status, &expected, sizeof(expected)) == 0); census_tag_set_destroy(cts); tag.value_len = CENSUS_MAX_TAG_KV_LEN; cts = census_tag_set_create(NULL, &tag, 1, &status); - GPR_ASSERT(census_tag_set_ntags(cts) == 1); - GPR_ASSERT(memcmp(&status, &expected2, sizeof(status)) == 0); + GPR_ASSERT(memcmp(status, &expected2, sizeof(expected2)) == 0); census_tag_set_destroy(cts); // 0 length key. key[0] = 0; cts = census_tag_set_create(NULL, &tag, 1, &status); - GPR_ASSERT(census_tag_set_ntags(cts) == 0); - GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); + GPR_ASSERT(memcmp(status, &expected, sizeof(expected)) == 0); census_tag_set_destroy(cts); } @@ -213,13 +208,11 @@ static void invalid_test(void) { static void copy_test(void) { struct census_tag_set *cts = census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); - GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); - census_tag_set_create_status status; + const census_tag_set_create_status *status; struct census_tag_set *cts2 = census_tag_set_create(cts, NULL, 0, &status); - GPR_ASSERT(census_tag_set_ntags(cts2) == BASIC_TAG_COUNT); census_tag_set_create_status expected = {2, 2, 4, 0, 0, 0, 0, 0}; - GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); - for (int i = 0; i < census_tag_set_ntags(cts2); i++) { + GPR_ASSERT(memcmp(status, &expected, sizeof(expected)) == 0); + for (int i = 0; i < BASIC_TAG_COUNT; i++) { census_tag tag; GPR_ASSERT(census_tag_set_get_tag_by_key(cts2, basic_tags[i].key, &tag) == 1); @@ -233,13 +226,11 @@ static void copy_test(void) { static void replace_value_test(void) { struct census_tag_set *cts = census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); - GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); - census_tag_set_create_status status; + const census_tag_set_create_status *status; struct census_tag_set *cts2 = census_tag_set_create( cts, modify_tags + REPLACE_VALUE_OFFSET, 1, &status); - GPR_ASSERT(census_tag_set_ntags(cts2) == BASIC_TAG_COUNT); census_tag_set_create_status expected = {2, 2, 4, 0, 0, 1, 0, 0}; - GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); + GPR_ASSERT(memcmp(status, &expected, sizeof(expected)) == 0); census_tag tag; GPR_ASSERT(census_tag_set_get_tag_by_key( cts2, modify_tags[REPLACE_VALUE_OFFSET].key, &tag) == 1); @@ -252,13 +243,11 @@ static void replace_value_test(void) { static void replace_flags_test(void) { struct census_tag_set *cts = census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); - GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); - census_tag_set_create_status status; + const census_tag_set_create_status *status; struct census_tag_set *cts2 = census_tag_set_create(cts, modify_tags + REPLACE_FLAG_OFFSET, 1, &status); - GPR_ASSERT(census_tag_set_ntags(cts2) == BASIC_TAG_COUNT); census_tag_set_create_status expected = {1, 2, 5, 0, 0, 1, 0, 0}; - GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); + GPR_ASSERT(memcmp(status, &expected, sizeof(expected)) == 0); census_tag tag; GPR_ASSERT(census_tag_set_get_tag_by_key( cts2, modify_tags[REPLACE_FLAG_OFFSET].key, &tag) == 1); @@ -271,13 +260,11 @@ static void replace_flags_test(void) { static void delete_tag_test(void) { struct census_tag_set *cts = census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); - GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); - census_tag_set_create_status status; + const census_tag_set_create_status *status; struct census_tag_set *cts2 = census_tag_set_create(cts, modify_tags + DELETE_TAG_OFFSET, 1, &status); - GPR_ASSERT(census_tag_set_ntags(cts2) == BASIC_TAG_COUNT - 1); census_tag_set_create_status expected = {2, 1, 4, 1, 0, 0, 0, 0}; - GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); + GPR_ASSERT(memcmp(status, &expected, sizeof(expected)) == 0); census_tag tag; GPR_ASSERT(census_tag_set_get_tag_by_key( cts2, modify_tags[DELETE_TAG_OFFSET].key, &tag) == 0); @@ -289,13 +276,11 @@ static void delete_tag_test(void) { static void add_tag_test(void) { struct census_tag_set *cts = census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); - GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); - census_tag_set_create_status status; + const census_tag_set_create_status *status; struct census_tag_set *cts2 = census_tag_set_create(cts, modify_tags + ADD_TAG_OFFSET, 1, &status); - GPR_ASSERT(census_tag_set_ntags(cts2) == BASIC_TAG_COUNT + 1); census_tag_set_create_status expected = {2, 2, 5, 0, 1, 0, 0, 0}; - GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); + GPR_ASSERT(memcmp(status, &expected, sizeof(expected)) == 0); census_tag tag; GPR_ASSERT(census_tag_set_get_tag_by_key( cts2, modify_tags[ADD_TAG_OFFSET].key, &tag) == 1); @@ -308,13 +293,11 @@ static void add_tag_test(void) { static void replace_add_delete_test(void) { struct census_tag_set *cts = census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); - GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); - census_tag_set_create_status status; + const census_tag_set_create_status *status; struct census_tag_set *cts2 = census_tag_set_create(cts, modify_tags, MODIFY_TAG_COUNT, &status); - GPR_ASSERT(census_tag_set_ntags(cts2) == 9); census_tag_set_create_status expected = {2, 1, 6, 2, 3, 4, 0, 2}; - GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); + GPR_ASSERT(memcmp(status, &expected, sizeof(expected)) == 0); // validate tag set contents. Use specific indices into the two arrays // holding tag values. GPR_ASSERT(validate_tag(cts2, &basic_tags[3])); @@ -342,20 +325,19 @@ static void encode_decode_test(void) { char buf2[1000]; struct census_tag_set *cts = census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); - GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); GPR_ASSERT(census_tag_set_encode_propagated(cts, buf1, 1) == 0); size_t b1 = census_tag_set_encode_propagated(cts, buf1, 1000); GPR_ASSERT(b1 != 0); GPR_ASSERT(census_tag_set_encode_propagated_binary(cts, buf2, 1) == 0); size_t b2 = census_tag_set_encode_propagated_binary(cts, buf2, 1000); GPR_ASSERT(b2 != 0); - census_tag_set_create_status status; - census_tag_set *cts2 = census_tag_set_decode(buf1, b1, buf2, b2, &status); + census_tag_set *cts2 = census_tag_set_decode(buf1, b1, buf2, b2); GPR_ASSERT(cts2 != NULL); - GPR_ASSERT(census_tag_set_ntags(cts2) == 4); + const census_tag_set_create_status *status = + census_tag_set_get_create_status(cts2); census_tag_set_create_status expected = {2, 2, 0, 0, 0, 0, 0, 0}; - GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); - for (int i = 0; i < census_tag_set_ntags(cts); i++) { + GPR_ASSERT(memcmp(status, &expected, sizeof(expected)) == 0); + for (int i = 0; i < BASIC_TAG_COUNT; i++) { census_tag tag; if (CENSUS_TAG_IS_PROPAGATED(basic_tags[i].flags)) { GPR_ASSERT(census_tag_set_get_tag_by_key(cts2, basic_tags[i].key, &tag) ==