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 <simon.marchi@efficios.com>
Reviewed-on: https://review.lttng.org/c/babeltrace/+/4480
Reviewed-by: Philippe Proulx <eeppeliteloop@gmail.com>
Tested-by: jenkins <jenkins@lttng.org>

diff --git a/src/plugins/ctf/fs-sink/fs-sink-trace.c b/src/plugins/ctf/fs-sink/fs-sink-trace.c
index e9097848c8ab0b624b87a49e8acdd2d77a912c3e..60f668ea3df5929e26eaa41134e2bfb2363febeb 100644 (file)
--- 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 f1bc2e7fc4dd616419347d36e49960473aebe991..7077b6e980072791b9d72775fd82795d60c471ca 100644 (file)
--- 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 (file)
index 0000000..0dd2aed
--- /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 6654b8e32bdfafb9d755f1448d0b729bdb285605..10f5bf0823f7c9822c83e56ff4d9b6d1256c4771 100644 (file)
--- 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 (executable)
index 0000000..e8508dc
--- /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"