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>
Reviewed-on: https://review.lttng.org/c/babeltrace/+/4491

diff --git a/src/plugins/ctf/fs-sink/fs-sink-trace.c b/src/plugins/ctf/fs-sink/fs-sink-trace.c
index c21c168f6c72ae5fbf4e1d329e598b8863bf1f9a..223293dc173db8b629c3b63657e9071cef24c9ec 100644 (file)
--- a/src/plugins/ctf/fs-sink/fs-sink-trace.c
+++ b/src/plugins/ctf/fs-sink/fs-sink-trace.c
@@ -409,11 +409,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);
@@ -447,25 +443,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 28ac0c91a918a5ff78130f19ad68ba776a2ddf2e..02a93eb88a7231e3d033a2ae402a9dcd668589e2 100644 (file)
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -106,6 +106,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 7daaeedb06a1cac269db705f0350ad5a3787803a..76ce3c031e9cfb205081a6e64987676a53675344 100644 (file)
--- a/tests/plugins/sink.ctf.fs/Makefile.am
+++ b/tests/plugins/sink.ctf.fs/Makefile.am
@@ -1 +1,4 @@
 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"