ctf_fs_iterator_next and muxer_msg_iter_do_next are written in such a
way that if they successfully accumulate some messages, then hit an
error, they will return OK to make sure that those messages propagate
downstream in the graph, before the error stops the execution. This
assumes that the subsequent call to _next will fail again.
This poses a problem: when it returns OK in this case, it also leaves
the current thread error set. This is a postcondition breach.
One solution would be to simply clear the error in that case. This
would still rely on some error (perhaps the same, perhaps not) to be hit
again on the next _next call. What I don't like with that solution is
that the behaviour of the iterator, if we call _next again after a
failure, is hard to predict. We could hit the same error if we are
lucky, we could hit another error, we could hit an assertion because we
are in an unexpected state, etc.
Instead, this patch implements the following solution: return OK, but
take the error from the current thread and save it in the iterator. The
next time _next is called, that error is moved back to the current
thread and the error status is returned. This ensures that the error
the user will see is the one that originally caused the problem.
A test is added with a trace containing two valid events followed by an
invalid one. We expect to see the two events printed by the details
sink, as well as the error about the invalid event class id.
Change-Id: I725f1de0e6fa0b430aa34bfc524811c5dcd80fa3
Signed-off-by: Simon Marchi <simon.marchi@efficios.com>
Reviewed-on: https://review.lttng.org/c/babeltrace/+/2353
bt_message_array_const msgs, uint64_t capacity,
uint64_t *count)
{
bt_message_array_const msgs, uint64_t capacity,
uint64_t *count)
{
- bt_component_class_message_iterator_next_method_status status =
- BT_COMPONENT_CLASS_MESSAGE_ITERATOR_NEXT_METHOD_STATUS_OK;
+ bt_component_class_message_iterator_next_method_status status;
struct ctf_fs_msg_iter_data *msg_iter_data =
bt_self_message_iterator_get_data(iterator);
uint64_t i = 0;
struct ctf_fs_msg_iter_data *msg_iter_data =
bt_self_message_iterator_get_data(iterator);
uint64_t i = 0;
- while (i < capacity &&
- status == BT_COMPONENT_CLASS_MESSAGE_ITERATOR_NEXT_METHOD_STATUS_OK) {
+ if (G_UNLIKELY(msg_iter_data->next_saved_error)) {
+ /*
+ * Last time we were called, we hit an error but had some
+ * messages to deliver, so we stashed the error here. Return
+ * it now.
+ */
+ BT_CURRENT_THREAD_MOVE_ERROR_AND_RESET(msg_iter_data->next_saved_error);
+ status = msg_iter_data->next_saved_status;
+ goto end;
+ }
+
+ do {
status = ctf_fs_iterator_next_one(msg_iter_data, &msgs[i]);
if (status == BT_COMPONENT_CLASS_MESSAGE_ITERATOR_NEXT_METHOD_STATUS_OK) {
i++;
}
status = ctf_fs_iterator_next_one(msg_iter_data, &msgs[i]);
if (status == BT_COMPONENT_CLASS_MESSAGE_ITERATOR_NEXT_METHOD_STATUS_OK) {
i++;
}
+ } while (i < capacity &&
+ status == BT_COMPONENT_CLASS_MESSAGE_ITERATOR_NEXT_METHOD_STATUS_OK);
* called, possibly without any accumulated
* message, in which case we'll return it.
*/
* called, possibly without any accumulated
* message, in which case we'll return it.
*/
+ if (status < 0) {
+ /*
+ * Save this error for the next _next call. Assume that
+ * this component always appends error causes when
+ * returning an error status code, which will cause the
+ * current thread error to be non-NULL.
+ */
+ msg_iter_data->next_saved_error = bt_current_thread_take_error();
+ BT_ASSERT(msg_iter_data->next_saved_error);
+ msg_iter_data->next_saved_status = status;
+ }
+
*count = i;
status = BT_COMPONENT_CLASS_MESSAGE_ITERATOR_NEXT_METHOD_STATUS_OK;
}
*count = i;
status = BT_COMPONENT_CLASS_MESSAGE_ITERATOR_NEXT_METHOD_STATUS_OK;
}
/* Owned by this */
struct ctf_msg_iter *msg_iter;
/* Owned by this */
struct ctf_msg_iter *msg_iter;
+
+ /*
+ * Saved error. If we hit an error in the _next method, but have some
+ * messages ready to return, we save the error here and return it on
+ * the next _next call.
+ */
+ bt_component_class_message_iterator_next_method_status next_saved_status;
+ const struct bt_error *next_saved_error;
* MUXER_MSG_ITER_CLOCK_CLASS_EXPECTATION_NOT_ABS_SPEC_UUID.
*/
bt_uuid_t expected_clock_class_uuid;
* MUXER_MSG_ITER_CLOCK_CLASS_EXPECTATION_NOT_ABS_SPEC_UUID.
*/
bt_uuid_t expected_clock_class_uuid;
+
+ /*
+ * Saved error. If we hit an error in the _next method, but have some
+ * messages ready to return, we save the error here and return it on
+ * the next _next call.
+ */
+ bt_component_class_message_iterator_next_method_status next_saved_status;
+ const struct bt_error *next_saved_error;
bt_message_array_const msgs, uint64_t capacity,
uint64_t *count)
{
bt_message_array_const msgs, uint64_t capacity,
uint64_t *count)
{
- bt_component_class_message_iterator_next_method_status status =
- BT_COMPONENT_CLASS_MESSAGE_ITERATOR_NEXT_METHOD_STATUS_OK;
+ bt_component_class_message_iterator_next_method_status status;
- while (i < capacity && status == BT_COMPONENT_CLASS_MESSAGE_ITERATOR_NEXT_METHOD_STATUS_OK) {
+ if (G_UNLIKELY(muxer_msg_iter->next_saved_error)) {
+ /*
+ * Last time we were called, we hit an error but had some
+ * messages to deliver, so we stashed the error here. Return
+ * it now.
+ */
+ BT_CURRENT_THREAD_MOVE_ERROR_AND_RESET(muxer_msg_iter->next_saved_error);
+ status = muxer_msg_iter->next_saved_status;
+ goto end;
+ }
+
+ do {
status = muxer_msg_iter_do_next_one(muxer_comp,
muxer_msg_iter, &msgs[i]);
if (status == BT_COMPONENT_CLASS_MESSAGE_ITERATOR_NEXT_METHOD_STATUS_OK) {
i++;
}
status = muxer_msg_iter_do_next_one(muxer_comp,
muxer_msg_iter, &msgs[i]);
if (status == BT_COMPONENT_CLASS_MESSAGE_ITERATOR_NEXT_METHOD_STATUS_OK) {
i++;
}
+ } while (i < capacity && status == BT_COMPONENT_CLASS_MESSAGE_ITERATOR_NEXT_METHOD_STATUS_OK);
* called, possibly without any accumulated
* message, in which case we'll return it.
*/
* called, possibly without any accumulated
* message, in which case we'll return it.
*/
+ if (status < 0) {
+ /*
+ * Save this error for the next _next call. Assume that
+ * this component always appends error causes when
+ * returning an error status code, which will cause the
+ * current thread error to be non-NULL.
+ */
+ muxer_msg_iter->next_saved_error = bt_current_thread_take_error();
+ BT_ASSERT(muxer_msg_iter->next_saved_error);
+ muxer_msg_iter->next_saved_status = status;
+ }
+
*count = i;
status = BT_COMPONENT_CLASS_MESSAGE_ITERATOR_NEXT_METHOD_STATUS_OK;
}
*count = i;
status = BT_COMPONENT_CLASS_MESSAGE_ITERATOR_NEXT_METHOD_STATUS_OK;
}
--- /dev/null
+This trace was written by hand. It contains two valid events, followed by an
+invalid one (the event id is invalid).
--- /dev/null
+\ 1\ 1ÿ
\ No newline at end of file
--- /dev/null
+/* CTF 1.8 */
+
+typealias integer { size = 8; align = 8; signed = false; } := uint8_t;
+
+trace {
+ major = 1;
+ minor = 8;
+ byte_order = be;
+};
+
+stream {
+ event.header := struct {
+ uint8_t id;
+ };
+};
+
+
+event {
+ name = gadoua;
+ id = 1;
+};
--- /dev/null
+Trace class:
+ Stream class (ID 0):
+ Supports packets: Yes
+ Packets have beginning default clock snapshot: No
+ Packets have end default clock snapshot: No
+ Supports discarded events: No
+ Supports discarded packets: No
+ Event class `gadoua` (ID 1):
+
+{Trace 0, Stream class ID 0, Stream ID 0}
+Stream beginning:
+ Trace:
+ Stream (ID 0, Class ID 0)
+
+{Trace 0, Stream class ID 0, Stream ID 0}
+Packet beginning
+
+{Trace 0, Stream class ID 0, Stream ID 0}
+Event `gadoua` (Class ID 1):
+
+{Trace 0, Stream class ID 0, Stream ID 0}
+Event `gadoua` (Class ID 1):
stdout_file=$(mktemp -t test_ctf_fail_stdout.XXXXXX)
stderr_file=$(mktemp -t test_ctf_fail_stderr.XXXXXX)
stdout_file=$(mktemp -t test_ctf_fail_stdout.XXXXXX)
stderr_file=$(mktemp -t test_ctf_fail_stderr.XXXXXX)
+data_dir="${BT_TESTS_SRCDIR}/data/plugins/src.ctf.fs/fail"
test_fail() {
local name="$1"
test_fail() {
local name="$1"
- local expected_error_msg="$2"
+ local expected_stdout_file="$2"
+ local expected_error_msg="$3"
bt_cli "${stdout_file}" "${stderr_file}" \
bt_cli "${stdout_file}" "${stderr_file}" \
- "${fail_trace_dir}/${name}"
+ -c sink.text.details -p "with-trace-name=no,with-stream-name=no" "${fail_trace_dir}/${name}"
isnt $? 0 "Trace ${name}: babeltrace exits with an error"
isnt $? 0 "Trace ${name}: babeltrace exits with an error"
+ bt_diff "${expected_stdout_file}" "${stdout_file}"
+ ok $? "Trace ${name}: babeltrace produces the expected stdout"
+
+ # The expected error message will likely be found in the error stream
+ # even if Babeltrace aborts (e.g. hits an assert). Check that the
+ # Babeltrace CLI finishes gracefully by checking that the error stream
+ # contains an error stack printed by the CLI.
+ grep --silent "^CAUSED BY " "${stderr_file}"
+ ok $? "Trace ${name}: babeltrace produces an error stack"
+
grep --silent "${expected_error_msg}" "${stderr_file}"
ok $? "Trace ${name}: babeltrace produces the expected error message"
}
grep --silent "${expected_error_msg}" "${stderr_file}"
ok $? "Trace ${name}: babeltrace produces the expected error message"
}
+plan_tests 8
+
+test_fail \
+ "invalid-packet-size/trace" \
+ "/dev/null" \
+ "Failed to index CTF stream file '.*channel0_3'"
-test_fail "invalid-packet-size/trace" "Failed to index CTF stream file '.*channel0_3'"
+test_fail \
+ "valid-events-then-invalid-events" \
+ "${data_dir}/valid-events-then-invalid-events.expect" \
+ "No event class with ID of event class ID to use in stream class: .*stream-class-id=0, event-class-id=255"
rm -f "${stdout_file}" "${stderr_file}"
rm -f "${stdout_file}" "${stderr_file}"