From 2a3ced3c38855ea18eb9b29e58563a7301b98216 Mon Sep 17 00:00:00 2001 From: Philippe Proulx Date: Wed, 31 Jan 2018 16:17:19 -0500 Subject: [PATCH] lib: allow a single mapped clock class within a stream class This patch makes it illegal to have more than one mapped clock class within the field types of a stream class and all its event classes. Many field types can map to the same, unique clock class. Multiple clock classes within a given stream class is not a well-defined nor well-supported feature, if it is one in the first place, of CTF. Considering the known use cases we have in the field, it is not used either. A clock's value should be reset to the value of the `timestamp_begin` field of the packet's context for each packet, but this field can only be mapped to a single clock class anyway. What should its initial value be in this case? The specification is silent on this matter. We keep an "expected" clock class in the stream class object. This is only valid when the stream class is frozen; otherwise, the stream class's field types can change, even when event classes are already part of it. If the stream class is already frozen, then if the expected clock class is still NULL (no mapped clock class at this point), an added event class can set it (for future added event classes). If the stream class is eventually part of a trace which was created by a CTF writer, we make sure that the expected clock class is the stream class's clock's class, if any. This is important because, in bt_trace_add_stream_class(), on success, we try to map some special fields to the stream's clock's class. Because we know at this point that the stream's clock's class is also the stream class's expected clock class, it is safe to perform this mapping once the single clock class validation is already done. Signed-off-by: Philippe Proulx --- .../babeltrace/ctf-ir/event-class-internal.h | 5 + .../babeltrace/ctf-ir/stream-class-internal.h | 24 +++ lib/ctf-ir/event-class.c | 48 ++++++ lib/ctf-ir/event.c | 45 ++++++ lib/ctf-ir/stream-class.c | 145 ++++++++++++++++++ lib/ctf-ir/trace.c | 85 ++++++++++ lib/ctf-ir/utils.c | 15 +- 7 files changed, 364 insertions(+), 3 deletions(-) diff --git a/include/babeltrace/ctf-ir/event-class-internal.h b/include/babeltrace/ctf-ir/event-class-internal.h index 8bf9f416..160200bf 100644 --- a/include/babeltrace/ctf-ir/event-class-internal.h +++ b/include/babeltrace/ctf-ir/event-class-internal.h @@ -125,4 +125,9 @@ const char *bt_event_class_log_level_string( } }; +BT_HIDDEN +int bt_event_class_validate_single_clock_class( + struct bt_event_class *event_class, + struct bt_clock_class **expected_clock_class); + #endif /* BABELTRACE_CTF_IR_EVENT_CLASS_INTERNAL_H */ diff --git a/include/babeltrace/ctf-ir/stream-class-internal.h b/include/babeltrace/ctf-ir/stream-class-internal.h index 3bde4ed9..24c75848 100644 --- a/include/babeltrace/ctf-ir/stream-class-internal.h +++ b/include/babeltrace/ctf-ir/stream-class-internal.h @@ -59,6 +59,25 @@ struct bt_stream_class { * stream class is _always_ frozen. */ int valid; + + /* + * Unique clock class mapped to any field type within this + * stream class, including all the stream class's event class + * field types. This is only set if the stream class is frozen. + * + * If the stream class is frozen and this is still NULL, it is + * still possible that it becomes non-NULL because + * bt_stream_class_add_event_class() can add an event class + * containing a field type mapped to some clock class. In this + * case, this is the mapped clock class, and at this point, both + * the new event class and the stream class are frozen, so the + * next added event classes are expected to contain field types + * which only map to this specific clock class. + * + * If this is a CTF writer stream class, then this is the + * backing clock class of the `clock` member above. + */ + struct bt_clock_class *clock_class; }; BT_HIDDEN @@ -87,6 +106,11 @@ int bt_stream_class_map_clock_class( struct bt_field_type *packet_context_type, struct bt_field_type *event_header_type); +BT_HIDDEN +int bt_stream_class_validate_single_clock_class( + struct bt_stream_class *stream_class, + struct bt_clock_class **expected_clock_class); + static inline struct bt_trace *bt_stream_class_borrow_trace( struct bt_stream_class *stream_class) diff --git a/lib/ctf-ir/event-class.c b/lib/ctf-ir/event-class.c index 8ff8bc03..8e685eb7 100644 --- a/lib/ctf-ir/event-class.c +++ b/lib/ctf-ir/event-class.c @@ -38,6 +38,7 @@ #include #include #include +#include #include #include #include @@ -709,3 +710,50 @@ end: BT_PUT(attr_value); return ret; } + +BT_HIDDEN +int bt_event_class_validate_single_clock_class( + struct bt_event_class *event_class, + struct bt_clock_class **expected_clock_class) +{ + int ret = 0; + + assert(event_class); + assert(expected_clock_class); + ret = bt_validate_single_clock_class(event_class->context, + expected_clock_class); + if (ret) { + BT_LOGW("Event class's context field type " + "is not recursively mapped to the " + "expected clock class: " + "event-class-addr=%p, " + "event-class-name=\"%s\", " + "event-class-id=%" PRId64 ", " + "ft-addr=%p", + event_class, + bt_event_class_get_name(event_class), + event_class->id, + event_class->context); + goto end; + } + + ret = bt_validate_single_clock_class(event_class->fields, + expected_clock_class); + if (ret) { + BT_LOGW("Event class's payload field type " + "is not recursively mapped to the " + "expected clock class: " + "event-class-addr=%p, " + "event-class-name=\"%s\", " + "event-class-id=%" PRId64 ", " + "ft-addr=%p", + event_class, + bt_event_class_get_name(event_class), + event_class->id, + event_class->fields); + goto end; + } + +end: + return ret; +} diff --git a/lib/ctf-ir/event.c b/lib/ctf-ir/event.c index 6dc7ad07..fd456374 100644 --- a/lib/ctf-ir/event.c +++ b/lib/ctf-ir/event.c @@ -48,6 +48,7 @@ #include #include #include +#include #include #include #include @@ -78,6 +79,7 @@ struct bt_event *bt_event_create(struct bt_event_class *event_class) struct bt_value *environment = NULL; struct bt_validation_output validation_output = { 0 }; int trace_valid = 0; + struct bt_clock_class *expected_clock_class = NULL; BT_LOGD("Creating event object: event-class-addr=%p, " "event-class-name=\"%s\", event-class-id=%" PRId64, @@ -103,6 +105,40 @@ struct bt_event *bt_event_create(struct bt_event_class *event_class) /* The event class was frozen when added to its stream class */ assert(event_class->frozen); + if (!stream_class->frozen) { + if (stream_class->clock) { + expected_clock_class = + bt_get(stream_class->clock->clock_class); + } + + /* + * Because this function freezes the stream class, + * validate that this stream class contains at most a + * single clock class so that we set its expected clock + * class for future checks. + */ + ret = bt_stream_class_validate_single_clock_class( + stream_class, &expected_clock_class); + if (ret) { + BT_LOGW("Event class's stream class or one of its event " + "classes contains a field type which is not " + "recursively mapped to the expected " + "clock class: " + "stream-class-addr=%p, " + "stream-class-id=%" PRId64 ", " + "stream-class-name=\"%s\", " + "expected-clock-class-addr=%p, " + "expected-clock-class-name=\"%s\"", + stream_class, bt_stream_class_get_id(stream_class), + bt_stream_class_get_name(stream_class), + expected_clock_class, + expected_clock_class ? + bt_clock_class_get_name(expected_clock_class) : + NULL); + goto error; + } + } + /* Validate the trace (if any), the stream class, and the event class */ trace = bt_stream_class_get_trace(stream_class); if (trace) { @@ -242,6 +278,14 @@ struct bt_event *bt_event_create(struct bt_event_class *event_class) */ bt_stream_class_freeze(stream_class); + /* + * It is safe to set the stream class's unique clock class + * now because the stream class is frozen. + */ + if (expected_clock_class) { + BT_MOVE(stream_class->clock_class, expected_clock_class); + } + /* * Mark stream class, and event class as valid since * they're all frozen now. @@ -267,6 +311,7 @@ error: BT_PUT(stream_event_context); BT_PUT(event_context); BT_PUT(event_payload); + bt_put(expected_clock_class); assert(!packet_header_type); assert(!packet_context_type); assert(!event_header_type); diff --git a/lib/ctf-ir/stream-class.c b/lib/ctf-ir/stream-class.c index 3223f505..d1dc9bda 100644 --- a/lib/ctf-ir/stream-class.c +++ b/lib/ctf-ir/stream-class.c @@ -43,6 +43,7 @@ #include #include #include +#include #include #include #include @@ -399,6 +400,7 @@ int bt_stream_class_add_event_class( struct bt_field_type *event_payload_type = NULL; const enum bt_validation_flag validation_flags = BT_VALIDATION_FLAG_EVENT; + struct bt_clock_class *expected_clock_class = NULL; if (!stream_class || !event_class) { BT_LOGW("Invalid parameter: stream class or event class is NULL: " @@ -420,10 +422,55 @@ int bt_stream_class_add_event_class( trace = bt_stream_class_get_trace(stream_class); if (trace && trace->is_static) { + BT_LOGW("Invalid parameter: stream class's trace is static: " + "trace-addr=%p, trace-name=\"%s\"", + trace, bt_trace_get_name(trace)); ret = -1; goto end; } + if (stream_class->frozen) { + /* + * We only check that the event class to be added has a + * single class which matches the stream class's + * expected clock class if the stream class is frozen. + * If it's not, then this event class is added "as is" + * and the validation will be performed when calling + * either bt_trace_add_stream_class() or + * bt_event_create(). This is because the stream class's + * field types (packet context, event header, event + * context) could change before the next call to one of + * those two functions. + */ + expected_clock_class = bt_get(stream_class->clock_class); + + /* + * At this point, `expected_clock_class` can be NULL, + * and bt_event_class_validate_single_clock_class() + * below can set it. + */ + ret = bt_event_class_validate_single_clock_class( + event_class, &expected_clock_class); + if (ret) { + BT_LOGW("Event class contains a field type which is not " + "recursively mapped to its stream class's " + "expected clock class: " + "stream-class-addr=%p, " + "stream-class-id=%" PRId64 ", " + "stream-class-name=\"%s\", " + "expected-clock-class-addr=%p, " + "expected-clock-class-name=\"%s\"", + stream_class, + bt_stream_class_get_id(stream_class), + bt_stream_class_get_name(stream_class), + expected_clock_class, + expected_clock_class ? + bt_clock_class_get_name(expected_clock_class) : + NULL); + goto end; + } + } + event_id = g_new(int64_t, 1); if (!event_id) { BT_LOGE_STR("Failed to allocate one int64_t."); @@ -562,6 +609,16 @@ int bt_stream_class_add_event_class( /* Freeze the event class */ bt_event_class_freeze(event_class); + /* + * It is safe to set the stream class's unique clock class + * now if the stream class is frozen. + */ + if (stream_class->frozen && expected_clock_class) { + assert(!stream_class->clock_class || + stream_class->clock_class == expected_clock_class); + BT_MOVE(stream_class->clock_class, expected_clock_class); + } + /* Notifiy listeners of the trace's schema modification. */ if (trace) { struct bt_visitor_object obj = { .object = event_class, @@ -584,6 +641,7 @@ end: BT_PUT(trace); BT_PUT(old_stream_class); bt_validation_output_put_types(&validation_output); + bt_put(expected_clock_class); assert(!packet_header_type); assert(!packet_context_type); assert(!event_header_type); @@ -1081,6 +1139,7 @@ void bt_stream_class_destroy(struct bt_object *obj) stream_class, bt_stream_class_get_name(stream_class), bt_stream_class_get_id(stream_class)); bt_put(stream_class->clock); + bt_put(stream_class->clock_class); if (stream_class->event_classes_ht) { g_hash_table_destroy(stream_class->event_classes_ht); @@ -1315,3 +1374,89 @@ int bt_stream_class_map_clock_class( end: return ret; } + +BT_HIDDEN +int bt_stream_class_validate_single_clock_class( + struct bt_stream_class *stream_class, + struct bt_clock_class **expected_clock_class) +{ + int ret; + uint64_t i; + + assert(stream_class); + assert(expected_clock_class); + ret = bt_validate_single_clock_class(stream_class->packet_context_type, + expected_clock_class); + if (ret) { + BT_LOGW("Stream class's packet context field type " + "is not recursively mapped to the " + "expected clock class: " + "stream-class-addr=%p, " + "stream-class-name=\"%s\", " + "stream-class-id=%" PRId64 ", " + "ft-addr=%p", + stream_class, + bt_stream_class_get_name(stream_class), + stream_class->id, + stream_class->packet_context_type); + goto end; + } + + ret = bt_validate_single_clock_class(stream_class->event_header_type, + expected_clock_class); + if (ret) { + BT_LOGW("Stream class's event header field type " + "is not recursively mapped to the " + "expected clock class: " + "stream-class-addr=%p, " + "stream-class-name=\"%s\", " + "stream-class-id=%" PRId64 ", " + "ft-addr=%p", + stream_class, + bt_stream_class_get_name(stream_class), + stream_class->id, + stream_class->event_header_type); + goto end; + } + + ret = bt_validate_single_clock_class(stream_class->event_context_type, + expected_clock_class); + if (ret) { + BT_LOGW("Stream class's event context field type " + "is not recursively mapped to the " + "expected clock class: " + "stream-class-addr=%p, " + "stream-class-name=\"%s\", " + "stream-class-id=%" PRId64 ", " + "ft-addr=%p", + stream_class, + bt_stream_class_get_name(stream_class), + stream_class->id, + stream_class->event_context_type); + goto end; + } + + for (i = 0; i < stream_class->event_classes->len; i++) { + struct bt_event_class *event_class = + g_ptr_array_index(stream_class->event_classes, i); + + assert(event_class); + ret = bt_event_class_validate_single_clock_class(event_class, + expected_clock_class); + if (ret) { + BT_LOGW("Stream class's event class contains a " + "field type which is not recursively mapped to " + "the expected clock class: " + "stream-class-addr=%p, " + "stream-class-name=\"%s\", " + "stream-class-id=%" PRId64, + stream_class, + bt_stream_class_get_name(stream_class), + stream_class->id); + goto end; + } + } + +end: + return ret; +} diff --git a/lib/ctf-ir/trace.c b/lib/ctf-ir/trace.c index 7a583862..d1efd9f9 100644 --- a/lib/ctf-ir/trace.c +++ b/lib/ctf-ir/trace.c @@ -1156,6 +1156,7 @@ int bt_trace_add_stream_class(struct bt_trace *trace, struct bt_field_type *stream_event_ctx_type = NULL; int64_t event_class_count; struct bt_trace *current_parent_trace = NULL; + struct bt_clock_class *expected_clock_class = NULL; if (!trace) { BT_LOGW_STR("Invalid parameter: trace is NULL."); @@ -1243,6 +1244,79 @@ int bt_trace_add_stream_class(struct bt_trace *trace, ret = -1; goto end; } + + if (stream_class->clock_class && + stream_class->clock_class != + stream_class->clock->clock_class) { + /* + * Stream class already has an expected clock + * class, but it does not match its clock's + * class. + */ + BT_LOGW("Invalid parameter: stream class's clock's " + "class does not match stream class's " + "expected clock class: " + "stream-class-addr=%p, " + "stream-class-id=%" PRId64 ", " + "stream-class-name=\"%s\", " + "expected-clock-class-addr=%p, " + "expected-clock-class-name=\"%s\"", + stream_class, + bt_stream_class_get_id(stream_class), + bt_stream_class_get_name(stream_class), + expected_clock_class, + bt_clock_class_get_name(expected_clock_class)); + } else if (!stream_class->clock_class) { + /* + * Set expected clock class to stream class's + * clock's class. + */ + expected_clock_class = + bt_get(stream_class->clock->clock_class); + } + } + + if (!stream_class->frozen) { + /* + * Stream class is not frozen yet. Validate that the + * stream class contains at most a single clock class + * because the previous + * bt_stream_class_add_event_class() calls did not make + * this validation since the stream class's direct field + * types (packet context, event header, event context) + * could change afterwards. This stream class is about + * to be frozen and those field types won't be changed + * if this function succeeds. + * + * At this point we're also sure that the stream class's + * clock, if any, has the same class as the stream + * class's expected clock class, if any. This is why, if + * bt_stream_class_validate_single_clock_class() + * succeeds below, the call to + * bt_stream_class_map_clock_class() at the end of this + * function is safe because it maps to the same, single + * clock class. + */ + ret = bt_stream_class_validate_single_clock_class(stream_class, + &expected_clock_class); + if (ret) { + BT_LOGW("Invalid parameter: stream class or one of its " + "event classes contains a field type which is " + "not recursively mapped to the expected " + "clock class: " + "stream-class-addr=%p, " + "stream-class-id=%" PRId64 ", " + "stream-class-name=\"%s\", " + "expected-clock-class-addr=%p, " + "expected-clock-class-name=\"%s\"", + stream_class, bt_stream_class_get_id(stream_class), + bt_stream_class_get_name(stream_class), + expected_clock_class, + expected_clock_class ? + bt_clock_class_get_name(expected_clock_class) : + NULL); + goto end; + } } ret = check_packet_header_type_has_no_clock_class(trace); @@ -1442,6 +1516,8 @@ int bt_trace_add_stream_class(struct bt_trace *trace, } } + + bt_object_set_parent(stream_class, trace); g_ptr_array_add(trace->stream_classes, stream_class); @@ -1486,6 +1562,14 @@ int bt_trace_add_stream_class(struct bt_trace *trace, bt_stream_class_freeze(stream_class); bt_trace_freeze(trace); + /* + * It is safe to set the stream class's unique clock class + * now because the stream class is frozen. + */ + if (expected_clock_class) { + BT_MOVE(stream_class->clock_class, expected_clock_class); + } + /* Notifiy listeners of the trace's schema modification. */ bt_stream_class_visit(stream_class, bt_trace_object_modification, trace); @@ -1512,6 +1596,7 @@ end: g_free(ec_validation_outputs); bt_validation_output_put_types(&trace_sc_validation_output); bt_put(current_parent_trace); + bt_put(expected_clock_class); assert(!packet_header_type); assert(!packet_context_type); assert(!event_header_type); diff --git a/lib/ctf-ir/utils.c b/lib/ctf-ir/utils.c index fba66fd5..8690f025 100644 --- a/lib/ctf-ir/utils.c +++ b/lib/ctf-ir/utils.c @@ -34,6 +34,7 @@ #include #include #include +#include #include static @@ -130,7 +131,11 @@ int bt_validate_single_clock_class(struct bt_field_type *field_type, struct bt_clock_class **expected_clock_class) { int ret = 0; - assert(field_type); + + if (!field_type) { + goto end; + } + assert(expected_clock_class); switch (bt_field_type_get_type_id(field_type)) { @@ -155,9 +160,13 @@ int bt_validate_single_clock_class(struct bt_field_type *field_type, BT_LOGW("Integer field type is not mapped to " "the expected clock class: " "mapped-clock-class-addr=%p, " - "expected-clock-class-addr=%p", + "mapped-clock-class-name=\"%s\", " + "expected-clock-class-addr=%p, " + "expected-clock-class-name=\"%s\"", mapped_clock_class, - *expected_clock_class); + bt_clock_class_get_name(mapped_clock_class), + *expected_clock_class, + bt_clock_class_get_name(*expected_clock_class)); bt_put(mapped_clock_class); ret = -1; goto end; -- 2.34.1