keep status with tag set, eliminate tag_set_ntags

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);

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;
 }

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) ==