From 72d458a311ee9b8c2cc84d8ade5754e5ea517fcb Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Wed, 2 Dec 2020 16:04:19 -0500 Subject: [PATCH] Fix: sink.ctf.fs: remove spurious directory level when using assume-single-trace The behavior of `sink.ctf.fs` with the `assume-single-trace=true` parameter does not match the documentation nor the comments in the code. The man page says: If the assume-single-trace parameter is true, then the output trace path to use for the single input trace is the directory specified by the path parameter. What I understand from this is that if you pass `path=/tmp/yo`, that should produce the `/tmp/yo/metadata` file and the data files alongside it. The documentation on the `fs_sink_comp::assume_single_trace` says: /* * True if the component assumes that it will only write a * single CTF trace (which can contain one or more data * streams). This makes the component write the stream files * directly in the output directory (`output_dir_path` above). */ bool assume_single_trace; The `output_dir_path` would contain `/tmp/yo`, in the previous example, so that confirms the previous assumption. The actual behavior is that the sink puts the trace in an extra `trace` directory, in `/tmp/yo/trace`. We end up with the `trace` relative trace path (relative to the base output directory) because `make_trace_path_rel` returns an empty string when `assume_single_trace` is true and `sanitize_trace_path` replaces it with `trace`. When using `assume-single-trace=true`, we should not deal with a relative trace path, as the trace is output directly in the base output directory. We also don't want to run the path in `make_unique_trace_path`, as the trace should be output in the base directory specified by the user, not another directory. Anyway, if the user specifies an existing directory, `sink.ctf.fs` will initially error out with: Single trace mode, but output path exists: output-path="/tmp/yo" Although that's a TOCTOU bug, two babeltrace instances could both check at the same time that the same output directory does not exist, and both write in the same directory. That would not be good. Change-Id: Ib2415420eabbc096a920d113863993161105e90a Signed-off-by: Simon Marchi Reviewed-on: https://review.lttng.org/c/babeltrace/+/4480 Reviewed-by: Philippe Proulx Tested-by: jenkins --- src/plugins/ctf/fs-sink/fs-sink-trace.c | 44 ++++++----- tests/Makefile.am | 1 + .../assume-single-trace/bt_plugin_foo.py | 38 ++++++++++ tests/plugins/sink.ctf.fs/Makefile.am | 3 + .../sink.ctf.fs/test_assume_single_trace | 74 +++++++++++++++++++ 5 files changed, 140 insertions(+), 20 deletions(-) create mode 100644 tests/data/plugins/sink.ctf.fs/assume-single-trace/bt_plugin_foo.py create mode 100755 tests/plugins/sink.ctf.fs/test_assume_single_trace diff --git a/src/plugins/ctf/fs-sink/fs-sink-trace.c b/src/plugins/ctf/fs-sink/fs-sink-trace.c index e9097848..60f668ea 100644 --- a/src/plugins/ctf/fs-sink/fs-sink-trace.c +++ b/src/plugins/ctf/fs-sink/fs-sink-trace.c @@ -393,11 +393,7 @@ GString *make_trace_path_rel(const struct fs_sink_trace *trace) GString *path = NULL; const char *trace_name; - if (trace->fs_sink->assume_single_trace) { - /* Use output directory directly */ - path = g_string_new(""); - goto end; - } + BT_ASSERT(!trace->fs_sink->assume_single_trace); /* First, try to build a path using environment fields written by LTTng. */ path = make_lttng_trace_path_rel(trace); @@ -431,25 +427,33 @@ GString *make_trace_path(const struct fs_sink_trace *trace, const char *output_b GString *full_path = NULL; GString *unique_full_path = NULL; - rel_path = make_trace_path_rel(trace); - if (!rel_path) { - goto end; - } + if (trace->fs_sink->assume_single_trace) { + /* Use output directory directly */ + unique_full_path = g_string_new(output_base_directory); + if (!unique_full_path) { + goto end; + } + } else { + rel_path = make_trace_path_rel(trace); + if (!rel_path) { + goto end; + } - rel_path_san = sanitize_trace_path(rel_path->str); - if (!rel_path_san) { - goto end; - } + rel_path_san = sanitize_trace_path(rel_path->str); + if (!rel_path_san) { + goto end; + } - full_path = g_string_new(NULL); - if (!full_path) { - goto end; - } + full_path = g_string_new(NULL); + if (!full_path) { + goto end; + } - g_string_printf(full_path, "%s" G_DIR_SEPARATOR_S "%s", - output_base_directory, rel_path_san->str); + g_string_printf(full_path, "%s" G_DIR_SEPARATOR_S "%s", + output_base_directory, rel_path_san->str); - unique_full_path = make_unique_trace_path(full_path->str); + unique_full_path = make_unique_trace_path(full_path->str); + } end: if (rel_path) { diff --git a/tests/Makefile.am b/tests/Makefile.am index f1bc2e7f..7077b6e9 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -109,6 +109,7 @@ if ENABLE_PYTHON_BINDINGS TESTS_PLUGINS += plugins/src.ctf.fs/query/test_query_support_info TESTS_PLUGINS += plugins/src.ctf.fs/query/test_query_trace_info TESTS_PLUGINS += plugins/src.ctf.fs/query/test_query_metadata_info +TESTS_PLUGINS += plugins/sink.ctf.fs/test_assume_single_trace endif endif diff --git a/tests/data/plugins/sink.ctf.fs/assume-single-trace/bt_plugin_foo.py b/tests/data/plugins/sink.ctf.fs/assume-single-trace/bt_plugin_foo.py new file mode 100644 index 00000000..0dd2aed6 --- /dev/null +++ b/tests/data/plugins/sink.ctf.fs/assume-single-trace/bt_plugin_foo.py @@ -0,0 +1,38 @@ +# SPDX-License-Identifier: GPL-2.0-only +# +# Copyright (C) 2020 EfficiOS Inc. +# + +import bt2 + + +class TheSourceIterator(bt2._UserMessageIterator): + def __init__(self, config, port): + tc, sc, ec = port.user_data + + trace = tc() + stream = trace.create_stream(sc, name='the-stream') + + self._msgs = [ + self._create_stream_beginning_message(stream), + self._create_event_message(ec, stream), + self._create_stream_end_message(stream), + ] + + def __next__(self): + if len(self._msgs) == 0: + raise StopIteration + + return self._msgs.pop(0) + + +@bt2.plugin_component_class +class TheSource(bt2._UserSourceComponent, message_iterator_class=TheSourceIterator): + def __init__(self, config, params, obj): + tc = self._create_trace_class() + sc = tc.create_stream_class() + ec = sc.create_event_class(name='the-event') + self._add_output_port('out', user_data=(tc, sc, ec)) + + +bt2.register_plugin(__name__, "foo") diff --git a/tests/plugins/sink.ctf.fs/Makefile.am b/tests/plugins/sink.ctf.fs/Makefile.am index 6654b8e3..10f5bf08 100644 --- a/tests/plugins/sink.ctf.fs/Makefile.am +++ b/tests/plugins/sink.ctf.fs/Makefile.am @@ -1,3 +1,6 @@ # SPDX-License-Identifier: MIT SUBDIRS = succeed + +dist_check_SCRIPTS = \ + test_assume_single_trace diff --git a/tests/plugins/sink.ctf.fs/test_assume_single_trace b/tests/plugins/sink.ctf.fs/test_assume_single_trace new file mode 100755 index 00000000..e8508dc3 --- /dev/null +++ b/tests/plugins/sink.ctf.fs/test_assume_single_trace @@ -0,0 +1,74 @@ +#!/bin/bash +# +# SPDX-License-Identifier: GPL-2.0-only +# +# Copyright (C) 2020 EfficiOS Inc. +# + +# This file tests the assume-single-trace parameter of sink.ctf.fs. + +SH_TAP=1 + +if [ "x${BT_TESTS_SRCDIR:-}" != "x" ]; then + UTILSSH="$BT_TESTS_SRCDIR/utils/utils.sh" +else + UTILSSH="$(dirname "$0")/../../utils/utils.sh" +fi + +# shellcheck source=../../utils/utils.sh +source "$UTILSSH" + +# Directory containing the Python test source. +data_dir="$BT_TESTS_DATADIR/plugins/sink.ctf.fs/assume-single-trace" + +temp_stdout=$(mktemp) +temp_expected_stdout=$(mktemp) +temp_stderr=$(mktemp) +temp_output_dir=$(mktemp -d) + +trace_dir="$temp_output_dir/the-trace" + +plan_tests 7 + +bt_cli "$temp_stdout" "$temp_stderr" \ + "--plugin-path=${data_dir}" \ + -c src.foo.TheSource \ + -c sink.ctf.fs -p "path=\"${trace_dir}\"" -p 'assume-single-trace=true' +ok "$?" "run sink.ctf.fs with assume-single-trace=true" + +# Check stdout. +if [ "$BT_OS_TYPE" = "mingw" ]; then + echo "Created CTF trace \`$(cygpath -m ${trace_dir})\`." > "$temp_expected_stdout" +else + echo "Created CTF trace \`${trace_dir}\`." > "$temp_expected_stdout" +fi +bt_diff "$temp_expected_stdout" "$temp_stdout" +ok "$?" "expected message on stdout" + +# Check stderr. +bt_diff "/dev/null" "$temp_stderr" +ok "$?" "stderr is empty" + +# Verify only the expected files exist. +files=("$trace_dir"/*) +num_files=${#files[@]} +is "$num_files" "2" "expected number of files in output directory" + +test -f "$trace_dir/metadata" +ok "$?" "metadata file exists" + +test -f "$trace_dir/the-stream" +ok "$?" "the-stream file exists" + +# Read back the output trace to make sure it's properly formed. +echo "the-event: " > "$temp_expected_stdout" +bt_diff_cli "$temp_expected_stdout" /dev/null "$trace_dir" +ok "$?" "read back output trace" + +rm -f "$temp_stdout" +rm -f "$temp_stderr" +rm -f "$temp_expected_stdout" +rm -f "$trace_dir/metadata" +rm -f "$trace_dir/the-stream" +rmdir "$trace_dir" +rmdir "$temp_output_dir" -- 2.34.1