diff mbox series

[RFC,babeltrace,1/2] fs-src: add argument for metadata src dir

Message ID 20180430213107.10666-2-aring@mojatatu.com
State RFC, archived
Headers show
Series fs-src: add metadata-src parameter | expand

Commit Message

Alexander Aring April 30, 2018, 9:31 p.m. UTC
This patch adds an argument for the fs-src plugin to declare the
directory to find the metadata file instead of placing it in every
subdir of traces.

If parameter assign every subdirectory which does not have a
subdirectory and at least some regular files will be assumed as a trace
directory. The regular files will be assumed as ctf trace files.

Signed-off-by: Alexander Aring <aring at mojatatu.com>
---
 cli/babeltrace-cfg-cli-args.c | 10 +++++
 plugins/ctf/fs-src/fs.c       | 88 +++++++++++++++++++++++++++++++++++--------
 plugins/ctf/fs-src/fs.h       |  3 +-
 plugins/ctf/fs-src/metadata.c |  8 +++-
 plugins/ctf/fs-src/metadata.h |  1 +
 plugins/ctf/fs-src/query.c    |  2 +-
 6 files changed, 94 insertions(+), 18 deletions(-)

Comments

Philippe Proulx April 30, 2018, 10:36 p.m. UTC | #1
On Mon, Apr 30, 2018 at 5:31 PM, Alexander Aring <aring at mojatatu.com> wrote:
>
> This patch adds an argument for the fs-src plugin to declare the
> directory to find the metadata file instead of placing it in every
> subdir of traces.
>
> If parameter assign every subdirectory which does not have a
> subdirectory and at least some regular files will be assumed as a trace
> directory. The regular files will be assumed as ctf trace files.

Although I really appreciate the contribution effort, before even
reading this patch, let me indicate that a CTF trace located on the file
system is a directory containing the data stream and metadata stream
files. Knowing this, this patch implements a hack to circumvent this,
but I'm personally not willing to have this upstream as, in my opinion,
it is the user's responsibility to have valid CTF traces to open.

Your custom scenario asks for a custom solution on your side: copying
the metadata file to each trace's directory seems appropriate here.

Also, do your CTF traces have UUIDs? If they do, they should all be
different, but having the same metadata file makes them all the same.
This is not valid either.

Phil

