From bb30d2591fc52c5bacc309c107077d92d5afc70a Mon Sep 17 00:00:00 2001 From: Alistair Veitch Date: Tue, 12 Jan 2016 17:36:05 -0800 Subject: initial commit --- src/core/census/tag_set.c | 527 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 527 insertions(+) create mode 100644 src/core/census/tag_set.c (limited to 'src/core/census/tag_set.c') diff --git a/src/core/census/tag_set.c b/src/core/census/tag_set.c new file mode 100644 index 0000000000..80c209031f --- /dev/null +++ b/src/core/census/tag_set.c @@ -0,0 +1,527 @@ +/* + * + * Copyright 2015, Google Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + */ + +#include "tag_set.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include "src/core/support/murmur_hash.h" +#include "src/core/support/string.h" + +// Functions in this file support the public tag_set API, as well as +// encoding/decoding tag_sets as part of context transmission across +// RPC's. The overall requirements (in approximate priority order) for the +// tag_set representations: +// 1. Efficient conversion to/from wire format +// 2. Minimal bytes used on-wire +// 3. Efficient tag set creation +// 4. Efficient lookup of value for a key +// 5. Efficient lookup of value for an index (to support iteration) +// 6. Minimal memory footprint +// +// Notes on tradeoffs/decisions: +// * tag includes 1 byte length of key, as well as nil-terminating byte. These +// are to aid in efficient parsing and the ability to directly return key +// strings. This is more important than saving a single byte/tag on the wire. +// * The wire encoding uses only single byte values. This eliminates the need +// to handle endian-ness conversions. +// * Keep all tag information (keys/values/flags) in a single memory buffer, +// that can be directly copied to the wire. This makes iteration by index +// somewhat less efficient. +// * Binary tags are encoded seperately from non-binary tags. There are a +// primarily because non-binary tags are far more likely to be repeated +// across multiple RPC calls, so are more efficiently cached and +// compressed in any metadata schemes. +// * deleted/modified tags are kept in memory, just marked with a deleted +// flag. This enables faster processing TODO: benchmark this +// * all lengths etc. are restricted to one byte. This eliminates endian +// issues. + +// Structure representing a set of tags. Essentially a count of number of tags +// present, and pointer to a chunk of memory that contains the per-tag details. +struct tag_set { + int ntags; // number of tags. + int ntags_alloc; // ntags + number of deleted tags (total number of tags + // in all of kvm). + size_t kvm_size; // number of bytes allocated for key/value storage. + size_t kvm_used; // number of bytes of used key/value memory + char *kvm; // key/value memory. Consists of repeated entries of: + // Offset Size Description + // 0 1 Key length, including trailing 0. (K) + // 1 1 Value length. (V) + // 2 1 Flags + // 3 K Key bytes + // 3 + K V Value bytes + // + // We refer to the first 3 entries as the 'tag header'. +}; + +// Number of bytes in tag header. +#define TAG_HEADER_SIZE 3 // key length (1) + value length (1) + flags (1) +// Offsets to tag header entries. +#define KEY_LEN_OFFSET 0 +#define VALUE_LEN_OFFSET 1 +#define FLAG_OFFSET 2 + +// raw_tag represents the raw-storage form of a tag in the kvm of a tag_set. +struct raw_tag { + uint8_t key_len; + uint8_t value_len; + uint8_t flags; + char *key; + char *value; +}; + +// use reserved flag bit for indication of deleted tag. +#define CENSUS_TAG_DELETED CENSUS_TAG_RESERVED +#define CENSUS_TAG_IS_DELETED(flags) (flags & CENSUS_TAG_DELETED) + +// Primary (external) representation of a tag set. Composed of 3 underlying +// tag_set structs, one for each of the binary/printable propagated tags, and +// one for everything else. +struct census_tag_set { + struct tag_set propagated_tags; + struct tag_set propagated_binary_tags; + struct tag_set local_tags; +}; + +// Extract a raw tag given a pointer (raw) to the tag header. Allow for some +// extra bytes in the tag header (see encode/decode for usage: allows for +// future expansion of the tag header). +static char *decode_tag(struct raw_tag *tag, char *header, int offset) { + tag->key_len = (uint8_t)(*header++); + tag->value_len = (uint8_t)(*header++); + tag->flags = (uint8_t)(*header++); + header += offset; + tag->key = header; + header += tag->key_len; + tag->value = header; + return header + tag->value_len; +} + +// Make a copy (in 'to') of an existing tag_set. +static void tag_set_copy(struct tag_set *to, const struct tag_set *from) { + memcpy(to, from, sizeof(struct tag_set)); + to->kvm = gpr_malloc(to->kvm_size); + memcpy(to->kvm, from->kvm, to->kvm_used); +} + +// We may want to keep information about a deleted tag for a short time, +// in case we can reuse the space (same tag is reinserted). This structure +// is used for that purpose. +struct deleted_tag_info { + struct raw_tag raw; // raw tag information. + uint8_t *raw_flags_p; // pointer to original flags + struct tag_set *tags; // the tag set from which tag was deleted. +}; + +// Delete a tag from a tag set, if it exists. Returns true if the tag was +// originally present (and is now deleted), false if it wasn't. +static bool tag_set_delete_tag(struct tag_set *tags, + struct deleted_tag_info *dtag, const char *key, + size_t key_len) { + char *kvp = tags->kvm; + for (int i = 0; i < tags->ntags_alloc; i++) { + dtag->raw_flags_p = (uint8_t *)(kvp + FLAG_OFFSET); + kvp = decode_tag(&dtag->raw, kvp, 0); + if (CENSUS_TAG_IS_DELETED(dtag->raw.flags)) continue; + if ((key_len == dtag->raw.key_len) && + (memcmp(key, dtag->raw.key, key_len) == 0)) { + *(dtag->raw_flags_p) |= CENSUS_TAG_DELETED; + tags->ntags--; + return true; + } + } + return false; +} + +// Delete a tag from a tag set. If the tag is found in any of the underlying +// tag sets, *and* that tag set corresponds to the one in which the tag (if +// later inserted) would be placed, then fills in dtag, and returns true. +// Returns false otherwise. This information is later used to optimize the +// placement of the tag if the value space can be reused, effectively +// "undeleting" the tag. +static bool cts_delete_tag(census_tag_set *tags, const census_tag *tag, + size_t key_len, struct deleted_tag_info *dtag) { + // use the to-be-deleted tag flags as a hint to determine the order in which + // we delete from the underlying tag sets. + if (CENSUS_TAG_IS_PROPAGATED(tag->flags)) { + if (CENSUS_TAG_IS_BINARY(tag->flags)) { + if (tag_set_delete_tag(&tags->propagated_binary_tags, dtag, tag->key, + key_len)) { + dtag->tags = &tags->propagated_binary_tags; + return true; + } + if (tag_set_delete_tag(&tags->propagated_tags, dtag, tag->key, key_len)) + return false; + tag_set_delete_tag(&tags->local_tags, dtag, tag->key, key_len); + } else { + if (tag_set_delete_tag(&tags->propagated_tags, dtag, tag->key, key_len)) { + dtag->tags = &tags->propagated_tags; + return true; + } + if (tag_set_delete_tag(&tags->propagated_binary_tags, dtag, tag->key, + key_len)) + return false; + tag_set_delete_tag(&tags->local_tags, dtag, tag->key, key_len); + } + } else { + if (tag_set_delete_tag(&tags->local_tags, dtag, tag->key, key_len)) { + dtag->tags = &tags->local_tags; + return true; + } + if (tag_set_delete_tag(&tags->propagated_tags, dtag, tag->key, key_len)) + return false; + tag_set_delete_tag(&tags->propagated_binary_tags, dtag, tag->key, key_len); + } + return false; +} + +// Add a tag to a tag set. +static void tag_set_add_tag(struct tag_set *tags, const census_tag *tag, + size_t key_len) { + const size_t tag_size = key_len + tag->value_len + TAG_HEADER_SIZE; + // drop tag if too many. + if (tags->ntags == CENSUS_MAX_TAGS) { + return; + } + if (tags->kvm_used + tag_size > tags->kvm_size) { + tags->kvm_size += 2 * CENSUS_MAX_TAG_KV_LEN + TAG_HEADER_SIZE; + char *new_kvm = gpr_malloc(tags->kvm_size); + memcpy(new_kvm, tags->kvm, tags->kvm_used); + gpr_free(tags->kvm); + tags->kvm = new_kvm; + } + char *kvp = tags->kvm + tags->kvm_used; + *kvp++ = (char)key_len; + *kvp++ = (char)tag->value_len; + // ensure reserved flags are not used. + *kvp++ = (char)(tag->flags & (CENSUS_TAG_PROPAGATE | CENSUS_TAG_STATS | + CENSUS_TAG_BINARY)); + memcpy(kvp, tag->key, key_len); + kvp += key_len; + memcpy(kvp, tag->value, tag->value_len); + tags->kvm_used += tag_size; + tags->ntags++; + tags->ntags_alloc++; +} + +// Add a tag to a census_tag_set +static void cts_add_tag(census_tag_set *tags, const census_tag *tag, + size_t key_len) { + // first delete the tag if it is already present + struct deleted_tag_info dtag; + bool deleted_match = cts_delete_tag(tags, tag, key_len, &dtag); + if (tag->value != NULL && tag->value_len != 0) { + if (deleted_match && tag->value_len == dtag.raw.value_len) { + // if we have a close match for tag being added to one just deleted, + // only need to modify value and flags. + memcpy(dtag.raw.value, tag->value, tag->value_len); + *dtag.raw_flags_p = (tag->flags & (CENSUS_TAG_PROPAGATE | + CENSUS_TAG_STATS | CENSUS_TAG_BINARY)); + dtag.tags->ntags++; + } else { + if (CENSUS_TAG_IS_PROPAGATED(tag->flags)) { + if (CENSUS_TAG_IS_BINARY(tag->flags)) { + tag_set_add_tag(&tags->propagated_binary_tags, tag, key_len); + } else { + tag_set_add_tag(&tags->propagated_tags, tag, key_len); + } + } else { + tag_set_add_tag(&tags->local_tags, tag, key_len); + } + } + } +} + +census_tag_set *census_tag_set_create(const census_tag_set *base, + const census_tag *tags, int ntags) { + census_tag_set *new_ts = gpr_malloc(sizeof(census_tag_set)); + if (base == NULL) { + memset(new_ts, 0, sizeof(census_tag_set)); + } else { + tag_set_copy(&new_ts->propagated_tags, &base->propagated_tags); + tag_set_copy(&new_ts->propagated_binary_tags, + &base->propagated_binary_tags); + tag_set_copy(&new_ts->local_tags, &base->local_tags); + } + for (int i = 0; i < ntags; i++) { + const census_tag *tag = &tags[i]; + size_t key_len = strlen(tag->key) + 1; + // 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_add_tag(new_ts, tag, key_len); + } + } + return new_ts; +} + +void census_tag_set_destroy(census_tag_set *tags) { + gpr_free(tags->propagated_tags.kvm); + gpr_free(tags->propagated_binary_tags.kvm); + gpr_free(tags->local_tags.kvm); + gpr_free(tags); +} + +int census_tag_set_ntags(const census_tag_set *tags) { + return tags->propagated_tags.ntags + tags->propagated_binary_tags.ntags + + tags->local_tags.ntags; +} + +// Get the nth tag in a tag set. The caller must validate that index is +// in range. +static void tag_set_get_tag_by_index(const struct tag_set *tags, int index, + census_tag *tag) { + GPR_ASSERT(index < tags->ntags); + char *kvp = tags->kvm; + for (;;) { + struct raw_tag raw; + kvp = decode_tag(&raw, kvp, 0); + if (CENSUS_TAG_IS_DELETED(raw.flags)) { + continue; + } else if (index == 0) { + tag->key = raw.key; + tag->value = raw.value; + tag->value_len = raw.value_len; + tag->flags = raw.flags; + return; + } + index--; + } + // NOT REACHED +} + +int census_tag_set_get_tag_by_index(const census_tag_set *tags, int index, + census_tag *tag) { + if (index < 0) return 0; + if (index < tags->propagated_tags.ntags) { + tag_set_get_tag_by_index(&tags->propagated_tags, index, tag); + return 1; + } + index -= tags->propagated_tags.ntags; + if (index < tags->propagated_binary_tags.ntags) { + tag_set_get_tag_by_index(&tags->propagated_binary_tags, index, tag); + return 1; + } + index -= tags->propagated_binary_tags.ntags; + if (index < tags->local_tags.ntags) { + tag_set_get_tag_by_index(&tags->local_tags, index, tag); + return 1; + } + return 0; +} + +// Find a tag in a tag_set by key. Return true if found, false otherwise. +static bool tag_set_get_tag_by_key(const struct tag_set *tags, const char *key, + size_t key_len, census_tag *tag) { + char *kvp = tags->kvm; + for (int i = 0; i < tags->ntags; i++) { + struct raw_tag raw; + do { + kvp = decode_tag(&raw, kvp, 0); + } while (CENSUS_TAG_IS_DELETED(raw.flags)); + if (key_len == raw.key_len && memcmp(raw.key, key, key_len) == 0) { + tag->key = raw.key; + tag->value = raw.value; + tag->value_len = raw.value_len; + tag->flags = raw.flags; + return true; + } + } + return false; +} + +int census_tag_set_get_tag_by_key(const census_tag_set *tags, const char *key, + census_tag *tag) { + size_t key_len = strlen(key) + 1; + if (key_len == 1) { + return 0; + } + if (tag_set_get_tag_by_key(&tags->propagated_tags, key, key_len, tag) || + tag_set_get_tag_by_key(&tags->propagated_binary_tags, key, key_len, + tag) || + tag_set_get_tag_by_key(&tags->local_tags, key, key_len, tag)) { + return 1; + } + return 0; +} + +// tag_set encoding and decoding functions. +// +// Wire format for tag sets on the wire: +// +// First, a tag set header: +// +// offset bytes description +// 0 1 version number +// 1 1 number of bytes in this header. This allows for future +// expansion. +// 2 1 number of bytes in each tag header. +// 3 1 ntags value from tag set. +// 4 1 ntags_alloc value from tag set. +// +// This is followed by the key/value memory from struct tag_set. + +#define ENCODED_VERSION 0 // Version number +#define ENCODED_HEADER_SIZE 5 // size of tag set header + +// Pack a tag set into as few bytes as possible (eliding deleted tags). Assumes +// header is already generated. +static size_t tag_set_encode_packed(const struct tag_set *tags, char *buffer, + size_t buf_size) { + size_t encoded_size = 0; + char *kvp = tags->kvm; + for (int i = 0; i < tags->ntags_alloc; i++) { + struct raw_tag raw; + char *base = kvp; + kvp = decode_tag(&raw, kvp, 0); + size_t tag_size = + TAG_HEADER_SIZE + (size_t)raw.key_len + (size_t)raw.value_len; + if (!(CENSUS_TAG_IS_DELETED(raw.flags))) { + if (tag_size > buf_size) { + return 0; + } + memcpy(buffer, base, tag_size); + buffer += tag_size; + encoded_size += tag_size; + buf_size -= tag_size; + } + } + return encoded_size; +} + +// Encode a tag set. Returns 0 if buffer is too small. +static size_t tag_set_encode(const struct tag_set *tags, char *buffer, + size_t buf_size) { + if (buf_size < ENCODED_HEADER_SIZE) { + return 0; + } + buf_size -= ENCODED_HEADER_SIZE; + *buffer++ = (char)ENCODED_VERSION; + *buffer++ = (char)ENCODED_HEADER_SIZE; + *buffer++ = (char)TAG_HEADER_SIZE; + *buffer++ = (char)tags->ntags; + if (tags->ntags == 0) { + *buffer = (char)tags->ntags; + return ENCODED_HEADER_SIZE; + } + if (buf_size < tags->kvm_used || tags->ntags_alloc > CENSUS_MAX_TAGS) { + *buffer++ = (char)tags->ntags; + size_t enc_size = tag_set_encode_packed(tags, buffer, buf_size); + if (enc_size == 0) { + return 0; + } + return ENCODED_HEADER_SIZE + enc_size; + } + *buffer++ = (char)tags->ntags_alloc; + memcpy(buffer, tags->kvm, tags->kvm_used); + return ENCODED_HEADER_SIZE + tags->kvm_used; +} + +size_t census_tag_set_encode_propagated(const census_tag_set *tags, + char *buffer, size_t buf_size) { + return tag_set_encode(&tags->propagated_tags, buffer, buf_size); +} + +size_t census_tag_set_encode_propagated_binary(const census_tag_set *tags, + char *buffer, size_t buf_size) { + return tag_set_encode(&tags->propagated_binary_tags, buffer, buf_size); +} + +// Decode a tag set. +static void tag_set_decode(struct tag_set *tags, const char *buffer, + size_t size) { + uint8_t version = (uint8_t)(*buffer++); + uint8_t header_size = (uint8_t)(*buffer++); + uint8_t tag_header_size = (uint8_t)(*buffer++); + tags->ntags = (int)(*buffer++); + if (tags->ntags == 0) { + tags->ntags_alloc = 0; + tags->kvm_size = 0; + tags->kvm_used = 0; + tags->kvm = NULL; + return; + } + tags->ntags_alloc = (uint8_t)(*buffer++); + if (header_size != ENCODED_HEADER_SIZE) { + GPR_ASSERT(version != ENCODED_VERSION); + GPR_ASSERT(ENCODED_HEADER_SIZE < header_size); + buffer += (header_size - ENCODED_HEADER_SIZE); + } + tags->kvm_used = size - header_size; + tags->kvm_size = tags->kvm_used + CENSUS_MAX_TAG_KV_LEN; + tags->kvm = gpr_malloc(tags->kvm_size); + if (tag_header_size != TAG_HEADER_SIZE) { + // something new in the tag information. I don't understand it, so + // don't copy it over. + GPR_ASSERT(version != ENCODED_VERSION); + GPR_ASSERT(tag_header_size > TAG_HEADER_SIZE); + char *kvp = tags->kvm; + for (int i = 0; i < tags->ntags_alloc; i++) { + memcpy(kvp, buffer, TAG_HEADER_SIZE); + kvp += header_size; + struct raw_tag raw; + buffer = + decode_tag(&raw, (char *)buffer, tag_header_size - TAG_HEADER_SIZE); + memcpy(kvp, raw.key, (size_t)raw.key_len + raw.value_len); + kvp += raw.key_len + raw.value_len; + } + } else { + memcpy(tags->kvm, buffer, tags->kvm_used); + } +} + +census_tag_set *census_tag_set_decode(const char *buffer, size_t size, + const char *bin_buffer, size_t bin_size) { + census_tag_set *new_ts = gpr_malloc(sizeof(census_tag_set)); + memset(&new_ts->local_tags, 0, sizeof(struct tag_set)); + if (buffer == NULL) { + memset(&new_ts->propagated_tags, 0, sizeof(struct tag_set)); + } else { + tag_set_decode(&new_ts->propagated_tags, buffer, size); + } + if (bin_buffer == NULL) { + memset(&new_ts->propagated_binary_tags, 0, sizeof(struct tag_set)); + } else { + tag_set_decode(&new_ts->propagated_binary_tags, bin_buffer, bin_size); + } + // TODO(aveitch): check that BINARY flag is correct for each type. + return new_ts; +} -- cgit v1.2.3 From 0f690721bdc977b5d506e575f5e13be6d09c57fd Mon Sep 17 00:00:00 2001 From: Alistair Veitch Date: Wed, 13 Jan 2016 09:08:38 -0800 Subject: Move encode/decode API into census.h --- BUILD | 3 -- Makefile | 1 + build.yaml | 1 - gRPC.podspec | 4 +- include/grpc/census.h | 20 +++++++- src/core/census/tag_set.c | 6 +-- src/core/census/tag_set.h | 57 ---------------------- src/python/grpcio/grpc_core_dependencies.py | 1 + test/core/census/tag_set_test.c | 2 +- tools/doxygen/Doxyfile.core.internal | 1 - tools/run_tests/sources_and_headers.json | 4 -- vsprojects/vcxproj/grpc/grpc.vcxproj | 1 - vsprojects/vcxproj/grpc/grpc.vcxproj.filters | 3 -- .../vcxproj/grpc_unsecure/grpc_unsecure.vcxproj | 1 - .../grpc_unsecure/grpc_unsecure.vcxproj.filters | 3 -- 15 files changed, 23 insertions(+), 85 deletions(-) delete mode 100644 src/core/census/tag_set.h (limited to 'src/core/census/tag_set.c') diff --git a/BUILD b/BUILD index ae1a69073f..43e7ba3e18 100644 --- a/BUILD +++ b/BUILD @@ -268,7 +268,6 @@ cc_library( "src/core/census/aggregation.h", "src/core/census/context.h", "src/core/census/rpc_metric_id.h", - "src/core/census/tag_set.h", "src/core/httpcli/httpcli_security_connector.c", "src/core/security/base64.c", "src/core/security/client_auth_filter.c", @@ -561,7 +560,6 @@ cc_library( "src/core/census/aggregation.h", "src/core/census/context.h", "src/core/census/rpc_metric_id.h", - "src/core/census/tag_set.h", "src/core/surface/init_unsecure.c", "src/core/census/grpc_context.c", "src/core/census/grpc_filter.c", @@ -1367,7 +1365,6 @@ objc_library( "src/core/census/aggregation.h", "src/core/census/context.h", "src/core/census/rpc_metric_id.h", - "src/core/census/tag_set.h", ], includes = [ "include", diff --git a/Makefile b/Makefile index c359ca0375..a1b08b414b 100644 --- a/Makefile +++ b/Makefile @@ -8303,6 +8303,7 @@ $(BINDIR)/$(CONFIG)/tag_set_test: $(TAG_SET_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgr endif $(OBJDIR)/$(CONFIG)/test/core/census/tag_set_test.o: $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a + deps_tag_set_test: $(TAG_SET_TEST_OBJS:.o=.dep) ifneq ($(NO_SECURE),true) diff --git a/build.yaml b/build.yaml index 1177564f17..02bab57859 100644 --- a/build.yaml +++ b/build.yaml @@ -16,7 +16,6 @@ filegroups: - src/core/census/aggregation.h - src/core/census/context.h - src/core/census/rpc_metric_id.h - - src/core/census/tag_set.h src: - src/core/census/context.c - src/core/census/initialize.c diff --git a/gRPC.podspec b/gRPC.podspec index 4acf2d7e1b..cde97ac661 100644 --- a/gRPC.podspec +++ b/gRPC.podspec @@ -272,7 +272,6 @@ Pod::Spec.new do |s| 'src/core/census/aggregation.h', 'src/core/census/context.h', 'src/core/census/rpc_metric_id.h', - 'src/core/census/tag_set.h', 'include/grpc/grpc_security.h', 'include/grpc/byte_buffer.h', 'include/grpc/byte_buffer_reader.h', @@ -571,8 +570,7 @@ Pod::Spec.new do |s| 'src/core/transport/transport_impl.h', 'src/core/census/aggregation.h', 'src/core/census/context.h', - 'src/core/census/rpc_metric_id.h', - 'src/core/census/tag_set.h' + 'src/core/census/rpc_metric_id.h' ss.header_mappings_dir = '.' # This isn't officially supported in Cocoapods. We've asked for an alternative: diff --git a/include/grpc/census.h b/include/grpc/census.h index 908bcc96e2..2a62f4daca 100644 --- a/include/grpc/census.h +++ b/include/grpc/census.h @@ -1,6 +1,6 @@ /* * - * Copyright 2015, Google Inc. + * Copyright 2015-2016, Google Inc. * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -339,7 +339,7 @@ typedef struct { const char *key; const char *value; size_t value_len; - gpr_uint8 flags; + uint8_t flags; } census_tag; /* Tag flags. */ @@ -386,6 +386,22 @@ int census_tag_set_get_tag_by_index(const census_tag_set *tags, int index, int census_tag_set_get_tag_by_key(const census_tag_set *tags, const char *key, census_tag *tag); +/* Encode to-be-propagated non-binary tags from a tag set into a memory + buffer. The total number of bytes used in the buffer is returned. If the + buffer is too small to contain the encoded tag set, then 0 is returned. */ +size_t census_tag_set_encode_propagated(const census_tag_set *tags, + char *buffer, size_t buf_size); + +/* Encode to-be-propagated binary tags from a tag set into a memory + buffer. The total number of bytes used in the buffer is returned. If the + buffer is too small to contain the encoded tag set, then 0 is returned. */ +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_*(). */ +census_tag_set *census_tag_set_decode(const char *buffer, size_t size, + 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 80c209031f..fcfe67afad 100644 --- a/src/core/census/tag_set.c +++ b/src/core/census/tag_set.c @@ -1,6 +1,6 @@ /* * - * Copyright 2015, Google Inc. + * Copyright 2015-2016, Google Inc. * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -31,17 +31,13 @@ * */ -#include "tag_set.h" - #include #include #include #include -#include #include #include #include -#include "src/core/support/murmur_hash.h" #include "src/core/support/string.h" // Functions in this file support the public tag_set API, as well as diff --git a/src/core/census/tag_set.h b/src/core/census/tag_set.h deleted file mode 100644 index 9eec0ad438..0000000000 --- a/src/core/census/tag_set.h +++ /dev/null @@ -1,57 +0,0 @@ -/* - * - * Copyright 2015, Google Inc. - * All rights reserved. - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions are - * met: - * - * * Redistributions of source code must retain the above copyright - * notice, this list of conditions and the following disclaimer. - * * Redistributions in binary form must reproduce the above - * copyright notice, this list of conditions and the following disclaimer - * in the documentation and/or other materials provided with the - * distribution. - * * Neither the name of Google Inc. nor the names of its - * contributors may be used to endorse or promote products derived from - * this software without specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS - * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT - * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR - * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT - * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, - * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT - * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, - * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY - * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE - * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - * - */ - -#ifndef GRPC_INTERNAL_CORE_CENSUS_TAG_SET_H -#define GRPC_INTERNAL_CORE_CENSUS_TAG_SET_H - -#include -#include -#include - -/* Encode to-be-propagated tags from a tag set into a memory buffer. The total - number of bytes used in the buffer is returned. If the buffer is too small - to contain the encoded tag set, then 0 is returned. */ -size_t census_tag_set_encode_propagated(const census_tag_set *tags, - char *buffer, size_t buf_size); - -/* Encode to-be-propagated binary tags from a tag set into a memory - buffer. The total number of bytes used in the buffer is returned. If the - buffer is too small to contain the encoded tag set, then 0 is returned. */ -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_*(). */ -census_tag_set *census_tag_set_decode(const char *buffer, size_t size, - const char *bin_buffer, size_t bin_size); - -#endif /* GRPC_INTERNAL_CORE_CENSUS_TAG_SET_H */ diff --git a/src/python/grpcio/grpc_core_dependencies.py b/src/python/grpcio/grpc_core_dependencies.py index 63cf0a4c74..f5b8b897f9 100644 --- a/src/python/grpcio/grpc_core_dependencies.py +++ b/src/python/grpcio/grpc_core_dependencies.py @@ -220,6 +220,7 @@ CORE_SOURCE_FILES = [ 'src/core/census/context.c', 'src/core/census/initialize.c', 'src/core/census/operation.c', + 'src/core/census/tag_set.c', 'src/core/census/tracing.c', 'src/boringssl/err_data.c', 'third_party/boringssl/crypto/aes/aes.c', diff --git a/test/core/census/tag_set_test.c b/test/core/census/tag_set_test.c index d6a7e45334..44ecb2819c 100644 --- a/test/core/census/tag_set_test.c +++ b/test/core/census/tag_set_test.c @@ -33,7 +33,7 @@ // Test census_tag_set functions, including encoding/decoding -#include "src/core/census/tag_set.h" +#include #include #include #include diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index ac57dd03b7..222ad3c6a2 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -897,7 +897,6 @@ src/core/transport/transport_impl.h \ src/core/census/aggregation.h \ src/core/census/context.h \ src/core/census/rpc_metric_id.h \ -src/core/census/tag_set.h \ src/core/httpcli/httpcli_security_connector.c \ src/core/security/base64.c \ src/core/security/client_auth_filter.c \ diff --git a/tools/run_tests/sources_and_headers.json b/tools/run_tests/sources_and_headers.json index 5e474a8b7b..4067863f89 100644 --- a/tools/run_tests/sources_and_headers.json +++ b/tools/run_tests/sources_and_headers.json @@ -2874,7 +2874,6 @@ "src/core/census/context.h", "src/core/census/grpc_filter.h", "src/core/census/rpc_metric_id.h", - "src/core/census/tag_set.h", "src/core/channel/channel_args.h", "src/core/channel/channel_stack.h", "src/core/channel/client_channel.h", @@ -3022,7 +3021,6 @@ "src/core/census/operation.c", "src/core/census/rpc_metric_id.h", "src/core/census/tag_set.c", - "src/core/census/tag_set.h", "src/core/census/tracing.c", "src/core/channel/channel_args.c", "src/core/channel/channel_args.h", @@ -3389,7 +3387,6 @@ "src/core/census/context.h", "src/core/census/grpc_filter.h", "src/core/census/rpc_metric_id.h", - "src/core/census/tag_set.h", "src/core/channel/channel_args.h", "src/core/channel/channel_stack.h", "src/core/channel/client_channel.h", @@ -3522,7 +3519,6 @@ "src/core/census/operation.c", "src/core/census/rpc_metric_id.h", "src/core/census/tag_set.c", - "src/core/census/tag_set.h", "src/core/census/tracing.c", "src/core/channel/channel_args.c", "src/core/channel/channel_args.h", diff --git a/vsprojects/vcxproj/grpc/grpc.vcxproj b/vsprojects/vcxproj/grpc/grpc.vcxproj index 037b8ff222..b143de9d86 100644 --- a/vsprojects/vcxproj/grpc/grpc.vcxproj +++ b/vsprojects/vcxproj/grpc/grpc.vcxproj @@ -404,7 +404,6 @@ - diff --git a/vsprojects/vcxproj/grpc/grpc.vcxproj.filters b/vsprojects/vcxproj/grpc/grpc.vcxproj.filters index 0ddbd5e46d..93848293c6 100644 --- a/vsprojects/vcxproj/grpc/grpc.vcxproj.filters +++ b/vsprojects/vcxproj/grpc/grpc.vcxproj.filters @@ -869,9 +869,6 @@ src\core\census - - src\core\census - diff --git a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj index a3532e5d05..dae8edf17f 100644 --- a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj +++ b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj @@ -382,7 +382,6 @@ - diff --git a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters index f63e3012e7..1f850b2629 100644 --- a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters +++ b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters @@ -764,9 +764,6 @@ src\core\census - - src\core\census - -- cgit v1.2.3 From a237796233b601e7a5bf1af79c68b1cc269a6f87 Mon Sep 17 00:00:00 2001 From: Alistair Veitch Date: Wed, 13 Jan 2016 11:09:16 -0800 Subject: Always compress tags --- src/core/census/tag_set.c | 218 +++++++++++++++++----------------------- test/core/census/tag_set_test.c | 13 --- 2 files changed, 93 insertions(+), 138 deletions(-) (limited to 'src/core/census/tag_set.c') diff --git a/src/core/census/tag_set.c b/src/core/census/tag_set.c index fcfe67afad..e879ac0606 100644 --- a/src/core/census/tag_set.c +++ b/src/core/census/tag_set.c @@ -59,13 +59,12 @@ // to handle endian-ness conversions. // * Keep all tag information (keys/values/flags) in a single memory buffer, // that can be directly copied to the wire. This makes iteration by index -// somewhat less efficient. +// somewhat less efficient. If it becomes a problem, we could consider +// building an index at tag_set creation. // * Binary tags are encoded seperately from non-binary tags. There are a // primarily because non-binary tags are far more likely to be repeated // across multiple RPC calls, so are more efficiently cached and // compressed in any metadata schemes. -// * deleted/modified tags are kept in memory, just marked with a deleted -// flag. This enables faster processing TODO: benchmark this // * all lengths etc. are restricted to one byte. This eliminates endian // issues. @@ -74,7 +73,8 @@ struct tag_set { int ntags; // number of tags. int ntags_alloc; // ntags + number of deleted tags (total number of tags - // in all of kvm). + // in all of kvm). This will always be == ntags, except + // during the process of building a new tag set. size_t kvm_size; // number of bytes allocated for key/value storage. size_t kvm_used; // number of bytes of used key/value memory char *kvm; // key/value memory. Consists of repeated entries of: @@ -138,28 +138,17 @@ static void tag_set_copy(struct tag_set *to, const struct tag_set *from) { memcpy(to->kvm, from->kvm, to->kvm_used); } -// We may want to keep information about a deleted tag for a short time, -// in case we can reuse the space (same tag is reinserted). This structure -// is used for that purpose. -struct deleted_tag_info { - struct raw_tag raw; // raw tag information. - uint8_t *raw_flags_p; // pointer to original flags - struct tag_set *tags; // the tag set from which tag was deleted. -}; - -// Delete a tag from a tag set, if it exists. Returns true if the tag was -// originally present (and is now deleted), false if it wasn't. -static bool tag_set_delete_tag(struct tag_set *tags, - struct deleted_tag_info *dtag, const char *key, +// Delete a tag from a tag set, if it exists (returns true it it did). +static bool tag_set_delete_tag(struct tag_set *tags, const char *key, size_t key_len) { char *kvp = tags->kvm; for (int i = 0; i < tags->ntags_alloc; i++) { - dtag->raw_flags_p = (uint8_t *)(kvp + FLAG_OFFSET); - kvp = decode_tag(&dtag->raw, kvp, 0); - if (CENSUS_TAG_IS_DELETED(dtag->raw.flags)) continue; - if ((key_len == dtag->raw.key_len) && - (memcmp(key, dtag->raw.key, key_len) == 0)) { - *(dtag->raw_flags_p) |= CENSUS_TAG_DELETED; + uint8_t *flags = (uint8_t *)(kvp + FLAG_OFFSET); + struct raw_tag tag; + kvp = decode_tag(&tag, kvp, 0); + if (CENSUS_TAG_IS_DELETED(tag.flags)) continue; + if ((key_len == tag.key_len) && (memcmp(key, tag.key, key_len) == 0)) { + *flags |= CENSUS_TAG_DELETED; tags->ntags--; return true; } @@ -167,46 +156,34 @@ static bool tag_set_delete_tag(struct tag_set *tags, return false; } -// Delete a tag from a tag set. If the tag is found in any of the underlying -// tag sets, *and* that tag set corresponds to the one in which the tag (if -// later inserted) would be placed, then fills in dtag, and returns true. -// Returns false otherwise. This information is later used to optimize the -// placement of the tag if the value space can be reused, effectively -// "undeleting" the tag. -static bool cts_delete_tag(census_tag_set *tags, const census_tag *tag, - size_t key_len, struct deleted_tag_info *dtag) { +// Delete a tag from a tag set. +static void cts_delete_tag(census_tag_set *tags, const census_tag *tag, + size_t key_len) { // use the to-be-deleted tag flags as a hint to determine the order in which // we delete from the underlying tag sets. if (CENSUS_TAG_IS_PROPAGATED(tag->flags)) { if (CENSUS_TAG_IS_BINARY(tag->flags)) { - if (tag_set_delete_tag(&tags->propagated_binary_tags, dtag, tag->key, + if (tag_set_delete_tag(&tags->propagated_binary_tags, tag->key, key_len)) { - dtag->tags = &tags->propagated_binary_tags; - return true; + return; } - if (tag_set_delete_tag(&tags->propagated_tags, dtag, tag->key, key_len)) - return false; - tag_set_delete_tag(&tags->local_tags, dtag, tag->key, key_len); + if (tag_set_delete_tag(&tags->propagated_tags, tag->key, key_len)) return; + tag_set_delete_tag(&tags->local_tags, tag->key, key_len); } else { - if (tag_set_delete_tag(&tags->propagated_tags, dtag, tag->key, key_len)) { - dtag->tags = &tags->propagated_tags; - return true; + if (tag_set_delete_tag(&tags->propagated_tags, tag->key, key_len)) { + return; } - if (tag_set_delete_tag(&tags->propagated_binary_tags, dtag, tag->key, - key_len)) - return false; - tag_set_delete_tag(&tags->local_tags, dtag, tag->key, key_len); + if (tag_set_delete_tag(&tags->propagated_binary_tags, tag->key, key_len)) + return; + tag_set_delete_tag(&tags->local_tags, tag->key, key_len); } } else { - if (tag_set_delete_tag(&tags->local_tags, dtag, tag->key, key_len)) { - dtag->tags = &tags->local_tags; - return true; + if (tag_set_delete_tag(&tags->local_tags, tag->key, key_len)) { + return; } - if (tag_set_delete_tag(&tags->propagated_tags, dtag, tag->key, key_len)) - return false; - tag_set_delete_tag(&tags->propagated_binary_tags, dtag, tag->key, key_len); + if (tag_set_delete_tag(&tags->propagated_tags, tag->key, key_len)) return; + tag_set_delete_tag(&tags->propagated_binary_tags, tag->key, key_len); } - return false; } // Add a tag to a tag set. @@ -218,6 +195,7 @@ static void tag_set_add_tag(struct tag_set *tags, const census_tag *tag, return; } if (tags->kvm_used + tag_size > tags->kvm_size) { + // allocate new memory if needed tags->kvm_size += 2 * CENSUS_MAX_TAG_KV_LEN + TAG_HEADER_SIZE; char *new_kvm = gpr_malloc(tags->kvm_size); memcpy(new_kvm, tags->kvm, tags->kvm_used); @@ -242,28 +220,60 @@ static void tag_set_add_tag(struct tag_set *tags, const census_tag *tag, static void cts_add_tag(census_tag_set *tags, const census_tag *tag, size_t key_len) { // first delete the tag if it is already present - struct deleted_tag_info dtag; - bool deleted_match = cts_delete_tag(tags, tag, key_len, &dtag); + cts_delete_tag(tags, tag, key_len); if (tag->value != NULL && tag->value_len != 0) { - if (deleted_match && tag->value_len == dtag.raw.value_len) { - // if we have a close match for tag being added to one just deleted, - // only need to modify value and flags. - memcpy(dtag.raw.value, tag->value, tag->value_len); - *dtag.raw_flags_p = (tag->flags & (CENSUS_TAG_PROPAGATE | - CENSUS_TAG_STATS | CENSUS_TAG_BINARY)); - dtag.tags->ntags++; - } else { - if (CENSUS_TAG_IS_PROPAGATED(tag->flags)) { - if (CENSUS_TAG_IS_BINARY(tag->flags)) { - tag_set_add_tag(&tags->propagated_binary_tags, tag, key_len); - } else { - tag_set_add_tag(&tags->propagated_tags, tag, key_len); - } + if (CENSUS_TAG_IS_PROPAGATED(tag->flags)) { + if (CENSUS_TAG_IS_BINARY(tag->flags)) { + tag_set_add_tag(&tags->propagated_binary_tags, tag, key_len); } else { - tag_set_add_tag(&tags->local_tags, tag, key_len); + tag_set_add_tag(&tags->propagated_tags, tag, key_len); + } + } else { + tag_set_add_tag(&tags->local_tags, tag, key_len); + } + } +} + +// Remove any deleted tags from the tag set. Basic algorithm: +// 1) Walk through tag set to find first deleted tag. Record where it is. +// 2) Find the next not-deleted tag. Copy all of kvm from there to the end +// "over" the deleted tags +// 3) repeat #1 and #2 until we have seen all tags +// 4) if we are still looking for a not-deleted tag, then all the end portion +// of the kvm is deleted. Just reduce the used amount of memory by the +// appropriate amount. +static void tag_set_compress(struct tag_set *tags) { + if (tags->ntags == tags->ntags_alloc) return; + bool find_deleted = true; // are we looking for deleted tags? + char *kvp = tags->kvm; + char *dbase; // record location of deleted tag + for (int i = 0; i < tags->ntags_alloc; i++) { + struct raw_tag tag; + char *next_kvp = decode_tag(&tag, kvp, 0); + if (find_deleted) { + if (CENSUS_TAG_IS_DELETED(tag.flags)) { + dbase = kvp; + find_deleted = false; + } + } else { + if (!CENSUS_TAG_IS_DELETED(tag.flags)) { + ptrdiff_t reduce = kvp - dbase; // #bytes in deleted tags + GPR_ASSERT(reduce > 0); + ptrdiff_t copy_size = tags->kvm + tags->kvm_used - kvp; + GPR_ASSERT(copy_size > 0); + memmove(dbase, kvp, (size_t)copy_size); + tags->kvm_used -= (size_t)reduce; + next_kvp -= reduce; + find_deleted = true; } } + kvp = next_kvp; } + if (!find_deleted) { + GPR_ASSERT(dbase > tags->kvm); + tags->kvm_used = (size_t)(dbase - tags->kvm); + } + tags->ntags_alloc = tags->ntags; } census_tag_set *census_tag_set_create(const census_tag_set *base, @@ -286,6 +296,9 @@ census_tag_set *census_tag_set_create(const census_tag_set *base, cts_add_tag(new_ts, tag, key_len); } } + tag_set_compress(&new_ts->propagated_tags); + tag_set_compress(&new_ts->propagated_binary_tags); + tag_set_compress(&new_ts->local_tags); return new_ts; } @@ -307,21 +320,15 @@ static void tag_set_get_tag_by_index(const struct tag_set *tags, int index, census_tag *tag) { GPR_ASSERT(index < tags->ntags); char *kvp = tags->kvm; - for (;;) { - struct raw_tag raw; + struct raw_tag raw; + kvp = decode_tag(&raw, kvp, 0); + for (int i = 0; i < index; i++) { kvp = decode_tag(&raw, kvp, 0); - if (CENSUS_TAG_IS_DELETED(raw.flags)) { - continue; - } else if (index == 0) { - tag->key = raw.key; - tag->value = raw.value; - tag->value_len = raw.value_len; - tag->flags = raw.flags; - return; - } - index--; } - // NOT REACHED + tag->key = raw.key; + tag->value = raw.value; + tag->value_len = raw.value_len; + tag->flags = raw.flags; } int census_tag_set_get_tag_by_index(const census_tag_set *tags, int index, @@ -350,9 +357,7 @@ static bool tag_set_get_tag_by_key(const struct tag_set *tags, const char *key, char *kvp = tags->kvm; for (int i = 0; i < tags->ntags; i++) { struct raw_tag raw; - do { - kvp = decode_tag(&raw, kvp, 0); - } while (CENSUS_TAG_IS_DELETED(raw.flags)); + kvp = decode_tag(&raw, kvp, 0); if (key_len == raw.key_len && memcmp(raw.key, key, key_len) == 0) { tag->key = raw.key; tag->value = raw.value; @@ -391,42 +396,16 @@ int census_tag_set_get_tag_by_key(const census_tag_set *tags, const char *key, // expansion. // 2 1 number of bytes in each tag header. // 3 1 ntags value from tag set. -// 4 1 ntags_alloc value from tag set. // // This is followed by the key/value memory from struct tag_set. #define ENCODED_VERSION 0 // Version number -#define ENCODED_HEADER_SIZE 5 // size of tag set header - -// Pack a tag set into as few bytes as possible (eliding deleted tags). Assumes -// header is already generated. -static size_t tag_set_encode_packed(const struct tag_set *tags, char *buffer, - size_t buf_size) { - size_t encoded_size = 0; - char *kvp = tags->kvm; - for (int i = 0; i < tags->ntags_alloc; i++) { - struct raw_tag raw; - char *base = kvp; - kvp = decode_tag(&raw, kvp, 0); - size_t tag_size = - TAG_HEADER_SIZE + (size_t)raw.key_len + (size_t)raw.value_len; - if (!(CENSUS_TAG_IS_DELETED(raw.flags))) { - if (tag_size > buf_size) { - return 0; - } - memcpy(buffer, base, tag_size); - buffer += tag_size; - encoded_size += tag_size; - buf_size -= tag_size; - } - } - return encoded_size; -} +#define ENCODED_HEADER_SIZE 4 // size of tag set header // Encode a tag set. Returns 0 if buffer is too small. static size_t tag_set_encode(const struct tag_set *tags, char *buffer, size_t buf_size) { - if (buf_size < ENCODED_HEADER_SIZE) { + if (buf_size < ENCODED_HEADER_SIZE + tags->kvm_used) { return 0; } buf_size -= ENCODED_HEADER_SIZE; @@ -435,18 +414,8 @@ static size_t tag_set_encode(const struct tag_set *tags, char *buffer, *buffer++ = (char)TAG_HEADER_SIZE; *buffer++ = (char)tags->ntags; if (tags->ntags == 0) { - *buffer = (char)tags->ntags; return ENCODED_HEADER_SIZE; } - if (buf_size < tags->kvm_used || tags->ntags_alloc > CENSUS_MAX_TAGS) { - *buffer++ = (char)tags->ntags; - size_t enc_size = tag_set_encode_packed(tags, buffer, buf_size); - if (enc_size == 0) { - return 0; - } - return ENCODED_HEADER_SIZE + enc_size; - } - *buffer++ = (char)tags->ntags_alloc; memcpy(buffer, tags->kvm, tags->kvm_used); return ENCODED_HEADER_SIZE + tags->kvm_used; } @@ -467,7 +436,7 @@ static void tag_set_decode(struct tag_set *tags, const char *buffer, uint8_t version = (uint8_t)(*buffer++); uint8_t header_size = (uint8_t)(*buffer++); uint8_t tag_header_size = (uint8_t)(*buffer++); - tags->ntags = (int)(*buffer++); + tags->ntags = tags->ntags_alloc = (int)(*buffer++); if (tags->ntags == 0) { tags->ntags_alloc = 0; tags->kvm_size = 0; @@ -475,7 +444,6 @@ static void tag_set_decode(struct tag_set *tags, const char *buffer, tags->kvm = NULL; return; } - tags->ntags_alloc = (uint8_t)(*buffer++); if (header_size != ENCODED_HEADER_SIZE) { GPR_ASSERT(version != ENCODED_VERSION); GPR_ASSERT(ENCODED_HEADER_SIZE < header_size); @@ -490,7 +458,7 @@ static void tag_set_decode(struct tag_set *tags, const char *buffer, GPR_ASSERT(version != ENCODED_VERSION); GPR_ASSERT(tag_header_size > TAG_HEADER_SIZE); char *kvp = tags->kvm; - for (int i = 0; i < tags->ntags_alloc; i++) { + for (int i = 0; i < tags->ntags; i++) { memcpy(kvp, buffer, TAG_HEADER_SIZE); kvp += header_size; struct raw_tag raw; diff --git a/test/core/census/tag_set_test.c b/test/core/census/tag_set_test.c index 44ecb2819c..4c286b3e4f 100644 --- a/test/core/census/tag_set_test.c +++ b/test/core/census/tag_set_test.c @@ -367,19 +367,6 @@ static void complex_encode_decode_test(void) { GPR_ASSERT(census_tag_set_ntags(cts3) == 2); GPR_ASSERT(validate_tag(cts3, &basic_tags[4])); GPR_ASSERT(validate_tag(cts3, &modify_tags[8])); - // Now force tag set to be in smaller space - census_tag_set_destroy(cts3); - size_t nb1 = census_tag_set_encode_propagated(cts2, buf1, b1 - 1); - GPR_ASSERT(nb1 != 0); - GPR_ASSERT(nb1 < b1); - size_t nb2 = census_tag_set_encode_propagated_binary(cts2, buf2, b2 - 1); - GPR_ASSERT(nb2 != 0); - GPR_ASSERT(nb2 < b2); - cts3 = census_tag_set_decode(buf1, nb1, buf2, nb2); - GPR_ASSERT(cts3 != NULL); - GPR_ASSERT(census_tag_set_ntags(cts3) == 2); - GPR_ASSERT(validate_tag(cts3, &basic_tags[4])); - GPR_ASSERT(validate_tag(cts3, &modify_tags[8])); census_tag_set_destroy(cts3); census_tag_set_destroy(cts2); census_tag_set_destroy(cts); -- cgit v1.2.3 From fc999adcc7570ef19e16bf580199babb98096d4c Mon Sep 17 00:00:00 2001 From: Alistair Veitch Date: Thu, 14 Jan 2016 17:05:46 -0800 Subject: new iterator interface --- include/grpc/census.h | 45 ++++++++-- src/core/census/tag_set.c | 187 +++++++++++++++++++++------------------- test/core/census/tag_set_test.c | 77 +++++++---------- 3 files changed, 167 insertions(+), 142 deletions(-) (limited to 'src/core/census/tag_set.c') diff --git a/include/grpc/census.h b/include/grpc/census.h index 2a62f4daca..1d7e65cdd9 100644 --- a/include/grpc/census.h +++ b/include/grpc/census.h @@ -354,8 +354,21 @@ typedef struct { #define CENSUS_TAG_IS_STATS(flags) (flags & CENSUS_TAG_STATS) #define CENSUS_TAG_IS_BINARY(flags) (flags & CENSUS_TAG_BINARY) -#define CENSUS_MAX_TAG_KV_LEN 255 /* Maximum length of key/value in a tag. */ -#define CENSUS_MAX_TAGS 255 /* Maximum number of tags in a tag set. */ +/* Maximum length of key/value in a tag. */ +#define CENSUS_MAX_TAG_KV_LEN 255 +/* Maximum number of propagatable tags. */ +#define CENSUS_MAX_PROPAGATED_TAGS 255 + +typedef struct { + int n_propagated_tags; /* number of propagated printable tags */ + int n_propagated_binary_tags; /* number of propagated binary tags */ + int n_local_tags; /* number of non-propagated (local) tags */ + int n_deleted_tags; /* number of tags that were deleted */ + int n_added_tags; /* number of tags that were added */ + int n_modified_tags; /* number of tags that were modified */ + int n_invalid_tags; /* number of tags with bad keys or values (e.g. + longer than CENSUS_MAX_TAG_KV_LEN) */ +} census_tag_set_create_stats; /* Create a new tag set, adding and removing tags from an existing tag set. @param base Base tag set to build upon. Can be NULL. @@ -365,9 +378,12 @@ typedef struct { both, but with NULL or zero-length values, will be deleted from the tag set. @param ntags number of tags in 'tags' + @param stats Information about the tag set created and actions taken during + its creation. */ census_tag_set *census_tag_set_create(const census_tag_set *base, - const census_tag *tags, int ntags); + const census_tag *tags, int ntags, + census_tag_set_create_stats *stats); /* Destroy a tag set created by census_tag_set_create(). Once this function has been called, the tag set cannot be reused. */ @@ -376,10 +392,25 @@ void census_tag_set_destroy(census_tag_set *tags); /* Get the number of tags in the tag set. */ int census_tag_set_ntags(const census_tag_set *tags); -/* Get a tag by it's index in the tag set. Returns 0 if the index is invalid - (<0 or >= census_tag_set_ntags). There is no guarantee on tag ordering. */ -int census_tag_set_get_tag_by_index(const census_tag_set *tags, int index, - census_tag *tag); +/* Structure used for tag set iteration. API clients should not use or + reference internal fields - neither their contents or presence/absence are + guaranteed. */ +typedef struct { + const census_tag_set *tags; + int base; + int index; + char *kvm; +} census_tag_set_iterator; + +/* 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, + census_tag_set_iterator *iterator); + +/* Get the contents of the "next" tag in the tag set. If there are no more + tags in the tag set, returns 0 (and 'tag' contents will be unchanged), + otherwise returns 1. */ +int census_tag_set_next_tag(census_tag_set_iterator *iterator, census_tag *tag); /* Get a tag by its key. Returns 0 if the key is not present in the tag set. */ diff --git a/src/core/census/tag_set.c b/src/core/census/tag_set.c index e879ac0606..3b8efb1935 100644 --- a/src/core/census/tag_set.c +++ b/src/core/census/tag_set.c @@ -30,6 +30,13 @@ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. * */ +/* +- ability to add extra tags in encode? +- add drops error count to create_ts +- add mask to ntags? +- comment about key/value ptrs being to mem +- add comment about encode/decode being for RPC use only. +*/ #include #include @@ -112,11 +119,14 @@ struct raw_tag { // tag_set structs, one for each of the binary/printable propagated tags, and // one for everything else. struct census_tag_set { - struct tag_set propagated_tags; - struct tag_set propagated_binary_tags; - struct tag_set local_tags; + struct tag_set tags[3]; }; +// Indices into the tags member of census_tag_set +#define PROPAGATED_TAGS 0 +#define PROPAGATED_BINARY_TAGS 1 +#define LOCAL_TAGS 2 + // Extract a raw tag given a pointer (raw) to the tag header. Allow for some // extra bytes in the tag header (see encode/decode for usage: allows for // future expansion of the tag header). @@ -156,44 +166,19 @@ static bool tag_set_delete_tag(struct tag_set *tags, const char *key, return false; } -// Delete a tag from a tag set. -static void cts_delete_tag(census_tag_set *tags, const census_tag *tag, +// Delete a tag from a tag set, return true if it existed. +static bool cts_delete_tag(census_tag_set *tags, const census_tag *tag, size_t key_len) { - // use the to-be-deleted tag flags as a hint to determine the order in which - // we delete from the underlying tag sets. - if (CENSUS_TAG_IS_PROPAGATED(tag->flags)) { - if (CENSUS_TAG_IS_BINARY(tag->flags)) { - if (tag_set_delete_tag(&tags->propagated_binary_tags, tag->key, - key_len)) { - return; - } - if (tag_set_delete_tag(&tags->propagated_tags, tag->key, key_len)) return; - tag_set_delete_tag(&tags->local_tags, tag->key, key_len); - } else { - if (tag_set_delete_tag(&tags->propagated_tags, tag->key, key_len)) { - return; - } - if (tag_set_delete_tag(&tags->propagated_binary_tags, tag->key, key_len)) - return; - tag_set_delete_tag(&tags->local_tags, tag->key, key_len); - } - } else { - if (tag_set_delete_tag(&tags->local_tags, tag->key, key_len)) { - return; - } - if (tag_set_delete_tag(&tags->propagated_tags, tag->key, key_len)) return; - tag_set_delete_tag(&tags->propagated_binary_tags, tag->key, key_len); - } + return (tag_set_delete_tag(&tags->tags[LOCAL_TAGS], tag->key, key_len) || + tag_set_delete_tag(&tags->tags[PROPAGATED_TAGS], tag->key, key_len) || + tag_set_delete_tag(&tags->tags[PROPAGATED_BINARY_TAGS], tag->key, + key_len)); } // Add a tag to a tag set. static void tag_set_add_tag(struct tag_set *tags, const census_tag *tag, size_t key_len) { const size_t tag_size = key_len + tag->value_len + TAG_HEADER_SIZE; - // drop tag if too many. - if (tags->ntags == CENSUS_MAX_TAGS) { - return; - } if (tags->kvm_used + tag_size > tags->kvm_size) { // allocate new memory if needed tags->kvm_size += 2 * CENSUS_MAX_TAG_KV_LEN + TAG_HEADER_SIZE; @@ -224,12 +209,12 @@ static void cts_add_tag(census_tag_set *tags, const census_tag *tag, if (tag->value != NULL && tag->value_len != 0) { if (CENSUS_TAG_IS_PROPAGATED(tag->flags)) { if (CENSUS_TAG_IS_BINARY(tag->flags)) { - tag_set_add_tag(&tags->propagated_binary_tags, tag, key_len); + tag_set_add_tag(&tags->tags[PROPAGATED_BINARY_TAGS], tag, key_len); } else { - tag_set_add_tag(&tags->propagated_tags, tag, key_len); + tag_set_add_tag(&tags->tags[PROPAGATED_TAGS], tag, key_len); } } else { - tag_set_add_tag(&tags->local_tags, tag, key_len); + tag_set_add_tag(&tags->tags[LOCAL_TAGS], tag, key_len); } } } @@ -242,7 +227,7 @@ static void cts_add_tag(census_tag_set *tags, const census_tag *tag, // 4) if we are still looking for a not-deleted tag, then all the end portion // of the kvm is deleted. Just reduce the used amount of memory by the // appropriate amount. -static void tag_set_compress(struct tag_set *tags) { +static void tag_set_flatten(struct tag_set *tags) { if (tags->ntags == tags->ntags_alloc) return; bool find_deleted = true; // are we looking for deleted tags? char *kvp = tags->kvm; @@ -277,15 +262,17 @@ static void tag_set_compress(struct tag_set *tags) { } census_tag_set *census_tag_set_create(const census_tag_set *base, - const census_tag *tags, int ntags) { + const census_tag *tags, int ntags, + census_tag_set_create_stats *stats) { + int n_invalid_tags = 0; census_tag_set *new_ts = gpr_malloc(sizeof(census_tag_set)); if (base == NULL) { memset(new_ts, 0, sizeof(census_tag_set)); } else { - tag_set_copy(&new_ts->propagated_tags, &base->propagated_tags); - tag_set_copy(&new_ts->propagated_binary_tags, - &base->propagated_binary_tags); - tag_set_copy(&new_ts->local_tags, &base->local_tags); + tag_set_copy(&new_ts->tags[PROPAGATED_TAGS], &base->tags[PROPAGATED_TAGS]); + 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]); } for (int i = 0; i < ntags; i++) { const census_tag *tag = &tags[i]; @@ -294,61 +281,81 @@ census_tag_set *census_tag_set_create(const census_tag_set *base, if (key_len != 1 && key_len <= CENSUS_MAX_TAG_KV_LEN && tag->value_len <= CENSUS_MAX_TAG_KV_LEN) { cts_add_tag(new_ts, tag, key_len); + } else { + n_invalid_tags++; } } - tag_set_compress(&new_ts->propagated_tags); - tag_set_compress(&new_ts->propagated_binary_tags); - tag_set_compress(&new_ts->local_tags); + 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 (stats != NULL) { + stats->n_propagated_tags = new_ts->tags[PROPAGATED_TAGS].ntags; + stats->n_propagated_binary_tags = + new_ts->tags[PROPAGATED_BINARY_TAGS].ntags; + stats->n_local_tags = new_ts->tags[LOCAL_TAGS].ntags; + stats->n_invalid_tags = n_invalid_tags; + } return new_ts; } void census_tag_set_destroy(census_tag_set *tags) { - gpr_free(tags->propagated_tags.kvm); - gpr_free(tags->propagated_binary_tags.kvm); - gpr_free(tags->local_tags.kvm); + gpr_free(tags->tags[PROPAGATED_TAGS].kvm); + gpr_free(tags->tags[PROPAGATED_BINARY_TAGS].kvm); + gpr_free(tags->tags[LOCAL_TAGS].kvm); gpr_free(tags); } int census_tag_set_ntags(const census_tag_set *tags) { - return tags->propagated_tags.ntags + tags->propagated_binary_tags.ntags + - tags->local_tags.ntags; + return tags->tags[PROPAGATED_TAGS].ntags + + tags->tags[PROPAGATED_BINARY_TAGS].ntags + + tags->tags[LOCAL_TAGS].ntags; } -// Get the nth tag in a tag set. The caller must validate that index is -// in range. -static void tag_set_get_tag_by_index(const struct tag_set *tags, int index, - census_tag *tag) { - GPR_ASSERT(index < tags->ntags); - char *kvp = tags->kvm; - struct raw_tag raw; - kvp = decode_tag(&raw, kvp, 0); - for (int i = 0; i < index; i++) { - kvp = decode_tag(&raw, kvp, 0); +/* 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, + census_tag_set_iterator *iterator) { + iterator->tags = tags; + iterator->index = 0; + if (tags->tags[PROPAGATED_TAGS].ntags != 0) { + iterator->base = PROPAGATED_TAGS; + iterator->kvm = tags->tags[PROPAGATED_TAGS].kvm; + } else if (tags->tags[PROPAGATED_BINARY_TAGS].ntags != 0) { + iterator->base = PROPAGATED_BINARY_TAGS; + iterator->kvm = tags->tags[PROPAGATED_BINARY_TAGS].kvm; + } else if (tags->tags[LOCAL_TAGS].ntags != 0) { + iterator->base = LOCAL_TAGS; + iterator->kvm = tags->tags[LOCAL_TAGS].kvm; + } else { + iterator->base = -1; + } +} + +/* Get the contents of the "next" tag in the tag set. If there are no more + tags in the tag set, returns 0 (and 'tag' contents will be unchanged), + otherwise returns 1. */ +int census_tag_set_next_tag(census_tag_set_iterator *iterator, + census_tag *tag) { + if (iterator->base < 0) { + return 0; } + struct raw_tag raw; + iterator->kvm = decode_tag(&raw, iterator->kvm, 0); tag->key = raw.key; tag->value = raw.value; tag->value_len = raw.value_len; tag->flags = raw.flags; -} - -int census_tag_set_get_tag_by_index(const census_tag_set *tags, int index, - census_tag *tag) { - if (index < 0) return 0; - if (index < tags->propagated_tags.ntags) { - tag_set_get_tag_by_index(&tags->propagated_tags, index, tag); - return 1; - } - index -= tags->propagated_tags.ntags; - if (index < tags->propagated_binary_tags.ntags) { - tag_set_get_tag_by_index(&tags->propagated_binary_tags, index, tag); - return 1; - } - index -= tags->propagated_binary_tags.ntags; - if (index < tags->local_tags.ntags) { - tag_set_get_tag_by_index(&tags->local_tags, index, tag); - return 1; + if (++iterator->index == iterator->tags->tags[iterator->base].ntags) { + do { + if (iterator->base == LOCAL_TAGS) { + iterator->base = -1; + return 1; + } + } while (iterator->tags->tags[++iterator->base].ntags == 0); + iterator->index = 0; + iterator->kvm = iterator->tags->tags[iterator->base].kvm; } - return 0; + return 1; } // Find a tag in a tag_set by key. Return true if found, false otherwise. @@ -375,10 +382,10 @@ int census_tag_set_get_tag_by_key(const census_tag_set *tags, const char *key, if (key_len == 1) { return 0; } - if (tag_set_get_tag_by_key(&tags->propagated_tags, key, key_len, tag) || - tag_set_get_tag_by_key(&tags->propagated_binary_tags, key, key_len, + if (tag_set_get_tag_by_key(&tags->tags[PROPAGATED_TAGS], key, key_len, tag) || + tag_set_get_tag_by_key(&tags->tags[PROPAGATED_BINARY_TAGS], key, key_len, tag) || - tag_set_get_tag_by_key(&tags->local_tags, key, key_len, tag)) { + tag_set_get_tag_by_key(&tags->tags[LOCAL_TAGS], key, key_len, tag)) { return 1; } return 0; @@ -422,12 +429,12 @@ static size_t tag_set_encode(const struct tag_set *tags, char *buffer, size_t census_tag_set_encode_propagated(const census_tag_set *tags, char *buffer, size_t buf_size) { - return tag_set_encode(&tags->propagated_tags, buffer, buf_size); + return tag_set_encode(&tags->tags[PROPAGATED_TAGS], buffer, buf_size); } size_t census_tag_set_encode_propagated_binary(const census_tag_set *tags, char *buffer, size_t buf_size) { - return tag_set_encode(&tags->propagated_binary_tags, buffer, buf_size); + return tag_set_encode(&tags->tags[PROPAGATED_BINARY_TAGS], buffer, buf_size); } // Decode a tag set. @@ -475,16 +482,16 @@ 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 *new_ts = gpr_malloc(sizeof(census_tag_set)); - memset(&new_ts->local_tags, 0, sizeof(struct tag_set)); + memset(&new_ts->tags[LOCAL_TAGS], 0, sizeof(struct tag_set)); if (buffer == NULL) { - memset(&new_ts->propagated_tags, 0, sizeof(struct tag_set)); + memset(&new_ts->tags[PROPAGATED_TAGS], 0, sizeof(struct tag_set)); } else { - tag_set_decode(&new_ts->propagated_tags, buffer, size); + tag_set_decode(&new_ts->tags[PROPAGATED_TAGS], buffer, size); } if (bin_buffer == NULL) { - memset(&new_ts->propagated_binary_tags, 0, sizeof(struct tag_set)); + memset(&new_ts->tags[PROPAGATED_BINARY_TAGS], 0, sizeof(struct tag_set)); } else { - tag_set_decode(&new_ts->propagated_binary_tags, bin_buffer, bin_size); + tag_set_decode(&new_ts->tags[PROPAGATED_BINARY_TAGS], bin_buffer, bin_size); } // 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 4c286b3e4f..140aa8117b 100644 --- a/test/core/census/tag_set_test.c +++ b/test/core/census/tag_set_test.c @@ -108,46 +108,34 @@ 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); + struct census_tag_set *cts = census_tag_set_create(NULL, NULL, 0, NULL); GPR_ASSERT(census_tag_set_ntags(cts) == 0); census_tag_set_destroy(cts); } -// Create basic tag set, and test that retreiving tag by index works. +// Test create and iteration over basic tag set. static void basic_test(void) { struct census_tag_set *cts = - census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT); + census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); - for (int i = 0; i < census_tag_set_ntags(cts); i++) { - census_tag tag; - GPR_ASSERT(census_tag_set_get_tag_by_index(cts, i, &tag) == 1); + census_tag_set_iterator it; + census_tag_set_initialize_iterator(cts, &it); + census_tag tag; + while (census_tag_set_next_tag(&it, &tag)) { // can't rely on tag return order: make sure it matches exactly one. int matches = 0; - for (int j = 0; j < BASIC_TAG_COUNT; j++) { - if (compare_tag(&tag, &basic_tags[j])) matches++; + for (int i = 0; i < BASIC_TAG_COUNT; i++) { + if (compare_tag(&tag, &basic_tags[i])) matches++; } GPR_ASSERT(matches == 1); } census_tag_set_destroy(cts); } -// Try census_tag_set_get_tag_by_index() with bad indices. -static void bad_index_test(void) { - struct census_tag_set *cts = - census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT); - GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); - census_tag tag; - GPR_ASSERT(census_tag_set_get_tag_by_index(cts, -1, &tag) == 0); - GPR_ASSERT(census_tag_set_get_tag_by_index(cts, BASIC_TAG_COUNT, &tag) == 0); - GPR_ASSERT(census_tag_set_get_tag_by_index(cts, BASIC_TAG_COUNT + 1, &tag) == - 0); - census_tag_set_destroy(cts); -} - // Test that census_tag_set_get_tag_by_key(). static void lookup_by_key_test(void) { struct census_tag_set *cts = - census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT); + 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++) { @@ -175,35 +163,35 @@ 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); - struct census_tag_set *cts = census_tag_set_create(NULL, &tag, 1); + struct census_tag_set *cts = census_tag_set_create(NULL, &tag, 1, NULL); GPR_ASSERT(census_tag_set_ntags(cts) == 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); + cts = census_tag_set_create(NULL, &tag, 1, NULL); GPR_ASSERT(census_tag_set_ntags(cts) == 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); + cts = census_tag_set_create(NULL, &tag, 1, NULL); GPR_ASSERT(census_tag_set_ntags(cts) == 1); census_tag_set_destroy(cts); // now try with long values tag.value_len = 300; - cts = census_tag_set_create(NULL, &tag, 1); + cts = census_tag_set_create(NULL, &tag, 1, NULL); GPR_ASSERT(census_tag_set_ntags(cts) == 0); census_tag_set_destroy(cts); tag.value_len = CENSUS_MAX_TAG_KV_LEN + 1; - cts = census_tag_set_create(NULL, &tag, 1); + cts = census_tag_set_create(NULL, &tag, 1, NULL); GPR_ASSERT(census_tag_set_ntags(cts) == 0); census_tag_set_destroy(cts); tag.value_len = CENSUS_MAX_TAG_KV_LEN; - cts = census_tag_set_create(NULL, &tag, 1); + cts = census_tag_set_create(NULL, &tag, 1, NULL); GPR_ASSERT(census_tag_set_ntags(cts) == 1); census_tag_set_destroy(cts); // 0 length key. key[0] = 0; - cts = census_tag_set_create(NULL, &tag, 1); + cts = census_tag_set_create(NULL, &tag, 1, NULL); GPR_ASSERT(census_tag_set_ntags(cts) == 0); census_tag_set_destroy(cts); } @@ -211,9 +199,9 @@ static void invalid_test(void) { // Make a copy of a tag set static void copy_test(void) { struct census_tag_set *cts = - census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT); + census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); - struct census_tag_set *cts2 = census_tag_set_create(cts, NULL, 0); + struct census_tag_set *cts2 = census_tag_set_create(cts, NULL, 0, NULL); GPR_ASSERT(census_tag_set_ntags(cts2) == BASIC_TAG_COUNT); for (int i = 0; i < census_tag_set_ntags(cts2); i++) { census_tag tag; @@ -228,10 +216,10 @@ static void copy_test(void) { // replace a single tag value static void replace_value_test(void) { struct census_tag_set *cts = - census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT); + census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); struct census_tag_set *cts2 = - census_tag_set_create(cts, modify_tags + REPLACE_VALUE_OFFSET, 1); + census_tag_set_create(cts, modify_tags + REPLACE_VALUE_OFFSET, 1, NULL); GPR_ASSERT(census_tag_set_ntags(cts2) == BASIC_TAG_COUNT); census_tag tag; GPR_ASSERT(census_tag_set_get_tag_by_key( @@ -244,10 +232,10 @@ static void replace_value_test(void) { // replace a single tags flags static void replace_flags_test(void) { struct census_tag_set *cts = - census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT); + census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); struct census_tag_set *cts2 = - census_tag_set_create(cts, modify_tags + REPLACE_FLAG_OFFSET, 1); + census_tag_set_create(cts, modify_tags + REPLACE_FLAG_OFFSET, 1, NULL); GPR_ASSERT(census_tag_set_ntags(cts2) == BASIC_TAG_COUNT); census_tag tag; GPR_ASSERT(census_tag_set_get_tag_by_key( @@ -260,10 +248,10 @@ static void replace_flags_test(void) { // delete a single tag. static void delete_tag_test(void) { struct census_tag_set *cts = - census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT); + census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); struct census_tag_set *cts2 = - census_tag_set_create(cts, modify_tags + DELETE_TAG_OFFSET, 1); + census_tag_set_create(cts, modify_tags + DELETE_TAG_OFFSET, 1, NULL); GPR_ASSERT(census_tag_set_ntags(cts2) == BASIC_TAG_COUNT - 1); census_tag tag; GPR_ASSERT(census_tag_set_get_tag_by_key( @@ -275,10 +263,10 @@ static void delete_tag_test(void) { // add a single new tag. static void add_tag_test(void) { struct census_tag_set *cts = - census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT); + census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); struct census_tag_set *cts2 = - census_tag_set_create(cts, modify_tags + ADD_TAG_OFFSET, 1); + census_tag_set_create(cts, modify_tags + ADD_TAG_OFFSET, 1, NULL); GPR_ASSERT(census_tag_set_ntags(cts2) == BASIC_TAG_COUNT + 1); census_tag tag; GPR_ASSERT(census_tag_set_get_tag_by_key( @@ -291,10 +279,10 @@ static void add_tag_test(void) { // test many changes at once. static void replace_add_delete_test(void) { struct census_tag_set *cts = - census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT); + census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); struct census_tag_set *cts2 = - census_tag_set_create(cts, modify_tags, MODIFY_TAG_COUNT); + census_tag_set_create(cts, modify_tags, MODIFY_TAG_COUNT, NULL); GPR_ASSERT(census_tag_set_ntags(cts2) == 8); // validate tag set contents. Use specific indices into the two arrays // holding tag values. @@ -321,7 +309,7 @@ static void simple_encode_decode_test(void) { char buf1[1000]; char buf2[1000]; struct census_tag_set *cts = - census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT); + 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); @@ -352,10 +340,10 @@ static void complex_encode_decode_test(void) { char buf1[500]; char buf2[500]; struct census_tag_set *cts = - census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT); + census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); struct census_tag_set *cts2 = - census_tag_set_create(cts, modify_tags, MODIFY_TAG_COUNT); + census_tag_set_create(cts, modify_tags, MODIFY_TAG_COUNT, NULL); GPR_ASSERT(census_tag_set_ntags(cts2) == 8); size_t b1 = census_tag_set_encode_propagated(cts2, buf1, 500); @@ -376,7 +364,6 @@ int main(int argc, char *argv[]) { grpc_test_init(argc, argv); empty_test(); basic_test(); - bad_index_test(); lookup_by_key_test(); invalid_test(); copy_test(); -- cgit v1.2.3 From a88a22166d768fbfc2da14d88fada058cfad7735 Mon Sep 17 00:00:00 2001 From: Alistair Veitch Date: Fri, 15 Jan 2016 12:13:36 -0800 Subject: add creation status --- include/grpc/census.h | 22 ++++++--- src/core/census/tag_set.c | 72 +++++++++++++++++++-------- test/core/census/tag_set_test.c | 107 ++++++++++++++++++++++------------------ 3 files changed, 125 insertions(+), 76 deletions(-) (limited to 'src/core/census/tag_set.c') diff --git a/include/grpc/census.h b/include/grpc/census.h index 1d7e65cdd9..04b2c9db5f 100644 --- a/include/grpc/census.h +++ b/include/grpc/census.h @@ -368,22 +368,27 @@ typedef struct { int n_modified_tags; /* number of tags that were modified */ int n_invalid_tags; /* number of tags with bad keys or values (e.g. longer than CENSUS_MAX_TAG_KV_LEN) */ -} census_tag_set_create_stats; + int n_ignored_tags; /* number of tags ignored because of + CENSUS_MAX_PROPAGATED_TAGS limit. */ +} census_tag_set_create_status; /* Create a new tag set, adding and removing tags from an existing tag set. @param base Base tag set to build upon. Can be NULL. @param tags A set of tags to be added/changed/deleted. Tags with keys that are in 'tags', but not 'base', are added to the tag set. Keys that are in - both 'tags' and 'base' will have their value replaced. Tags with keys in - both, but with NULL or zero-length values, will be deleted from the - tag set. + both 'tags' and 'base' will have their value/flags modified. Tags with keys + in both, but with NULL or zero-length values, will be deleted from the tag + set. Tags with invalid (too long or short) keys or values will be ignored. + If adding a tag will result in more than CENSUS_MAX_PROPAGATED_TAGS in either + binary or non-binary tags, they will be ignored, as will deletions of + tags that don't exist. @param ntags number of tags in 'tags' @param stats Information about the tag set created and actions taken during its creation. */ census_tag_set *census_tag_set_create(const census_tag_set *base, const census_tag *tags, int ntags, - census_tag_set_create_stats *stats); + census_tag_set_create_status *status); /* Destroy a tag set created by census_tag_set_create(). Once this function has been called, the tag set cannot be reused. */ @@ -429,9 +434,12 @@ size_t census_tag_set_encode_propagated(const census_tag_set *tags, 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_*(). */ +/* 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. */ census_tag_set *census_tag_set_decode(const char *buffer, size_t size, - const char *bin_buffer, size_t bin_size); + const char *bin_buffer, size_t bin_size, + census_tag_set_create_status *status); /* 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 3b8efb1935..f5f32f5e0f 100644 --- a/src/core/census/tag_set.c +++ b/src/core/census/tag_set.c @@ -31,9 +31,6 @@ * */ /* -- ability to add extra tags in encode? -- add drops error count to create_ts -- add mask to ntags? - comment about key/value ptrs being to mem - add comment about encode/decode being for RPC use only. */ @@ -175,9 +172,13 @@ static bool cts_delete_tag(census_tag_set *tags, const census_tag *tag, key_len)); } -// Add a tag to a tag set. -static void tag_set_add_tag(struct tag_set *tags, const census_tag *tag, +// Add a tag to a tag set. Return true on sucess, false if the tag could +// not be added because of tag size constraints. +static bool tag_set_add_tag(struct tag_set *tags, const census_tag *tag, size_t key_len) { + if (tags->ntags == CENSUS_MAX_PROPAGATED_TAGS) { + return false; + } const size_t tag_size = key_len + tag->value_len + TAG_HEADER_SIZE; if (tags->kvm_used + tag_size > tags->kvm_size) { // allocate new memory if needed @@ -199,22 +200,41 @@ static void tag_set_add_tag(struct tag_set *tags, const census_tag *tag, tags->kvm_used += tag_size; tags->ntags++; tags->ntags_alloc++; + return true; } -// Add a tag to a census_tag_set +// Add a tag to a census_tag_set. static void cts_add_tag(census_tag_set *tags, const census_tag *tag, - size_t key_len) { + size_t key_len, census_tag_set_create_status *status) { // first delete the tag if it is already present - cts_delete_tag(tags, tag, key_len); - if (tag->value != NULL && tag->value_len != 0) { + bool deleted = cts_delete_tag(tags, tag, key_len); + bool call_add = tag->value != NULL && tag->value_len != 0; + bool added = false; + if (call_add) { if (CENSUS_TAG_IS_PROPAGATED(tag->flags)) { if (CENSUS_TAG_IS_BINARY(tag->flags)) { - tag_set_add_tag(&tags->tags[PROPAGATED_BINARY_TAGS], tag, key_len); + added = + tag_set_add_tag(&tags->tags[PROPAGATED_BINARY_TAGS], tag, key_len); + } else { + added = tag_set_add_tag(&tags->tags[PROPAGATED_TAGS], tag, key_len); + } + } else { + added = tag_set_add_tag(&tags->tags[LOCAL_TAGS], tag, key_len); + } + } + if (status) { + if (deleted) { + if (call_add) { + status->n_modified_tags++; } else { - tag_set_add_tag(&tags->tags[PROPAGATED_TAGS], tag, key_len); + status->n_deleted_tags++; } } else { - tag_set_add_tag(&tags->tags[LOCAL_TAGS], tag, key_len); + if (added) { + status->n_added_tags++; + } else { + status->n_ignored_tags++; + } } } } @@ -263,8 +283,11 @@ static void tag_set_flatten(struct tag_set *tags) { census_tag_set *census_tag_set_create(const census_tag_set *base, const census_tag *tags, int ntags, - census_tag_set_create_stats *stats) { + census_tag_set_create_status *status) { int n_invalid_tags = 0; + if (status) { + memset(status, 0, sizeof(*status)); + } census_tag_set *new_ts = gpr_malloc(sizeof(census_tag_set)); if (base == NULL) { memset(new_ts, 0, sizeof(census_tag_set)); @@ -280,7 +303,7 @@ 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_add_tag(new_ts, tag, key_len); + cts_add_tag(new_ts, tag, key_len, status); } else { n_invalid_tags++; } @@ -288,12 +311,12 @@ census_tag_set *census_tag_set_create(const census_tag_set *base, 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 (stats != NULL) { - stats->n_propagated_tags = new_ts->tags[PROPAGATED_TAGS].ntags; - stats->n_propagated_binary_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; - stats->n_local_tags = new_ts->tags[LOCAL_TAGS].ntags; - stats->n_invalid_tags = n_invalid_tags; + status->n_local_tags = new_ts->tags[LOCAL_TAGS].ntags; + status->n_invalid_tags = n_invalid_tags; } return new_ts; } @@ -480,7 +503,11 @@ 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) { + const char *bin_buffer, size_t bin_size, + census_tag_set_create_status *status) { + if (status) { + memset(status, 0, sizeof(*status)); + } 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) { @@ -493,6 +520,11 @@ 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; + } // 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 140aa8117b..bc084ec04b 100644 --- a/test/core/census/tag_set_test.c +++ b/test/core/census/tag_set_test.c @@ -70,7 +70,7 @@ static census_tag basic_tags[BASIC_TAG_COUNT] = { // you add or delete entries, you will also need to change the test. Other // tests that rely on specific instances have XXX_XXX_OFFSET definitions (also // change the defines below if you add/delete entires). -#define MODIFY_TAG_COUNT 10 +#define MODIFY_TAG_COUNT 11 static census_tag modify_tags[MODIFY_TAG_COUNT] = { #define REPLACE_VALUE_OFFSET 0 /* 0 */ {"key0", "replace printable", 18, 0}, // replaces tag value only @@ -88,8 +88,8 @@ static census_tag modify_tags[MODIFY_TAG_COUNT] = { /* 8 */ {"k2", (char *)&eight_byte_val, 8, CENSUS_TAG_BINARY | CENSUS_TAG_PROPAGATE}, // more flags change // non-binary -> binary - /* 9 */ {"k6", "bar", 4, - 0} // add back tag, with different value, but same length + /* 9 */ {"k6", "bar", 4, 0}, // add back tag, with different value + /* 10 */ {"foo", "bar", 4, CENSUS_TAG_PROPAGATE}, // another new tag }; // Utility function to compare tags. Returns true if all fields match. @@ -115,9 +115,12 @@ static void empty_test(void) { // Test create and iteration over basic tag set. static void basic_test(void) { + census_tag_set_create_status status; struct census_tag_set *cts = - census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); + 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); census_tag_set_iterator it; census_tag_set_initialize_iterator(cts, &it); census_tag tag; @@ -163,36 +166,46 @@ 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); - struct census_tag_set *cts = census_tag_set_create(NULL, &tag, 1, NULL); + 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); 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, NULL); + 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); 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, NULL); + 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); census_tag_set_destroy(cts); // now try with long values tag.value_len = 300; - cts = census_tag_set_create(NULL, &tag, 1, NULL); + 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); census_tag_set_destroy(cts); tag.value_len = CENSUS_MAX_TAG_KV_LEN + 1; - cts = census_tag_set_create(NULL, &tag, 1, NULL); + 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); census_tag_set_destroy(cts); tag.value_len = CENSUS_MAX_TAG_KV_LEN; - cts = census_tag_set_create(NULL, &tag, 1, NULL); + 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); census_tag_set_destroy(cts); // 0 length key. key[0] = 0; - cts = census_tag_set_create(NULL, &tag, 1, NULL); + 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); census_tag_set_destroy(cts); } @@ -201,8 +214,11 @@ 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); - struct census_tag_set *cts2 = census_tag_set_create(cts, NULL, 0, NULL); + 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++) { census_tag tag; GPR_ASSERT(census_tag_set_get_tag_by_key(cts2, basic_tags[i].key, &tag) == @@ -218,9 +234,12 @@ 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); - struct census_tag_set *cts2 = - census_tag_set_create(cts, modify_tags + REPLACE_VALUE_OFFSET, 1, NULL); + 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); census_tag tag; GPR_ASSERT(census_tag_set_get_tag_by_key( cts2, modify_tags[REPLACE_VALUE_OFFSET].key, &tag) == 1); @@ -234,9 +253,12 @@ 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; struct census_tag_set *cts2 = - census_tag_set_create(cts, modify_tags + REPLACE_FLAG_OFFSET, 1, NULL); + 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); census_tag tag; GPR_ASSERT(census_tag_set_get_tag_by_key( cts2, modify_tags[REPLACE_FLAG_OFFSET].key, &tag) == 1); @@ -250,9 +272,12 @@ 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; struct census_tag_set *cts2 = - census_tag_set_create(cts, modify_tags + DELETE_TAG_OFFSET, 1, NULL); + 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); census_tag tag; GPR_ASSERT(census_tag_set_get_tag_by_key( cts2, modify_tags[DELETE_TAG_OFFSET].key, &tag) == 0); @@ -265,9 +290,12 @@ 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; struct census_tag_set *cts2 = - census_tag_set_create(cts, modify_tags + ADD_TAG_OFFSET, 1, NULL); + 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); census_tag tag; GPR_ASSERT(census_tag_set_get_tag_by_key( cts2, modify_tags[ADD_TAG_OFFSET].key, &tag) == 1); @@ -281,9 +309,12 @@ 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; struct census_tag_set *cts2 = - census_tag_set_create(cts, modify_tags, MODIFY_TAG_COUNT, NULL); - GPR_ASSERT(census_tag_set_ntags(cts2) == 8); + 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); // validate tag set contents. Use specific indices into the two arrays // holding tag values. GPR_ASSERT(validate_tag(cts2, &basic_tags[3])); @@ -294,6 +325,7 @@ static void replace_add_delete_test(void) { GPR_ASSERT(validate_tag(cts2, &modify_tags[7])); GPR_ASSERT(validate_tag(cts2, &modify_tags[8])); GPR_ASSERT(validate_tag(cts2, &modify_tags[9])); + GPR_ASSERT(validate_tag(cts2, &modify_tags[10])); GPR_ASSERT(!validate_tag(cts2, &basic_tags[0])); GPR_ASSERT(!validate_tag(cts2, &basic_tags[1])); GPR_ASSERT(!validate_tag(cts2, &basic_tags[2])); @@ -304,8 +336,8 @@ static void replace_add_delete_test(void) { census_tag_set_destroy(cts2); } -// Use the basic tag set to test encode/decode. -static void simple_encode_decode_test(void) { +// test encode/decode. +static void encode_decode_test(void) { char buf1[1000]; char buf2[1000]; struct census_tag_set *cts = @@ -317,9 +349,12 @@ static void simple_encode_decode_test(void) { 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 *cts2 = census_tag_set_decode(buf1, b1, buf2, b2); + census_tag_set_create_status status; + census_tag_set *cts2 = census_tag_set_decode(buf1, b1, buf2, b2, &status); GPR_ASSERT(cts2 != NULL); GPR_ASSERT(census_tag_set_ntags(cts2) == 4); + 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++) { census_tag tag; if (CENSUS_TAG_IS_PROPAGATED(basic_tags[i].flags)) { @@ -335,31 +370,6 @@ static void simple_encode_decode_test(void) { census_tag_set_destroy(cts); } -// Use more complex/modified tag set to test encode/decode. -static void complex_encode_decode_test(void) { - char buf1[500]; - char buf2[500]; - 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); - struct census_tag_set *cts2 = - census_tag_set_create(cts, modify_tags, MODIFY_TAG_COUNT, NULL); - GPR_ASSERT(census_tag_set_ntags(cts2) == 8); - - size_t b1 = census_tag_set_encode_propagated(cts2, buf1, 500); - GPR_ASSERT(b1 != 0); - size_t b2 = census_tag_set_encode_propagated_binary(cts2, buf2, 500); - GPR_ASSERT(b2 != 0); - census_tag_set *cts3 = census_tag_set_decode(buf1, b1, buf2, b2); - GPR_ASSERT(cts3 != NULL); - GPR_ASSERT(census_tag_set_ntags(cts3) == 2); - GPR_ASSERT(validate_tag(cts3, &basic_tags[4])); - GPR_ASSERT(validate_tag(cts3, &modify_tags[8])); - census_tag_set_destroy(cts3); - census_tag_set_destroy(cts2); - census_tag_set_destroy(cts); -} - int main(int argc, char *argv[]) { grpc_test_init(argc, argv); empty_test(); @@ -372,7 +382,6 @@ int main(int argc, char *argv[]) { delete_tag_test(); add_tag_test(); replace_add_delete_test(); - simple_encode_decode_test(); - complex_encode_decode_test(); + encode_decode_test(); return 0; } -- cgit v1.2.3 From 4bbdb825677bde8e53cbab58ad4193c505dc9a50 Mon Sep 17 00:00:00 2001 From: Alistair Veitch Date: Tue, 19 Jan 2016 13:56:52 -0800 Subject: comment updates --- include/grpc/census.h | 24 ++++++++++---------- src/core/census/tag_set.c | 56 +++++++++++++++++++++++++---------------------- 2 files changed, 43 insertions(+), 37 deletions(-) (limited to 'src/core/census/tag_set.c') diff --git a/include/grpc/census.h b/include/grpc/census.h index 7b769c62ee..4b3d021410 100644 --- a/include/grpc/census.h +++ b/include/grpc/census.h @@ -329,12 +329,14 @@ void census_trace_scan_end(); a tag set. All contexts have an associated tag set. */ typedef struct census_tag_set census_tag_set; -/* A tag is a key:value pair. The key is a non-empty, printable, nil-terminated - string. The value is a binary string, that may be printable. There are no - limits on the sizes of either keys or values, but code authors should - remember that systems may have inbuilt limits (e.g. for propagated tags, - the bytes on the wire) and that larger tags means more memory consumed and - time in processing. */ +/* A tag is a key:value pair. The key is a non-empty, printable (UTF-8 + encoded), nil-terminated string. The value is a binary string, that may be + printable. There are limits on the sizes of both keys and values (see + CENSUS_MAX_TAG_KB_LEN definition below), and the number of tags that can be + propagated (CENSUS_MAX_PROPAGATED_TAGS). Users should also remember that + some systems may have limits on, e.g., the number of bytes that can be + transmitted as metadata, and that larger tags means more memory consumed + and time in processing. */ typedef struct { const char *key; const char *value; @@ -342,6 +344,11 @@ typedef struct { uint8_t flags; } census_tag; +/* Maximum length of a tag's key or value. */ +#define CENSUS_MAX_TAG_KV_LEN 255 +/* Maximum number of propagatable tags. */ +#define CENSUS_MAX_PROPAGATED_TAGS 255 + /* Tag flags. */ #define CENSUS_TAG_PROPAGATE 1 /* Tag should be propagated over RPC */ #define CENSUS_TAG_STATS 2 /* Tag will be used for statistics aggregation */ @@ -354,11 +361,6 @@ typedef struct { #define CENSUS_TAG_IS_STATS(flags) (flags & CENSUS_TAG_STATS) #define CENSUS_TAG_IS_BINARY(flags) (flags & CENSUS_TAG_BINARY) -/* Maximum length of key/value in a tag. */ -#define CENSUS_MAX_TAG_KV_LEN 255 -/* Maximum number of propagatable tags. */ -#define CENSUS_MAX_PROPAGATED_TAGS 255 - typedef struct { int n_propagated_tags; /* number of propagated printable tags */ int n_propagated_binary_tags; /* number of propagated binary tags */ diff --git a/src/core/census/tag_set.c b/src/core/census/tag_set.c index f5f32f5e0f..718cfe3b25 100644 --- a/src/core/census/tag_set.c +++ b/src/core/census/tag_set.c @@ -30,10 +30,6 @@ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. * */ -/* -- comment about key/value ptrs being to mem -- add comment about encode/decode being for RPC use only. -*/ #include #include @@ -65,10 +61,10 @@ // that can be directly copied to the wire. This makes iteration by index // somewhat less efficient. If it becomes a problem, we could consider // building an index at tag_set creation. -// * Binary tags are encoded seperately from non-binary tags. There are a -// primarily because non-binary tags are far more likely to be repeated -// across multiple RPC calls, so are more efficiently cached and -// compressed in any metadata schemes. +// * Binary tags share the same structure as, but are encoded seperately from, +// non-binary tags. This is primarily because non-binary tags are far more +// likely to be repeated across multiple RPC calls, so are more efficiently +// cached and compressed in any metadata schemes. // * all lengths etc. are restricted to one byte. This eliminates endian // issues. @@ -108,7 +104,7 @@ struct raw_tag { char *value; }; -// use reserved flag bit for indication of deleted tag. +// Use a reserved flag bit for indication of deleted tag. #define CENSUS_TAG_DELETED CENSUS_TAG_RESERVED #define CENSUS_TAG_IS_DELETED(flags) (flags & CENSUS_TAG_DELETED) @@ -125,8 +121,8 @@ struct census_tag_set { #define LOCAL_TAGS 2 // Extract a raw tag given a pointer (raw) to the tag header. Allow for some -// extra bytes in the tag header (see encode/decode for usage: allows for -// future expansion of the tag header). +// extra bytes in the tag header (see encode/decode functions for usage: this +// allows for future expansion of the tag header). static char *decode_tag(struct raw_tag *tag, char *header, int offset) { tag->key_len = (uint8_t)(*header++); tag->value_len = (uint8_t)(*header++); @@ -145,7 +141,7 @@ static void tag_set_copy(struct tag_set *to, const struct tag_set *from) { memcpy(to->kvm, from->kvm, to->kvm_used); } -// Delete a tag from a tag set, if it exists (returns true it it did). +// Delete a tag from a tag_set, if it exists (returns true it it did). static bool tag_set_delete_tag(struct tag_set *tags, const char *key, size_t key_len) { char *kvp = tags->kvm; @@ -163,7 +159,7 @@ static bool tag_set_delete_tag(struct tag_set *tags, const char *key, return false; } -// Delete a tag from a tag set, return true if it existed. +// Delete a tag from a census_tag_set, return true if it existed. static bool cts_delete_tag(census_tag_set *tags, const census_tag *tag, size_t key_len) { return (tag_set_delete_tag(&tags->tags[LOCAL_TAGS], tag->key, key_len) || @@ -172,8 +168,8 @@ static bool cts_delete_tag(census_tag_set *tags, const census_tag *tag, key_len)); } -// Add a tag to a tag set. Return true on sucess, false if the tag could -// not be added because of tag size constraints. +// Add a tag to a tag_set. Return true on sucess, false if the tag could +// not be added because of constraints on tag set size. static bool tag_set_add_tag(struct tag_set *tags, const census_tag *tag, size_t key_len) { if (tags->ntags == CENSUS_MAX_PROPAGATED_TAGS) { @@ -203,11 +199,14 @@ static bool tag_set_add_tag(struct tag_set *tags, const census_tag *tag, return true; } -// Add a tag to a census_tag_set. -static void cts_add_tag(census_tag_set *tags, const census_tag *tag, - size_t key_len, census_tag_set_create_status *status) { - // first delete the tag if it is already present +// 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) { + // 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. bool call_add = tag->value != NULL && tag->value_len != 0; bool added = false; if (call_add) { @@ -239,7 +238,7 @@ static void cts_add_tag(census_tag_set *tags, const census_tag *tag, } } -// Remove any deleted tags from the tag set. Basic algorithm: +// Remove memory used for deleted tags from the tag set. Basic algorithm: // 1) Walk through tag set to find first deleted tag. Record where it is. // 2) Find the next not-deleted tag. Copy all of kvm from there to the end // "over" the deleted tags @@ -289,6 +288,8 @@ census_tag_set *census_tag_set_create(const census_tag_set *base, memset(status, 0, sizeof(*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. if (base == NULL) { memset(new_ts, 0, sizeof(census_tag_set)); } else { @@ -297,17 +298,20 @@ census_tag_set *census_tag_set_create(const census_tag_set *base, &base->tags[PROPAGATED_BINARY_TAGS]); tag_set_copy(&new_ts->tags[LOCAL_TAGS], &base->tags[LOCAL_TAGS]); } + // Walk over the additional tags and, for those that aren't invalid, modify + // the tag set to add/replace/delete as required. for (int i = 0; i < ntags; i++) { const census_tag *tag = &tags[i]; size_t key_len = strlen(tag->key) + 1; // 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_add_tag(new_ts, tag, key_len, status); + cts_modify_tag(new_ts, tag, key_len, status); } else { 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]); @@ -334,8 +338,8 @@ int census_tag_set_ntags(const census_tag_set *tags) { tags->tags[LOCAL_TAGS].ntags; } -/* Initialize a tag set iterator. Must be called before first use of the - iterator. */ +// 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, census_tag_set_iterator *iterator) { iterator->tags = tags; @@ -354,9 +358,9 @@ void census_tag_set_initialize_iterator(const census_tag_set *tags, } } -/* Get the contents of the "next" tag in the tag set. If there are no more - tags in the tag set, returns 0 (and 'tag' contents will be unchanged), - otherwise returns 1. */ +// Get the contents of the "next" tag in the tag set. If there are no more +// tags in the tag set, returns 0 (and 'tag' contents will be unchanged), +// otherwise returns 1. */ int census_tag_set_next_tag(census_tag_set_iterator *iterator, census_tag *tag) { if (iterator->base < 0) { -- cgit v1.2.3 From 8e5b21fd9faf0b1ba2f74e88fe7e59bc3d5ed299 Mon Sep 17 00:00:00 2001 From: Alistair Veitch Date: Wed, 20 Jan 2016 09:17:11 -0800 Subject: comment updates --- include/grpc/census.h | 10 +++++++--- src/core/census/tag_set.c | 3 ++- 2 files changed, 9 insertions(+), 4 deletions(-) (limited to 'src/core/census/tag_set.c') diff --git a/include/grpc/census.h b/include/grpc/census.h index 4b3d021410..5443488fbe 100644 --- a/include/grpc/census.h +++ b/include/grpc/census.h @@ -375,6 +375,8 @@ typedef struct { } census_tag_set_create_status; /* Create a new tag set, adding and removing tags from an existing tag set. + This will copy all tags from it's input parameters, so it is recommended + to add as many tags in a single operation as is practical for the client. @param base Base tag set to build upon. Can be NULL. @param tags A set of tags to be added/changed/deleted. Tags with keys that are in 'tags', but not 'base', are added to the tag set. Keys that are in @@ -385,8 +387,10 @@ typedef struct { binary or non-binary tags, they will be ignored, as will deletions of tags that don't exist. @param ntags number of tags in 'tags' - @param stats Information about the tag set created and actions taken during - its creation. + @param status If not NULL, the pointed to structure will be filled in with + information about the new tag set and status of the tags used in its + 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, @@ -396,7 +400,7 @@ census_tag_set *census_tag_set_create(const census_tag_set *base, has been called, the tag set cannot be reused. */ void census_tag_set_destroy(census_tag_set *tags); -/* Get the number of tags in the tag set. */ +/* Get the total number of tags in the tag set. */ int census_tag_set_ntags(const census_tag_set *tags); /* Structure used for tag set iteration. API clients should not use or diff --git a/src/core/census/tag_set.c b/src/core/census/tag_set.c index 718cfe3b25..3b5d345910 100644 --- a/src/core/census/tag_set.c +++ b/src/core/census/tag_set.c @@ -56,7 +56,8 @@ // are to aid in efficient parsing and the ability to directly return key // strings. This is more important than saving a single byte/tag on the wire. // * The wire encoding uses only single byte values. This eliminates the need -// to handle endian-ness conversions. +// to handle endian-ness conversions. It also means there is a hard upper +// limit of 255 for both CENSUS_MAX_TAG_KV_LEN and CENSUS_MAX_PROPAGATED_TAGS. // * Keep all tag information (keys/values/flags) in a single memory buffer, // that can be directly copied to the wire. This makes iteration by index // somewhat less efficient. If it becomes a problem, we could consider -- cgit v1.2.3 From 9d8dabb5522bb877ee506a67ce370b649405338d Mon Sep 17 00:00:00 2001 From: Alistair Veitch Date: Thu, 21 Jan 2016 13:34:41 -0800 Subject: address tag_set comments --- src/core/census/tag_set.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) (limited to 'src/core/census/tag_set.c') diff --git a/src/core/census/tag_set.c b/src/core/census/tag_set.c index 3b5d345910..5cc777b085 100644 --- a/src/core/census/tag_set.c +++ b/src/core/census/tag_set.c @@ -41,7 +41,7 @@ #include "src/core/support/string.h" // Functions in this file support the public tag_set API, as well as -// encoding/decoding tag_sets as part of context transmission across +// encoding/decoding tag_sets as part of context propagation across // RPC's. The overall requirements (in approximate priority order) for the // tag_set representations: // 1. Efficient conversion to/from wire format @@ -59,10 +59,8 @@ // to handle endian-ness conversions. It also means there is a hard upper // limit of 255 for both CENSUS_MAX_TAG_KV_LEN and CENSUS_MAX_PROPAGATED_TAGS. // * Keep all tag information (keys/values/flags) in a single memory buffer, -// that can be directly copied to the wire. This makes iteration by index -// somewhat less efficient. If it becomes a problem, we could consider -// building an index at tag_set creation. -// * Binary tags share the same structure as, but are encoded seperately from, +// that can be directly copied to the wire.14 +// * Binary tags share the same structure as, but are encoded separately from, // non-binary tags. This is primarily because non-binary tags are far more // likely to be repeated across multiple RPC calls, so are more efficiently // cached and compressed in any metadata schemes. @@ -111,7 +109,8 @@ struct raw_tag { // Primary (external) representation of a tag set. Composed of 3 underlying // tag_set structs, one for each of the binary/printable propagated tags, and -// one for everything else. +// one for everything else. This is to efficiently support tag +// encoding/decoding. struct census_tag_set { struct tag_set tags[3]; }; @@ -139,10 +138,10 @@ static char *decode_tag(struct raw_tag *tag, char *header, int offset) { static void tag_set_copy(struct tag_set *to, const struct tag_set *from) { memcpy(to, from, sizeof(struct tag_set)); to->kvm = gpr_malloc(to->kvm_size); - memcpy(to->kvm, from->kvm, to->kvm_used); + memcpy(to->kvm, from->kvm, from->kvm_used); } -// Delete a tag from a tag_set, if it exists (returns true it it did). +// Delete a tag from a tag_set, if it exists (returns true if it did). static bool tag_set_delete_tag(struct tag_set *tags, const char *key, size_t key_len) { char *kvp = tags->kvm; @@ -249,18 +248,13 @@ static void cts_modify_tag(census_tag_set *tags, const census_tag *tag, // appropriate amount. static void tag_set_flatten(struct tag_set *tags) { if (tags->ntags == tags->ntags_alloc) return; - bool find_deleted = true; // are we looking for deleted tags? + bool found_deleted = false; // found a deleted tag. char *kvp = tags->kvm; char *dbase; // record location of deleted tag for (int i = 0; i < tags->ntags_alloc; i++) { struct raw_tag tag; char *next_kvp = decode_tag(&tag, kvp, 0); - if (find_deleted) { - if (CENSUS_TAG_IS_DELETED(tag.flags)) { - dbase = kvp; - find_deleted = false; - } - } else { + if (found_deleted) { if (!CENSUS_TAG_IS_DELETED(tag.flags)) { ptrdiff_t reduce = kvp - dbase; // #bytes in deleted tags GPR_ASSERT(reduce > 0); @@ -269,12 +263,17 @@ static void tag_set_flatten(struct tag_set *tags) { memmove(dbase, kvp, (size_t)copy_size); tags->kvm_used -= (size_t)reduce; next_kvp -= reduce; - find_deleted = true; + found_deleted = false; + } + } else { + if (CENSUS_TAG_IS_DELETED(tag.flags)) { + dbase = kvp; + found_deleted = true; } } kvp = next_kvp; } - if (!find_deleted) { + if (found_deleted) { GPR_ASSERT(dbase > tags->kvm); tags->kvm_used = (size_t)(dbase - tags->kvm); } -- cgit v1.2.3 From 6670add667d9341f065550719a209f09923fce5f Mon Sep 17 00:00:00 2001 From: Alistair Veitch Date: Thu, 21 Jan 2016 17:21:53 -0800 Subject: 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(-) (limited to 'src/core/census/tag_set.c') diff --git a/include/grpc/census.h b/include/grpc/census.h index 5443488fbe..ae4bf36b1b 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 5cc777b085..88269cf406 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 bc084ec04b..752a74d401 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) == -- cgit v1.2.3 From d409e3bf41d0102c7ab4370a5470c4259aad88a8 Mon Sep 17 00:00:00 2001 From: Alistair Veitch Date: Fri, 22 Jan 2016 09:34:23 -0800 Subject: update comments --- src/core/census/tag_set.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) (limited to 'src/core/census/tag_set.c') diff --git a/src/core/census/tag_set.c b/src/core/census/tag_set.c index 88269cf406..f573e909b3 100644 --- a/src/core/census/tag_set.c +++ b/src/core/census/tag_set.c @@ -72,19 +72,22 @@ struct tag_set { int ntags; // number of tags. int ntags_alloc; // ntags + number of deleted tags (total number of tags - // in all of kvm). This will always be == ntags, except - // during the process of building a new tag set. + // in all of kvm). This will always be == ntags, except during the process + // of building a new tag set. size_t kvm_size; // number of bytes allocated for key/value storage. size_t kvm_used; // number of bytes of used key/value memory char *kvm; // key/value memory. Consists of repeated entries of: - // Offset Size Description - // 0 1 Key length, including trailing 0. (K) - // 1 1 Value length. (V) - // 2 1 Flags - // 3 K Key bytes - // 3 + K V Value bytes - // - // We refer to the first 3 entries as the 'tag header'. + // Offset Size Description + // 0 1 Key length, including trailing 0. (K) + // 1 1 Value length. (V) + // 2 1 Flags + // 3 K Key bytes + // 3 + K V Value bytes + // + // We refer to the first 3 entries as the 'tag header'. If extra values are + // introduced in the header, you will need to modify the TAG_HEADER_SIZE + // constant, the raw_tag structure (and everything that uses it) and the + // encode/decode functions appropriately. }; // Number of bytes in tag header. @@ -170,7 +173,9 @@ static bool cts_delete_tag(census_tag_set *tags, const census_tag *tag, } // Add a tag to a tag_set. Return true on sucess, false if the tag could -// not be added because of constraints on tag set size. +// not be added because of constraints on tag set size. This function should +// not be called if the tag may already exist (in a non-deleted state) in +// the tag_set, as that would result in two tags with the same key. static bool tag_set_add_tag(struct tag_set *tags, const census_tag *tag, size_t key_len) { if (tags->ntags == CENSUS_MAX_PROPAGATED_TAGS) { -- cgit v1.2.3 From c45d088ae7d8f67f9422abc19562a81307bf78a2 Mon Sep 17 00:00:00 2001 From: Alistair Veitch Date: Fri, 22 Jan 2016 11:43:30 -0800 Subject: Single encode function --- include/grpc/census.h | 29 ++++++++++++++++++----------- src/core/census/tag_set.c | 23 +++++++++++++++-------- test/core/census/tag_set_test.c | 20 +++++++++++--------- 3 files changed, 44 insertions(+), 28 deletions(-) (limited to 'src/core/census/tag_set.c') diff --git a/include/grpc/census.h b/include/grpc/census.h index 84a3f8faf6..6e4fa5a825 100644 --- a/include/grpc/census.h +++ b/include/grpc/census.h @@ -433,17 +433,24 @@ int census_tag_set_get_tag_by_key(const census_tag_set *tags, const char *key, for use by RPC systems only, for purposes of transmitting/receiving tag sets. */ -/* Encode to-be-propagated non-binary tags from a tag set into a memory - buffer. The total number of bytes used in the buffer is returned. If the - buffer is too small to contain the encoded tag set, then 0 is returned. */ -size_t census_tag_set_encode_propagated(const census_tag_set *tags, - char *buffer, size_t buf_size); - -/* Encode to-be-propagated binary tags from a tag set into a memory - buffer. The total number of bytes used in the buffer is returned. If the - buffer is too small to contain the encoded tag set, then 0 is returned. */ -size_t census_tag_set_encode_propagated_binary(const census_tag_set *tags, - char *buffer, size_t buf_size); +/* Encode a tag set into a buffer. The propagated tags are encoded into the + buffer in two regions: one for printable tags, and one for binary tags. + @param tags tag set to be encoded + @param buffer pointer to buffer. This address will be used to encode the + printable tags. + @param buf_size On input, will be a pointer to total buffer size. On output, + will be set to total number of bytes consumed by printable + tags. + @param bin_buf_size on output, will be set to the number of bytes used to + encode the binary tags. + @return A pointer to the binary tag's encoded, or NULL if the buffer was + insufficiently large to hold the encoded tags. Thus, if successful, + printable tags are encoded into + [buffer, buffer + *buf_size) and binary tags into + [returned-ptr, returned-ptr + *bin_buf_size) (and the return value + should be buffer + *buf_size) */ +char *census_tag_set_encode(const census_tag_set *tags, char *buffer, + size_t *buf_size, size_t *bin_buf_size); /* Decode tag set buffers encoded with census_tag_set_encode_*(). Returns NULL if there is an error in parsing either buffer. */ diff --git a/src/core/census/tag_set.c b/src/core/census/tag_set.c index f573e909b3..b985eca52b 100644 --- a/src/core/census/tag_set.c +++ b/src/core/census/tag_set.c @@ -453,14 +453,21 @@ static size_t tag_set_encode(const struct tag_set *tags, char *buffer, return ENCODED_HEADER_SIZE + tags->kvm_used; } -size_t census_tag_set_encode_propagated(const census_tag_set *tags, - char *buffer, size_t buf_size) { - return tag_set_encode(&tags->tags[PROPAGATED_TAGS], buffer, buf_size); -} - -size_t census_tag_set_encode_propagated_binary(const census_tag_set *tags, - char *buffer, size_t buf_size) { - return tag_set_encode(&tags->tags[PROPAGATED_BINARY_TAGS], buffer, buf_size); +char *census_tag_set_encode(const census_tag_set *tags, char *buffer, + size_t *buf_size, size_t *bin_buf_size) { + size_t p_buf_size = + tag_set_encode(&tags->tags[PROPAGATED_TAGS], buffer, *buf_size); + if (p_buf_size == 0) { + return NULL; + } + char *b_buffer = buffer + p_buf_size; + *bin_buf_size = tag_set_encode(&tags->tags[PROPAGATED_BINARY_TAGS], b_buffer, + *buf_size - p_buf_size); + if (*bin_buf_size == 0) { + return NULL; + } + *buf_size = p_buf_size; + return b_buffer; } // Decode a tag set. diff --git a/test/core/census/tag_set_test.c b/test/core/census/tag_set_test.c index 752a74d401..4414ad9bc1 100644 --- a/test/core/census/tag_set_test.c +++ b/test/core/census/tag_set_test.c @@ -321,17 +321,19 @@ static void replace_add_delete_test(void) { // test encode/decode. static void encode_decode_test(void) { - char buf1[1000]; - char buf2[1000]; + const size_t BUF_SIZE = 200; + char buffer[BUF_SIZE]; struct census_tag_set *cts = census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); - 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 *cts2 = census_tag_set_decode(buf1, b1, buf2, b2); + size_t bsize = 2; // buffer size too small + size_t bin_bsize = 0; + GPR_ASSERT(census_tag_set_encode(cts, buffer, &bsize, &bin_bsize) == NULL); + bsize = BUF_SIZE; + char *b_buffer = census_tag_set_encode(cts, buffer, &bsize, &bin_bsize); + GPR_ASSERT(b_buffer != NULL && bsize > 0 && bin_bsize > 0 && + bsize + bin_bsize <= BUF_SIZE && b_buffer == buffer + bsize); + census_tag_set *cts2 = + census_tag_set_decode(buffer, bsize, b_buffer, bin_bsize); GPR_ASSERT(cts2 != NULL); const census_tag_set_create_status *status = census_tag_set_get_create_status(cts2); -- cgit v1.2.3 From ff14b44154b4667e666531763898e503f9f00bfd Mon Sep 17 00:00:00 2001 From: Alistair Veitch Date: Fri, 22 Jan 2016 13:01:49 -0800 Subject: Don't use buf_size as input and output in encode --- include/grpc/census.h | 17 +++++++++-------- src/core/census/tag_set.c | 14 +++++++------- test/core/census/tag_set_test.c | 19 +++++++++++-------- 3 files changed, 27 insertions(+), 23 deletions(-) (limited to 'src/core/census/tag_set.c') diff --git a/include/grpc/census.h b/include/grpc/census.h index 6e4fa5a825..ab0e0e4802 100644 --- a/include/grpc/census.h +++ b/include/grpc/census.h @@ -438,19 +438,20 @@ int census_tag_set_get_tag_by_key(const census_tag_set *tags, const char *key, @param tags tag set to be encoded @param buffer pointer to buffer. This address will be used to encode the printable tags. - @param buf_size On input, will be a pointer to total buffer size. On output, - will be set to total number of bytes consumed by printable - tags. - @param bin_buf_size on output, will be set to the number of bytes used to - encode the binary tags. + @param buf_size number of available bytes in buffer. + @param print_buf_size Will be set to the number of bytes consumed by + printable tags. + @param bin_buf_size Will be set to the number of bytes used to encode the + binary tags. @return A pointer to the binary tag's encoded, or NULL if the buffer was insufficiently large to hold the encoded tags. Thus, if successful, printable tags are encoded into - [buffer, buffer + *buf_size) and binary tags into + [buffer, buffer + *print_buf_size) and binary tags into [returned-ptr, returned-ptr + *bin_buf_size) (and the return value - should be buffer + *buf_size) */ + should be buffer + *print_buf_size) */ char *census_tag_set_encode(const census_tag_set *tags, char *buffer, - size_t *buf_size, size_t *bin_buf_size); + size_t buf_size, size_t *print_buf_size, + size_t *bin_buf_size); /* Decode tag set buffers encoded with census_tag_set_encode_*(). Returns NULL if there is an error in parsing either buffer. */ diff --git a/src/core/census/tag_set.c b/src/core/census/tag_set.c index b985eca52b..8908a2d5f3 100644 --- a/src/core/census/tag_set.c +++ b/src/core/census/tag_set.c @@ -454,19 +454,19 @@ static size_t tag_set_encode(const struct tag_set *tags, char *buffer, } char *census_tag_set_encode(const census_tag_set *tags, char *buffer, - size_t *buf_size, size_t *bin_buf_size) { - size_t p_buf_size = - tag_set_encode(&tags->tags[PROPAGATED_TAGS], buffer, *buf_size); - if (p_buf_size == 0) { + size_t buf_size, size_t *print_buf_size, + size_t *bin_buf_size) { + *print_buf_size = + tag_set_encode(&tags->tags[PROPAGATED_TAGS], buffer, buf_size); + if (*print_buf_size == 0) { return NULL; } - char *b_buffer = buffer + p_buf_size; + char *b_buffer = buffer + *print_buf_size; *bin_buf_size = tag_set_encode(&tags->tags[PROPAGATED_BINARY_TAGS], b_buffer, - *buf_size - p_buf_size); + buf_size - *print_buf_size); if (*bin_buf_size == 0) { return NULL; } - *buf_size = p_buf_size; return b_buffer; } diff --git a/test/core/census/tag_set_test.c b/test/core/census/tag_set_test.c index 4414ad9bc1..8e09e6c1c6 100644 --- a/test/core/census/tag_set_test.c +++ b/test/core/census/tag_set_test.c @@ -325,15 +325,18 @@ static void encode_decode_test(void) { char buffer[BUF_SIZE]; struct census_tag_set *cts = census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); - size_t bsize = 2; // buffer size too small - size_t bin_bsize = 0; - GPR_ASSERT(census_tag_set_encode(cts, buffer, &bsize, &bin_bsize) == NULL); - bsize = BUF_SIZE; - char *b_buffer = census_tag_set_encode(cts, buffer, &bsize, &bin_bsize); - GPR_ASSERT(b_buffer != NULL && bsize > 0 && bin_bsize > 0 && - bsize + bin_bsize <= BUF_SIZE && b_buffer == buffer + bsize); + size_t print_bsize; + size_t bin_bsize; + // Test with too small a buffer + GPR_ASSERT(census_tag_set_encode(cts, buffer, 2, &print_bsize, &bin_bsize) == + NULL); + char *b_buffer = + census_tag_set_encode(cts, buffer, BUF_SIZE, &print_bsize, &bin_bsize); + GPR_ASSERT(b_buffer != NULL && print_bsize > 0 && bin_bsize > 0 && + print_bsize + bin_bsize <= BUF_SIZE && + b_buffer == buffer + print_bsize); census_tag_set *cts2 = - census_tag_set_decode(buffer, bsize, b_buffer, bin_bsize); + census_tag_set_decode(buffer, print_bsize, b_buffer, bin_bsize); GPR_ASSERT(cts2 != NULL); const census_tag_set_create_status *status = census_tag_set_get_create_status(cts2); -- cgit v1.2.3 From 03a38e1dab254d125d00cc3507202f559249c40d Mon Sep 17 00:00:00 2001 From: Alistair Veitch Date: Fri, 22 Jan 2016 17:56:02 -0800 Subject: fix uninitialized variable usage --- src/core/census/tag_set.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/core/census/tag_set.c') diff --git a/src/core/census/tag_set.c b/src/core/census/tag_set.c index 8908a2d5f3..9b65a1dfa1 100644 --- a/src/core/census/tag_set.c +++ b/src/core/census/tag_set.c @@ -253,7 +253,7 @@ static void tag_set_flatten(struct tag_set *tags) { if (tags->ntags == tags->ntags_alloc) return; bool found_deleted = false; // found a deleted tag. char *kvp = tags->kvm; - char *dbase; // record location of deleted tag + char *dbase = NULL; // record location of deleted tag for (int i = 0; i < tags->ntags_alloc; i++) { struct raw_tag tag; char *next_kvp = decode_tag(&tag, kvp, 0); -- cgit v1.2.3