>
> Signed-off-by: Alexander Aring <aring at mojatatu.com>
> ---
>  cli/babeltrace-cfg-cli-args.c | 10 +++++
>  plugins/ctf/fs-src/fs.c       | 88 +++++++++++++++++++++++++++++++++++--------
>  plugins/ctf/fs-src/fs.h       |  3 +-
>  plugins/ctf/fs-src/metadata.c |  8 +++-
>  plugins/ctf/fs-src/metadata.h |  1 +
>  plugins/ctf/fs-src/query.c    |  2 +-
>  6 files changed, 94 insertions(+), 18 deletions(-)
>
> diff --git a/cli/babeltrace-cfg-cli-args.c b/cli/babeltrace-cfg-cli-args.c
> index 8f01e64b..ab76852e 100644
> --- a/cli/babeltrace-cfg-cli-args.c
> +++ b/cli/babeltrace-cfg-cli-args.c
> @@ -1306,6 +1306,7 @@ enum {
>         OPT_CLOCK_GMT,
>         OPT_CLOCK_OFFSET,
>         OPT_CLOCK_OFFSET_NS,
> +       OPT_METADATA_SRC,
>         OPT_CLOCK_SECONDS,
>         OPT_COLOR,
>         OPT_COMPONENT,
> @@ -2789,6 +2790,7 @@ void print_convert_usage(FILE *fp)
>         fprintf(fp, "\n");
>         fprintf(fp, "      --clock-offset=SEC            Set clock offset to SEC seconds\n");
>         fprintf(fp, "      --clock-offset-ns=NS          Set clock offset to NS ns\n");
> +       fprintf(fp, "      --metadata-src=PATH           Set a path to find the metadata\n");
>         fprintf(fp, "\n");
>         fprintf(fp, "Implicit `sink.text.pretty` component options:\n");
>         fprintf(fp, "\n");
> @@ -2886,6 +2888,7 @@ struct poptOption convert_long_options[] = {
>         { "clock-gmt", '\0', POPT_ARG_NONE, NULL, OPT_CLOCK_GMT, NULL, NULL },
>         { "clock-offset", '\0', POPT_ARG_STRING, NULL, OPT_CLOCK_OFFSET, NULL, NULL },
>         { "clock-offset-ns", '\0', POPT_ARG_STRING, NULL, OPT_CLOCK_OFFSET_NS, NULL, NULL },
> +       { "metadata-src", '\0', POPT_ARG_STRING, NULL, OPT_METADATA_SRC, NULL, NULL },
>         { "clock-seconds", '\0', POPT_ARG_NONE, NULL, OPT_CLOCK_SECONDS, NULL, NULL },
>         { "color", '\0', POPT_ARG_STRING, NULL, OPT_COLOR, NULL, NULL },
>         { "component", 'c', POPT_ARG_STRING, NULL, OPT_COMPONENT, NULL, NULL },
> @@ -3942,6 +3945,7 @@ struct bt_config *bt_config_convert_from_args(int argc, const char *argv[],
>                 case OPT_CLOCK_GMT:
>                 case OPT_CLOCK_OFFSET:
>                 case OPT_CLOCK_OFFSET_NS:
> +               case OPT_METADATA_SRC:
>                 case OPT_CLOCK_SECONDS:
>                 case OPT_COLOR:
>                 case OPT_DEBUG:
> @@ -4104,6 +4108,12 @@ struct bt_config *bt_config_convert_from_args(int argc, const char *argv[],
>                                         &base_implicit_ctf_input_args,
>                                         "clock-class-offset-ns", arg);
>                         break;
> +               case OPT_METADATA_SRC:
> +                       base_implicit_ctf_input_args.exists = true;
> +                       append_implicit_component_param(
> +                                       &base_implicit_ctf_input_args,
> +                                       "metadata-src", arg);
> +                       break;
>                 case OPT_CLOCK_SECONDS:
>                         append_implicit_component_param(
>                                 &implicit_text_args, "clock-seconds", "yes");
> diff --git a/plugins/ctf/fs-src/fs.c b/plugins/ctf/fs-src/fs.c
> index 2dacf97d..7f98dda5 100644
> --- a/plugins/ctf/fs-src/fs.c
> +++ b/plugins/ctf/fs-src/fs.c
> @@ -1039,26 +1039,70 @@ end:
>  }
>
>  BT_HIDDEN
> -int ctf_fs_find_traces(GList **trace_paths, const char *start_path)
> +int ctf_fs_find_traces(GList **trace_paths, const char *start_path,
> +                      const char *metadata_src)
>  {
>         int ret;
>         GError *error = NULL;
>         GDir *dir = NULL;
>         const char *basename = NULL;
> +       bool subdirs = false;
> +       bool regfile = false;
>
> -       /* Check if the starting path is a CTF trace itself */
> -       ret = path_is_ctf_trace(start_path);
> -       if (ret < 0) {
> -               goto end;
> -       }
> +       if (metadata_src) {
> +               ret = path_is_ctf_trace(metadata_src);
> +               if (ret < 0) {
> +                       goto end;
> +               }
>
> -       if (ret) {
> -               /*
> -                * Stop recursion: a CTF trace cannot contain another
> -                * CTF trace.
> -                */
> -               ret = add_trace_path(trace_paths, start_path);
> -               goto end;
> +               if (ret) {
> +                       dir = g_dir_open(start_path, 0, &error);
> +                       if (!dir) {
> +                               goto end;
> +                       }
> +
> +                       while ((basename = g_dir_read_name(dir))) {
> +                               GString *sub_path = g_string_new(NULL);
> +
> +                               if (!sub_path) {
> +                                       ret = -1;
> +                                       goto end;
> +                               }
> +
> +                               g_string_printf(sub_path, "%s" G_DIR_SEPARATOR_S "%s", start_path, basename);
> +                               if (g_file_test(sub_path->str, G_FILE_TEST_IS_DIR)) {
> +                                       subdirs = true;
> +                               }
> +                               if (g_file_test(sub_path->str, G_FILE_TEST_IS_REGULAR)) {
> +                                       regfile = true;
> +                               }
> +                               g_string_free(sub_path, TRUE);
> +                       }
> +
> +                       g_dir_close(dir);
> +                       dir = NULL;
> +
> +                       /* Look if dir has no subdirs but regfile(s) */
> +                       if (!subdirs && regfile) {
> +                               ret = add_trace_path(trace_paths, start_path);
> +                               goto end;
> +                       }
> +               }
> +       } else {
> +               /* Check if the starting path is a CTF trace itself */
> +               ret = path_is_ctf_trace(start_path);
> +               if (ret < 0) {
> +                       goto end;
> +               }
> +
> +               if (ret) {
> +                       /*
> +                        * Stop recursion: a CTF trace cannot contain another
> +                        * CTF trace.
> +                        */
> +                       ret = add_trace_path(trace_paths, start_path);
> +                       goto end;
> +               }
>         }
>
>         /* Look for subdirectories */
> @@ -1090,7 +1134,7 @@ int ctf_fs_find_traces(GList **trace_paths, const char *start_path)
>                 }
>
>                 g_string_printf(sub_path, "%s" G_DIR_SEPARATOR_S "%s", start_path, basename);
> -               ret = ctf_fs_find_traces(trace_paths, sub_path->str);
> +               ret = ctf_fs_find_traces(trace_paths, sub_path->str, metadata_src);
>                 g_string_free(sub_path, TRUE);
>                 if (ret) {
>                         goto end;
> @@ -1181,7 +1225,8 @@ int create_ctf_fs_traces(struct ctf_fs_component *ctf_fs,
>                 goto error;
>         }
>
> -       ret = ctf_fs_find_traces(&trace_paths, norm_path->str);
> +       ret = ctf_fs_find_traces(&trace_paths, norm_path->str,
> +                                ctf_fs->metadata_config.metadata_src);
>         if (ret) {
>                 goto error;
>         }
> @@ -1287,6 +1332,19 @@ struct ctf_fs_component *ctf_fs_create(struct bt_private_component *priv_comp,
>         value_ret = bt_value_string_get(value, &path_param);
>         assert(value_ret == BT_VALUE_STATUS_OK);
>         BT_PUT(value);
> +
> +       value = bt_value_map_get(params, "metadata-src");
> +       if (value) {
> +               if (!bt_value_is_string(value)) {
> +                       goto error;
> +               }
> +
> +               value_ret = bt_value_string_get(value,
> +                       &ctf_fs->metadata_config.metadata_src);
> +               assert(value_ret == BT_VALUE_STATUS_OK);
> +               BT_PUT(value);
> +       }
> +
>         value = bt_value_map_get(params, "clock-class-offset-s");
>         if (value) {
>                 if (!bt_value_is_integer(value)) {
> diff --git a/plugins/ctf/fs-src/fs.h b/plugins/ctf/fs-src/fs.h
> index bbac1bb4..f80aea74 100644
> --- a/plugins/ctf/fs-src/fs.h
> +++ b/plugins/ctf/fs-src/fs.h
> @@ -154,7 +154,8 @@ BT_HIDDEN
>  void ctf_fs_trace_destroy(struct ctf_fs_trace *trace);
>
>  BT_HIDDEN
> -int ctf_fs_find_traces(GList **trace_paths, const char *start_path);
> +int ctf_fs_find_traces(GList **trace_paths, const char *start_path,
> +                      const char *metadata_src);
>
>  BT_HIDDEN
>  GList *ctf_fs_create_trace_names(GList *trace_paths, const char *base_path);
> diff --git a/plugins/ctf/fs-src/metadata.c b/plugins/ctf/fs-src/metadata.c
> index 231d946c..02cbd0cd 100644
> --- a/plugins/ctf/fs-src/metadata.c
> +++ b/plugins/ctf/fs-src/metadata.c
> @@ -96,8 +96,14 @@ int ctf_fs_metadata_set_trace(struct ctf_fs_trace *ctf_fs_trace,
>                 .clock_class_offset_s = config ? config->clock_class_offset_s : 0,
>                 .clock_class_offset_ns = config ? config->clock_class_offset_ns : 0,
>         };
> +       const char *metadata_src;
>
> -       file = get_file(ctf_fs_trace->path->str);
> +       if (config->metadata_src)
> +               metadata_src = config->metadata_src;
> +       else
> +               metadata_src = ctf_fs_trace->path->str;
> +
> +       file = get_file(metadata_src);
>         if (!file) {
>                 BT_LOGE("Cannot create metadata file object");
>                 ret = -1;
> diff --git a/plugins/ctf/fs-src/metadata.h b/plugins/ctf/fs-src/metadata.h
> index 496a5ca9..00d8de67 100644
> --- a/plugins/ctf/fs-src/metadata.h
> +++ b/plugins/ctf/fs-src/metadata.h
> @@ -36,6 +36,7 @@ struct ctf_fs_metadata;
>  struct ctf_fs_metadata_config {
>         int64_t clock_class_offset_s;
>         int64_t clock_class_offset_ns;
> +       const char *metadata_src;
>  };
>
>  BT_HIDDEN
> diff --git a/plugins/ctf/fs-src/query.c b/plugins/ctf/fs-src/query.c
> index 04bf8c5b..6f2aa5fe 100644
> --- a/plugins/ctf/fs-src/query.c
> +++ b/plugins/ctf/fs-src/query.c
> @@ -500,7 +500,7 @@ struct bt_component_class_query_method_return trace_info_query(
>         }
>         assert(path);
>
> -       ret = ctf_fs_find_traces(&trace_paths, normalized_path->str);
> +       ret = ctf_fs_find_traces(&trace_paths, normalized_path->str, NULL);
>         if (ret) {
>                 goto error;
>         }
> --
> 2.11.0
>
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
Philippe Proulx May 1, 2018, 2:41 p.m. UTC | #2
On Mon, Apr 30, 2018 at 6:36 PM, Philippe Proulx
<eeppeliteloop at gmail.com> wrote:
> On Mon, Apr 30, 2018 at 5:31 PM, Alexander Aring <aring at mojatatu.com> wrote:
>>
>> This patch adds an argument for the fs-src plugin to declare the
>> directory to find the metadata file instead of placing it in every
>> subdir of traces.
>>
>> If parameter assign every subdirectory which does not have a
>> subdirectory and at least some regular files will be assumed as a trace
>> directory. The regular files will be assumed as ctf trace files.
>
> Although I really appreciate the contribution effort, before even
> reading this patch, let me indicate that a CTF trace located on the file
> system is a directory containing the data stream and metadata stream
> files. Knowing this, this patch implements a hack to circumvent this,
> but I'm personally not willing to have this upstream as, in my opinion,
> it is the user's responsibility to have valid CTF traces to open.
>
> Your custom scenario asks for a custom solution on your side: copying
> the metadata file to each trace's directory seems appropriate here.

Just had a thought: you can also create symbolic links in each trace's
directory to your common metadata file.

Phil

>
> Also, do your CTF traces have UUIDs? If they do, they should all be
> different, but having the same metadata file makes them all the same.
> This is not valid either.
>
> Phil
>
>>
>> Signed-off-by: Alexander Aring <aring at mojatatu.com>
>> ---
>>  cli/babeltrace-cfg-cli-args.c | 10 +++++
>>  plugins/ctf/fs-src/fs.c       | 88 +++++++++++++++++++++++++++++++++++--------
>>  plugins/ctf/fs-src/fs.h       |  3 +-
>>  plugins/ctf/fs-src/metadata.c |  8 +++-
>>  plugins/ctf/fs-src/metadata.h |  1 +
>>  plugins/ctf/fs-src/query.c    |  2 +-
>>  6 files changed, 94 insertions(+), 18 deletions(-)
>>
>> diff --git a/cli/babeltrace-cfg-cli-args.c b/cli/babeltrace-cfg-cli-args.c
>> index 8f01e64b..ab76852e 100644
>> --- a/cli/babeltrace-cfg-cli-args.c
>> +++ b/cli/babeltrace-cfg-cli-args.c
>> @@ -1306,6 +1306,7 @@ enum {
>>         OPT_CLOCK_GMT,
>>         OPT_CLOCK_OFFSET,
>>         OPT_CLOCK_OFFSET_NS,
>> +       OPT_METADATA_SRC,
>>         OPT_CLOCK_SECONDS,
>>         OPT_COLOR,
>>         OPT_COMPONENT,
>> @@ -2789,6 +2790,7 @@ void print_convert_usage(FILE *fp)
>>         fprintf(fp, "\n");
>>         fprintf(fp, "      --clock-offset=SEC            Set clock offset to SEC seconds\n");
>>         fprintf(fp, "      --clock-offset-ns=NS          Set clock offset to NS ns\n");
>> +       fprintf(fp, "      --metadata-src=PATH           Set a path to find the metadata\n");
>>         fprintf(fp, "\n");
>>         fprintf(fp, "Implicit `sink.text.pretty` component options:\n");
>>         fprintf(fp, "\n");
>> @@ -2886,6 +2888,7 @@ struct poptOption convert_long_options[] = {
>>         { "clock-gmt", '\0', POPT_ARG_NONE, NULL, OPT_CLOCK_GMT, NULL, NULL },
>>         { "clock-offset", '\0', POPT_ARG_STRING, NULL, OPT_CLOCK_OFFSET, NULL, NULL },
>>         { "clock-offset-ns", '\0', POPT_ARG_STRING, NULL, OPT_CLOCK_OFFSET_NS, NULL, NULL },
>> +       { "metadata-src", '\0', POPT_ARG_STRING, NULL, OPT_METADATA_SRC, NULL, NULL },
>>         { "clock-seconds", '\0', POPT_ARG_NONE, NULL, OPT_CLOCK_SECONDS, NULL, NULL },
>>         { "color", '\0', POPT_ARG_STRING, NULL, OPT_COLOR, NULL, NULL },
>>         { "component", 'c', POPT_ARG_STRING, NULL, OPT_COMPONENT, NULL, NULL },
>> @@ -3942,6 +3945,7 @@ struct bt_config *bt_config_convert_from_args(int argc, const char *argv[],
>>                 case OPT_CLOCK_GMT:
>>                 case OPT_CLOCK_OFFSET:
>>                 case OPT_CLOCK_OFFSET_NS:
>> +               case OPT_METADATA_SRC:
>>                 case OPT_CLOCK_SECONDS:
>>                 case OPT_COLOR:
>>                 case OPT_DEBUG:
>> @@ -4104,6 +4108,12 @@ struct bt_config *bt_config_convert_from_args(int argc, const char *argv[],
>>                                         &base_implicit_ctf_input_args,
>>                                         "clock-class-offset-ns", arg);
>>                         break;
>> +               case OPT_METADATA_SRC:
>> +                       base_implicit_ctf_input_args.exists = true;
>> +                       append_implicit_component_param(
>> +                                       &base_implicit_ctf_input_args,
>> +                                       "metadata-src", arg);
>> +                       break;
>>                 case OPT_CLOCK_SECONDS:
>>                         append_implicit_component_param(
>>                                 &implicit_text_args, "clock-seconds", "yes");
>> diff --git a/plugins/ctf/fs-src/fs.c b/plugins/ctf/fs-src/fs.c
>> index 2dacf97d..7f98dda5 100644
>> --- a/plugins/ctf/fs-src/fs.c
>> +++ b/plugins/ctf/fs-src/fs.c
>> @@ -1039,26 +1039,70 @@ end:
>>  }
>>
>>  BT_HIDDEN
>> -int ctf_fs_find_traces(GList **trace_paths, const char *start_path)
>> +int ctf_fs_find_traces(GList **trace_paths, const char *start_path,
>> +                      const char *metadata_src)
>>  {
>>         int ret;
>>         GError *error = NULL;
>>         GDir *dir = NULL;
>>         const char *basename = NULL;
>> +       bool subdirs = false;
>> +       bool regfile = false;
>>
>> -       /* Check if the starting path is a CTF trace itself */
>> -       ret = path_is_ctf_trace(start_path);
>> -       if (ret < 0) {
>> -               goto end;
>> -       }
>> +       if (metadata_src) {
>> +               ret = path_is_ctf_trace(metadata_src);
>> +               if (ret < 0) {
>> +                       goto end;
>> +               }
>>
>> -       if (ret) {
>> -               /*
>> -                * Stop recursion: a CTF trace cannot contain another
>> -                * CTF trace.
>> -                */
>> -               ret = add_trace_path(trace_paths, start_path);
>> -               goto end;
>> +               if (ret) {
>> +                       dir = g_dir_open(start_path, 0, &error);
>> +                       if (!dir) {
>> +                               goto end;
>> +                       }
>> +
>> +                       while ((basename = g_dir_read_name(dir))) {
>> +                               GString *sub_path = g_string_new(NULL);
>> +
>> +                               if (!sub_path) {
>> +                                       ret = -1;
>> +                                       goto end;
>> +                               }
>> +
>> +                               g_string_printf(sub_path, "%s" G_DIR_SEPARATOR_S "%s", start_path, basename);
>> +                               if (g_file_test(sub_path->str, G_FILE_TEST_IS_DIR)) {
>> +                                       subdirs = true;
>> +                               }
>> +                               if (g_file_test(sub_path->str, G_FILE_TEST_IS_REGULAR)) {
>> +                                       regfile = true;
>> +                               }
>> +                               g_string_free(sub_path, TRUE);
>> +                       }
>> +
>> +                       g_dir_close(dir);
>> +                       dir = NULL;
>> +
>> +                       /* Look if dir has no subdirs but regfile(s) */
>> +                       if (!subdirs && regfile) {
>> +                               ret = add_trace_path(trace_paths, start_path);
>> +                               goto end;
>> +                       }
>> +               }
>> +       } else {
>> +               /* Check if the starting path is a CTF trace itself */
>> +               ret = path_is_ctf_trace(start_path);
>> +               if (ret < 0) {
>> +                       goto end;
>> +               }
>> +
>> +               if (ret) {
>> +                       /*
>> +                        * Stop recursion: a CTF trace cannot contain another
>> +                        * CTF trace.
>> +                        */
>> +                       ret = add_trace_path(trace_paths, start_path);
>> +                       goto end;
>> +               }
>>         }
>>
>>         /* Look for subdirectories */
>> @@ -1090,7 +1134,7 @@ int ctf_fs_find_traces(GList **trace_paths, const char *start_path)
>>                 }
>>
>>                 g_string_printf(sub_path, "%s" G_DIR_SEPARATOR_S "%s", start_path, basename);
>> -               ret = ctf_fs_find_traces(trace_paths, sub_path->str);
>> +               ret = ctf_fs_find_traces(trace_paths, sub_path->str, metadata_src);
>>                 g_string_free(sub_path, TRUE);
>>                 if (ret) {
>>                         goto end;
>> @@ -1181,7 +1225,8 @@ int create_ctf_fs_traces(struct ctf_fs_component *ctf_fs,
>>                 goto error;
>>         }
>>
>> -       ret = ctf_fs_find_traces(&trace_paths, norm_path->str);
>> +       ret = ctf_fs_find_traces(&trace_paths, norm_path->str,
>> +                                ctf_fs->metadata_config.metadata_src);
>>         if (ret) {
>>                 goto error;
>>         }
>> @@ -1287,6 +1332,19 @@ struct ctf_fs_component *ctf_fs_create(struct bt_private_component *priv_comp,
>>         value_ret = bt_value_string_get(value, &path_param);
>>         assert(value_ret == BT_VALUE_STATUS_OK);
>>         BT_PUT(value);
>> +
>> +       value = bt_value_map_get(params, "metadata-src");
>> +       if (value) {
>> +               if (!bt_value_is_string(value)) {
>> +                       goto error;
>> +               }
>> +
>> +               value_ret = bt_value_string_get(value,
>> +                       &ctf_fs->metadata_config.metadata_src);
>> +               assert(value_ret == BT_VALUE_STATUS_OK);
>> +               BT_PUT(value);
>> +       }
>> +
>>         value = bt_value_map_get(params, "clock-class-offset-s");
>>         if (value) {
>>                 if (!bt_value_is_integer(value)) {
>> diff --git a/plugins/ctf/fs-src/fs.h b/plugins/ctf/fs-src/fs.h
>> index bbac1bb4..f80aea74 100644
>> --- a/plugins/ctf/fs-src/fs.h
>> +++ b/plugins/ctf/fs-src/fs.h
>> @@ -154,7 +154,8 @@ BT_HIDDEN
>>  void ctf_fs_trace_destroy(struct ctf_fs_trace *trace);
>>
>>  BT_HIDDEN
>> -int ctf_fs_find_traces(GList **trace_paths, const char *start_path);
>> +int ctf_fs_find_traces(GList **trace_paths, const char *start_path,
>> +                      const char *metadata_src);
>>
>>  BT_HIDDEN
>>  GList *ctf_fs_create_trace_names(GList *trace_paths, const char *base_path);
>> diff --git a/plugins/ctf/fs-src/metadata.c b/plugins/ctf/fs-src/metadata.c
>> index 231d946c..02cbd0cd 100644
>> --- a/plugins/ctf/fs-src/metadata.c
>> +++ b/plugins/ctf/fs-src/metadata.c
>> @@ -96,8 +96,14 @@ int ctf_fs_metadata_set_trace(struct ctf_fs_trace *ctf_fs_trace,
>>                 .clock_class_offset_s = config ? config->clock_class_offset_s : 0,
>>                 .clock_class_offset_ns = config ? config->clock_class_offset_ns : 0,
>>         };
>> +       const char *metadata_src;
>>
>> -       file = get_file(ctf_fs_trace->path->str);
>> +       if (config->metadata_src)
>> +               metadata_src = config->metadata_src;
>> +       else
>> +               metadata_src = ctf_fs_trace->path->str;
>> +
>> +       file = get_file(metadata_src);
>>         if (!file) {
>>                 BT_LOGE("Cannot create metadata file object");
>>                 ret = -1;
>> diff --git a/plugins/ctf/fs-src/metadata.h b/plugins/ctf/fs-src/metadata.h
>> index 496a5ca9..00d8de67 100644
>> --- a/plugins/ctf/fs-src/metadata.h
>> +++ b/plugins/ctf/fs-src/metadata.h
>> @@ -36,6 +36,7 @@ struct ctf_fs_metadata;
>>  struct ctf_fs_metadata_config {
>>         int64_t clock_class_offset_s;
>>         int64_t clock_class_offset_ns;
>> +       const char *metadata_src;
>>  };
>>
>>  BT_HIDDEN
>> diff --git a/plugins/ctf/fs-src/query.c b/plugins/ctf/fs-src/query.c
>> index 04bf8c5b..6f2aa5fe 100644
>> --- a/plugins/ctf/fs-src/query.c
>> +++ b/plugins/ctf/fs-src/query.c
>> @@ -500,7 +500,7 @@ struct bt_component_class_query_method_return trace_info_query(
>>         }
>>         assert(path);
>>
>> -       ret = ctf_fs_find_traces(&trace_paths, normalized_path->str);
>> +       ret = ctf_fs_find_traces(&trace_paths, normalized_path->str, NULL);
>>         if (ret) {
>>                 goto error;
>>         }
>> --
>> 2.11.0
>>
>> _______________________________________________
>> lttng-dev mailing list
>> lttng-dev at lists.lttng.org
>> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
Alexander Aring May 1, 2018, 3:12 p.m. UTC | #3
Hi,

On Mon, Apr 30, 2018 at 06:36:08PM -0400, Philippe Proulx wrote:
> On Mon, Apr 30, 2018 at 5:31 PM, Alexander Aring <aring at mojatatu.com> wrote:
> >
> > This patch adds an argument for the fs-src plugin to declare the
> > directory to find the metadata file instead of placing it in every
> > subdir of traces.
> >
> > If parameter assign every subdirectory which does not have a
> > subdirectory and at least some regular files will be assumed as a trace
> > directory. The regular files will be assumed as ctf trace files.
> 
> Although I really appreciate the contribution effort, before even
> reading this patch, let me indicate that a CTF trace located on the file
> system is a directory containing the data stream and metadata stream
> files. Knowing this, this patch implements a hack to circumvent this,
> but I'm personally not willing to have this upstream as, in my opinion,
> it is the user's responsibility to have valid CTF traces to open.
> 

Yea, I already thought that will happen... but I want to talk about my
use-case and how to handle it with babeltrace.

My use-case is the following:

 - I use barectf with a fs platform based platform. I want to contribute
   them back to barectf if this is welcome. I see I already have the
   barectf expert here. I use it for userspace application trace.

 - I have a distributed application which stores the stream files in a
   directory with the scheme:

   $TRACES/
           $HOSTNAME/
	             ....
	             $APPNAME_$PID_stream

Currently I mostly run my application on one host with one clock source,
which makes everything about clock handling very simple.

So far I know the multiple streams and merge has the use case to have a
trace per cpu, well we do it per application... I hope this is okay.

> Your custom scenario asks for a custom solution on your side: copying
> the metadata file to each trace's directory seems appropriate here.
> 

Really?

I thought about that I cannot use traces on a read-only filesystem,
because I am not allowed to copy there. One example where my solution
hits it's limitation...

I also thought about: using barectf and generate a object file linked
to my platform who has the metadata file inside and will be created to
my stream file when barectf init's (if it's not already exists).
So far I see it could be done because metadata file is compile time
related and binded to barectf generated code.

That will bloat my binary as one disadvantage, otherwise I can be sure
there is no mixed metadata handling everywhere.

> Also, do your CTF traces have UUIDs? If they do, they should all be
> different, but having the same metadata file makes them all the same.
> This is not valid either.
> 

I have UUID but my applications use all the same metadata file by
barectf. :-) I hope this is a valid point?

I admit, currently I have mostly functionality to make backwards
compatibility with an old trace handling. It's just put some ascii info
inside the stream. Later I want to move ascii into metadata description.
It works like tracelog [0], but just temporary.

- Alex

[0] http://man7.org/linux/man-pages/man3/tracelog.3.html
Philippe Proulx May 1, 2018, 3:22 p.m. UTC | #4
On Tue, May 1, 2018 at 11:12 AM, Alexander Aring <aring at mojatatu.com> wrote:
> Hi,
>
> On Mon, Apr 30, 2018 at 06:36:08PM -0400, Philippe Proulx wrote:
>> On Mon, Apr 30, 2018 at 5:31 PM, Alexander Aring <aring at mojatatu.com> wrote:
>> >
>> > This patch adds an argument for the fs-src plugin to declare the
>> > directory to find the metadata file instead of placing it in every
>> > subdir of traces.
>> >
>> > If parameter assign every subdirectory which does not have a
>> > subdirectory and at least some regular files will be assumed as a trace
>> > directory. The regular files will be assumed as ctf trace files.
>>
>> Although I really appreciate the contribution effort, before even
>> reading this patch, let me indicate that a CTF trace located on the file
>> system is a directory containing the data stream and metadata stream
>> files. Knowing this, this patch implements a hack to circumvent this,
>> but I'm personally not willing to have this upstream as, in my opinion,
>> it is the user's responsibility to have valid CTF traces to open.
>>
>
> Yea, I already thought that will happen... but I want to talk about my
> use-case and how to handle it with babeltrace.
>
> My use-case is the following:
>
>  - I use barectf with a fs platform based platform. I want to contribute
>    them back to barectf if this is welcome. I see I already have the
>    barectf expert here. I use it for userspace application trace.
>
>  - I have a distributed application which stores the stream files in a
>    directory with the scheme:
>
>    $TRACES/
>            $HOSTNAME/
>                      ....
>                      $APPNAME_$PID_stream
>
> Currently I mostly run my application on one host with one clock source,
> which makes everything about clock handling very simple.
>
> So far I know the multiple streams and merge has the use case to have a
> trace per cpu, well we do it per application... I hope this is okay.
>
>> Your custom scenario asks for a custom solution on your side: copying
>> the metadata file to each trace's directory seems appropriate here.
>>
>
> Really?
>
> I thought about that I cannot use traces on a read-only filesystem,
> because I am not allowed to copy there. One example where my solution
> hits it's limitation...
>
> I also thought about: using barectf and generate a object file linked
> to my platform who has the metadata file inside and will be created to
> my stream file when barectf init's (if it's not already exists).
> So far I see it could be done because metadata file is compile time
> related and binded to barectf generated code.

Yes this is a better idea. If you cannot postprocess the trace directories,
than it's better that you create valid traces in the beginning.

>
> That will bloat my binary as one disadvantage, otherwise I can be sure
> there is no mixed metadata handling everywhere.

Can you create symbolic links on this filesystem (when you have write
access)? This would avoid bloating.

>
>> Also, do your CTF traces have UUIDs? If they do, they should all be
>> different, but having the same metadata file makes them all the same.
>> This is not valid either.
>>
>
> I have UUID but my applications use all the same metadata file by
> barectf. :-) I hope this is a valid point?

Normally, a trace has a unique UUID. If you cannot ensure this, I
suggest that you remove the UUID field from the packet header.

Phil

>
> I admit, currently I have mostly functionality to make backwards
> compatibility with an old trace handling. It's just put some ascii info
> inside the stream. Later I want to move ascii into metadata description.
> It works like tracelog [0], but just temporary.
>
> - Alex
>
> [0] http://man7.org/linux/man-pages/man3/tracelog.3.html
Alexander Aring May 1, 2018, 3:43 p.m. UTC | #5
Hi,

On Tue, May 01, 2018 at 11:22:18AM -0400, Philippe Proulx wrote:
...
> >
> > I also thought about: using barectf and generate a object file linked
> > to my platform who has the metadata file inside and will be created to
> > my stream file when barectf init's (if it's not already exists).
> > So far I see it could be done because metadata file is compile time
> > related and binded to barectf generated code.
> 
> Yes this is a better idea. If you cannot postprocess the trace directories,
> than it's better that you create valid traces in the beginning.
> 

I see, I will have this idea in the background... I think I will switch
to it when I see some issues with metadata and this would avoid it.

> >
> > That will bloat my binary as one disadvantage, otherwise I can be sure
> > there is no mixed metadata handling everywhere.
> 
> Can you create symbolic links on this filesystem (when you have write
> access)? This would avoid bloating.
> 

I will switch to symbolic link and hopefully nobody will pack everything
in a .zip or something else that doesn't support symbolic links. :-)

> >
> >> Also, do your CTF traces have UUIDs? If they do, they should all be
> >> different, but having the same metadata file makes them all the same.
> >> This is not valid either.
> >>
> >
> > I have UUID but my applications use all the same metadata file by
> > barectf. :-) I hope this is a valid point?
> 
> Normally, a trace has a unique UUID. If you cannot ensure this, I
> suggest that you remove the UUID field from the packet header.
> 

I see, according [0]:

Trace UUID, used to ensure the event packet match the metadata used.
Note: we cannot use a metadata checksum in every cases instead of a UUID
because metadata can be appended to while tracing is active. This field
is optional.

---

Will this drop not a validation for me that the stream and metadata fits
together? So babeltrace will reject something when stream was made with
a different metadata as placed into my TRACES directory?

Somebody could use a different metadata file, by weird accident.

And "... metadata can be appended to while tracing is active." - woa, so
you can add more metadata during runtime and the streams can use them
(during runtime)?

I guess this is not possible with barectf where everything is made at
compile time... (currently).

- Alex

[0] http://diamon.org/ctf/#spec5
Alexander Aring May 1, 2018, 5:01 p.m. UTC | #6
On Tue, May 01, 2018 at 11:43:32AM -0400, Alexander Aring wrote:
...
> > 
> > Normally, a trace has a unique UUID. If you cannot ensure this, I
> > suggest that you remove the UUID field from the packet header.
> > 
> 
> I see, according [0]:
> 
> Trace UUID, used to ensure the event packet match the metadata used.
> Note: we cannot use a metadata checksum in every cases instead of a UUID
> because metadata can be appended to while tracing is active. This field
> is optional.
> 

I removed some other fields which I don't need. Thanks for reminding me!
I removed:


 - Stream-ID: I have only one stream
 - events_discarded: I don't handle this in fs platform right now, could
   be maybe as "filesystem is full?"

May I drop uuid when I have only one metadata file, but validation if
metadata fits or not could be useful for handling the case if somebody
use a different one.

I see that barectf will still generate some uuid and place it in:

trace {
	uuid = ....
};

but I guess it will not be used anymore in the stream. So far I don't
see it anymore with a simple hexdump (Looks for me as the best way to
check what barectf is doing).

Thanks.

- Alex
diff mbox series

Patch

diff --git a/cli/babeltrace-cfg-cli-args.c b/cli/babeltrace-cfg-cli-args.c
index 8f01e64b..ab76852e 100644
--- a/cli/babeltrace-cfg-cli-args.c
+++ b/cli/babeltrace-cfg-cli-args.c
@@ -1306,6 +1306,7 @@  enum {
 	OPT_CLOCK_GMT,
 	OPT_CLOCK_OFFSET,
 	OPT_CLOCK_OFFSET_NS,
+	OPT_METADATA_SRC,
 	OPT_CLOCK_SECONDS,
 	OPT_COLOR,
 	OPT_COMPONENT,
@@ -2789,6 +2790,7 @@  void print_convert_usage(FILE *fp)
 	fprintf(fp, "\n");
 	fprintf(fp, "      --clock-offset=SEC            Set clock offset to SEC seconds\n");
 	fprintf(fp, "      --clock-offset-ns=NS          Set clock offset to NS ns\n");
+	fprintf(fp, "      --metadata-src=PATH		 Set a path to find the metadata\n");
 	fprintf(fp, "\n");
 	fprintf(fp, "Implicit `sink.text.pretty` component options:\n");
 	fprintf(fp, "\n");
@@ -2886,6 +2888,7 @@  struct poptOption convert_long_options[] = {
 	{ "clock-gmt", '\0', POPT_ARG_NONE, NULL, OPT_CLOCK_GMT, NULL, NULL },
 	{ "clock-offset", '\0', POPT_ARG_STRING, NULL, OPT_CLOCK_OFFSET, NULL, NULL },
 	{ "clock-offset-ns", '\0', POPT_ARG_STRING, NULL, OPT_CLOCK_OFFSET_NS, NULL, NULL },
+	{ "metadata-src", '\0', POPT_ARG_STRING, NULL, OPT_METADATA_SRC, NULL, NULL },
 	{ "clock-seconds", '\0', POPT_ARG_NONE, NULL, OPT_CLOCK_SECONDS, NULL, NULL },
 	{ "color", '\0', POPT_ARG_STRING, NULL, OPT_COLOR, NULL, NULL },
 	{ "component", 'c', POPT_ARG_STRING, NULL, OPT_COMPONENT, NULL, NULL },
@@ -3942,6 +3945,7 @@  struct bt_config *bt_config_convert_from_args(int argc, const char *argv[],
 		case OPT_CLOCK_GMT:
 		case OPT_CLOCK_OFFSET:
 		case OPT_CLOCK_OFFSET_NS:
+		case OPT_METADATA_SRC:
 		case OPT_CLOCK_SECONDS:
 		case OPT_COLOR:
 		case OPT_DEBUG:
@@ -4104,6 +4108,12 @@  struct bt_config *bt_config_convert_from_args(int argc, const char *argv[],
 					&base_implicit_ctf_input_args,
 					"clock-class-offset-ns", arg);
 			break;
+		case OPT_METADATA_SRC:
+			base_implicit_ctf_input_args.exists = true;
+			append_implicit_component_param(
+					&base_implicit_ctf_input_args,
+					"metadata-src", arg);
+			break;
 		case OPT_CLOCK_SECONDS:
 			append_implicit_component_param(
 				&implicit_text_args, "clock-seconds", "yes");
diff --git a/plugins/ctf/fs-src/fs.c b/plugins/ctf/fs-src/fs.c
index 2dacf97d..7f98dda5 100644
--- a/plugins/ctf/fs-src/fs.c
+++ b/plugins/ctf/fs-src/fs.c
@@ -1039,26 +1039,70 @@  end:
 }
 
 BT_HIDDEN
-int ctf_fs_find_traces(GList **trace_paths, const char *start_path)
+int ctf_fs_find_traces(GList **trace_paths, const char *start_path,
+		       const char *metadata_src)
 {
 	int ret;
 	GError *error = NULL;
 	GDir *dir = NULL;
 	const char *basename = NULL;
+	bool subdirs = false;
+	bool regfile = false;
 
-	/* Check if the starting path is a CTF trace itself */
-	ret = path_is_ctf_trace(start_path);
-	if (ret < 0) {
-		goto end;
-	}
+	if (metadata_src) {
+		ret = path_is_ctf_trace(metadata_src);
+		if (ret < 0) {
+			goto end;
+		}
 
-	if (ret) {
-		/*
-		 * Stop recursion: a CTF trace cannot contain another
-		 * CTF trace.
-		 */
-		ret = add_trace_path(trace_paths, start_path);
-		goto end;
+		if (ret) {
+			dir = g_dir_open(start_path, 0, &error);
+			if (!dir) {
+				goto end;
+			}
+
+			while ((basename = g_dir_read_name(dir))) {
+				GString *sub_path = g_string_new(NULL);
+
+				if (!sub_path) {
+					ret = -1;
+					goto end;
+				}
+
+				g_string_printf(sub_path, "%s" G_DIR_SEPARATOR_S "%s", start_path, basename);
+				if (g_file_test(sub_path->str, G_FILE_TEST_IS_DIR)) {
+					subdirs = true;
+				}
+				if (g_file_test(sub_path->str, G_FILE_TEST_IS_REGULAR)) {
+					regfile = true;
+				}
+				g_string_free(sub_path, TRUE);
+			}
+
+			g_dir_close(dir);
+			dir = NULL;
+
+			/* Look if dir has no subdirs but regfile(s) */
+			if (!subdirs && regfile) {
+				ret = add_trace_path(trace_paths, start_path);
+				goto end;
+			}
+		}
+	} else {
+		/* Check if the starting path is a CTF trace itself */
+		ret = path_is_ctf_trace(start_path);
+		if (ret < 0) {
+			goto end;
+		}
+
+		if (ret) {
+			/*
+			 * Stop recursion: a CTF trace cannot contain another
+			 * CTF trace.
+			 */
+			ret = add_trace_path(trace_paths, start_path);
+			goto end;
+		}
 	}
 
 	/* Look for subdirectories */
@@ -1090,7 +1134,7 @@  int ctf_fs_find_traces(GList **trace_paths, const char *start_path)
 		}
 
 		g_string_printf(sub_path, "%s" G_DIR_SEPARATOR_S "%s", start_path, basename);
-		ret = ctf_fs_find_traces(trace_paths, sub_path->str);
+		ret = ctf_fs_find_traces(trace_paths, sub_path->str, metadata_src);
 		g_string_free(sub_path, TRUE);
 		if (ret) {
 			goto end;
@@ -1181,7 +1225,8 @@  int create_ctf_fs_traces(struct ctf_fs_component *ctf_fs,
 		goto error;
 	}
 
-	ret = ctf_fs_find_traces(&trace_paths, norm_path->str);
+	ret = ctf_fs_find_traces(&trace_paths, norm_path->str,
+				 ctf_fs->metadata_config.metadata_src);
 	if (ret) {
 		goto error;
 	}
@@ -1287,6 +1332,19 @@  struct ctf_fs_component *ctf_fs_create(struct bt_private_component *priv_comp,
 	value_ret = bt_value_string_get(value, &path_param);
 	assert(value_ret == BT_VALUE_STATUS_OK);
 	BT_PUT(value);
+
+	value = bt_value_map_get(params, "metadata-src");
+	if (value) {
+		if (!bt_value_is_string(value)) {
+			goto error;
+		}
+
+		value_ret = bt_value_string_get(value,
+			&ctf_fs->metadata_config.metadata_src);
+		assert(value_ret == BT_VALUE_STATUS_OK);
+		BT_PUT(value);
+	}
+
 	value = bt_value_map_get(params, "clock-class-offset-s");
 	if (value) {
 		if (!bt_value_is_integer(value)) {
diff --git a/plugins/ctf/fs-src/fs.h b/plugins/ctf/fs-src/fs.h
index bbac1bb4..f80aea74 100644
--- a/plugins/ctf/fs-src/fs.h
+++ b/plugins/ctf/fs-src/fs.h
@@ -154,7 +154,8 @@  BT_HIDDEN
 void ctf_fs_trace_destroy(struct ctf_fs_trace *trace);
 
 BT_HIDDEN
-int ctf_fs_find_traces(GList **trace_paths, const char *start_path);
+int ctf_fs_find_traces(GList **trace_paths, const char *start_path,
+		       const char *metadata_src);
 
 BT_HIDDEN
 GList *ctf_fs_create_trace_names(GList *trace_paths, const char *base_path);
diff --git a/plugins/ctf/fs-src/metadata.c b/plugins/ctf/fs-src/metadata.c
index 231d946c..02cbd0cd 100644
--- a/plugins/ctf/fs-src/metadata.c
+++ b/plugins/ctf/fs-src/metadata.c
@@ -96,8 +96,14 @@  int ctf_fs_metadata_set_trace(struct ctf_fs_trace *ctf_fs_trace,
 		.clock_class_offset_s = config ? config->clock_class_offset_s : 0,
 		.clock_class_offset_ns = config ? config->clock_class_offset_ns : 0,
 	};
+	const char *metadata_src;
 
-	file = get_file(ctf_fs_trace->path->str);
+	if (config->metadata_src)
+		metadata_src = config->metadata_src;
+	else
+		metadata_src = ctf_fs_trace->path->str;
+
+	file = get_file(metadata_src);
 	if (!file) {
 		BT_LOGE("Cannot create metadata file object");
 		ret = -1;
diff --git a/plugins/ctf/fs-src/metadata.h b/plugins/ctf/fs-src/metadata.h
index 496a5ca9..00d8de67 100644
--- a/plugins/ctf/fs-src/metadata.h
+++ b/plugins/ctf/fs-src/metadata.h
@@ -36,6 +36,7 @@  struct ctf_fs_metadata;
 struct ctf_fs_metadata_config {
 	int64_t clock_class_offset_s;
 	int64_t clock_class_offset_ns;
+	const char *metadata_src;
 };
 
 BT_HIDDEN
diff --git a/plugins/ctf/fs-src/query.c b/plugins/ctf/fs-src/query.c
index 04bf8c5b..6f2aa5fe 100644
--- a/plugins/ctf/fs-src/query.c
+++ b/plugins/ctf/fs-src/query.c
@@ -500,7 +500,7 @@  struct bt_component_class_query_method_return trace_info_query(
 	}
 	assert(path);
 
-	ret = ctf_fs_find_traces(&trace_paths, normalized_path->str);
+	ret = ctf_fs_find_traces(&trace_paths, normalized_path->str, NULL);
 	if (ret) {
 		goto error;
 	}