[lttng-dev] [PATCH babeltrace] Introduce shows_interest callback for format plugins
Simon Marchi
simon.marchi at polymtl.ca
Thu Oct 31 10:39:41 EDT 2013
On 31 October 2013 09:06, Mathieu Desnoyers
<mathieu.desnoyers at efficios.com> wrote:
> ----- Original Message -----
>> From: "simon marchi" <simon.marchi at polymtl.ca>
>> To: lttng-dev at lists.lttng.org
>> Sent: Thursday, October 24, 2013 11:19:27 AM
>> Subject: [lttng-dev] [PATCH babeltrace] Introduce shows_interest callback for format plugins
>>
>> From: Simon Marchi <simon.marchi at polymtl.ca>
>>
>> The format plugins should implement this callback to tell whether they
>> are interested (capable) in processing the given path or not. This
>> allows to move CTF-specific code from the core to ctf.c. This should
>> be a step towards having more than one input plugin.
>>
>> The callback should return 1 if it wants to handle this path, 0 otherwise. It
>> also return 0 in case of catastrophic failure.
>
> This feature makes sense. Comments below,
>
>>
>> Signed-off-by: Simon Marchi <simon.marchi at polymtl.ca>
>> ---
>> converter/babeltrace.c | 73
>> ++++++++++++++++++++-------------------------
>> formats/ctf/ctf.c | 50 +++++++++++++++++++++++++++++++
>> include/babeltrace/format.h | 1 +
>> 3 files changed, 84 insertions(+), 40 deletions(-)
>>
>>
>> Hellooooo,
>>
>> There is a little problem with babeltrace currently, which is that the
>> core of the converter contains CTF-specific code, in particular in the
>> recursive trace adding code. What the traverse_trace_dir function does
>> is simply tell whether a certain path looks like a CTF trace. The
>> solution I see is to move that code in the plugins by having a callback
>> through which the plugin can tell us if a given path is interesting for
>> him. I am notoriously bad at naming stuff, so I'd be happy if you found
>> a better clear name for the callback.
>
> Not sure why an email is embedded in the patch ? Can you merge the relevant pieces of info with the patch changelog ?
Documentation/SubmittingPatches in the kernel says:
One good use for the additional comments after the "---" marker is for
a diffstat, to show what files have changed, and the number of
inserted and deleted lines per file. A diffstat is especially useful
on bigger patches. Other comments relevant only to the moment or the
maintainer, not suitable for the permanent changelog, should also go
here.
So I thought I'd try to put the message there, but I agree it looks
weird. The relevant info should already be in the commit log.
>>
>> Thanks,
>>
>> Simon
>>
>> diff --git a/converter/babeltrace.c b/converter/babeltrace.c
>> index d5a7040..7489f35 100644
>> --- a/converter/babeltrace.c
>> +++ b/converter/babeltrace.c
>> @@ -416,6 +416,11 @@ end:
>> return ret;
>> }
>>
>> +/*
>> + * Oh noes! Since nftw doesn't pass a user data pointer to its callbacks, we
>> + * have to resort to global variables.
>
> Please remove "Oh noes!" (not proper English, and does not add useful information).
>
> Could you change this to e.g.
>
> /*
> * Since nftw doesn't pass a user data pointer to its callbacks, we
> * have to resort to global variables.
> */
Of course.
>
>> + */
>> +static struct bt_format *traverse_trace_dir_format = NULL;
>> static GPtrArray *traversed_paths = 0;
>
> Since we now have more than one global variable, we should at least put them in a structure (can be declared in the C file), and have only a single global object containing the bt_format and GPtrArray fields.
Yep.
>>
>> /*
>> @@ -429,53 +434,32 @@ static GPtrArray *traversed_paths = 0;
>> static int traverse_trace_dir(const char *fpath, const struct stat *sb,
>> int tflag, struct FTW *ftwbuf)
>> {
>> - int dirfd, metafd;
>> - int closeret;
>> + int ret;
>>
>> - if (tflag != FTW_D)
>> - return 0;
>> + assert(traverse_trace_dir_format != NULL);
>> + assert(traversed_paths != NULL);
>>
>> - dirfd = open(fpath, 0);
>> - if (dirfd < 0) {
>> - fprintf(stderr, "[error] [Context] Unable to open trace "
>> - "directory file descriptor.\n");
>> - return 0; /* partial error */
>> + if (!traverse_trace_dir_format->shows_interest) {
>
> We could rename "shows_interest" to "read_fit" ? Then the plugin can return 0 if "no fit", and a positive integer e.g. between 1 and 10, where 10 is "best fit", and 1 is "bad fit, but can read anyway". This will be usable in the future if a user does not specify any input plugin, and we would have to choose, for each trace, which input plugin is the best.
Not sure 10 levels are necessary, but I like the idea. We could
provide 3-4 enum values ranging from NO_FIT to BEST_FIT. It would give
a meaning to each level, whereas with numbers, you don't really know
what "4" means.
>> + /* Mr plug-in, if you don't implement the shows_interest callback, how
>> + * should we know if you are interested in a path? */
>
> The comment format is:
>
> /*
> * blah blah
> */
>
> Please remove "Mr plug-in", and change to:
>
> If the plugin does not implement...........
>
> ("Mr" here does not add useful information)
Indeed.
> Thanks,
>
> Mathieu
>
>> + return -1;
>> }
>> - metafd = openat(dirfd, "metadata", O_RDONLY);
>> - if (metafd < 0) {
>> - closeret = close(dirfd);
>> - if (closeret < 0) {
>> - perror("close");
>> - return -1;
>> - }
>> - /* No meta data, just return */
>> - return 0;
>> - } else {
>> - int err_close = 0;
>>
>> - closeret = close(metafd);
>> - if (closeret < 0) {
>> - perror("close");
>> - err_close = 1;
>> - }
>> - closeret = close(dirfd);
>> - if (closeret < 0) {
>> - perror("close");
>> - err_close = 1;
>> - }
>> - if (err_close) {
>> - return -1;
>> - }
>> + ret = traverse_trace_dir_format->shows_interest(fpath, tflag == FTW_D);
>>
>> - /* Add path to the global list */
>> - if (traversed_paths == NULL) {
>> - fprintf(stderr, "[error] [Context] Invalid open path array.\n");
>> - return -1;
>> - }
>> + /*
>> + * If ret is negative (means error), we return directly. This will
>> + * interrupt the file tree walk. If ret is 0, the format is not interested
>> + * by the path, we also return directly.
>> + */
>> +
>> + if (ret > 0) {
>> + /* Format is interested. Add path to the global list */
>> g_ptr_array_add(traversed_paths, g_string_new(fpath));
>> + ret = 0;
>> }
>>
>> - return 0;
>> + return ret;
>> }
>>
>> /*
>> @@ -499,7 +483,14 @@ int bt_context_add_traces_recursive(struct bt_context
>> *ctx, const char *path,
>>
>> /* Should lock traversed_paths mutex here if used in multithread */
>>
>> + struct bt_format *format =
>> bt_lookup_format(g_quark_from_string(format_str));
>> + if (!format) {
>> + ret = -1;
>> + goto error;
>> + }
>> +
>> traversed_paths = g_ptr_array_new();
>> + traverse_trace_dir_format = format;
>> ret = nftw(path, traverse_trace_dir, 10, 0);
>>
>> /* Process the array if ntfw did not return a fatal error */
>> @@ -539,6 +530,8 @@ int bt_context_add_traces_recursive(struct bt_context
>> *ctx, const char *path,
>> fprintf(stderr, "[error] Cannot open any trace for reading.\n\n");
>> ret = -ENOENT; /* failure */
>> }
>> +
>> +error:
>> return ret;
>> }
>>
>> @@ -697,7 +690,7 @@ int main(int argc, char **argv)
>> }
>> }
>> fmt_read = bt_lookup_format(g_quark_from_static_string(opt_input_format));
>> - if (!fmt_read || fmt_read->name != g_quark_from_static_string("ctf")) {
>> + if (!fmt_read) {
>> fprintf(stderr, "[error] Format \"%s\" is not supported.\n\n",
>> opt_input_format);
>> partial_error = 1;
>> diff --git a/formats/ctf/ctf.c b/formats/ctf/ctf.c
>> index 947b439..e6babc1 100644
>> --- a/formats/ctf/ctf.c
>> +++ b/formats/ctf/ctf.c
>> @@ -111,6 +111,9 @@ static
>> int ctf_convert_index_timestamp(struct bt_trace_descriptor *tdp);
>>
>> static
>> +int ctf_shows_interest(const char *fpath, int is_dir);
>> +
>> +static
>> rw_dispatch read_dispatch_table[] = {
>> [ CTF_TYPE_INTEGER ] = ctf_integer_read,
>> [ CTF_TYPE_FLOAT ] = ctf_float_read,
>> @@ -144,6 +147,7 @@ struct bt_format ctf_format = {
>> .timestamp_begin = ctf_timestamp_begin,
>> .timestamp_end = ctf_timestamp_end,
>> .convert_index_timestamp = ctf_convert_index_timestamp,
>> + .shows_interest = ctf_shows_interest,
>> };
>>
>> static
>> @@ -2095,6 +2099,52 @@ int ctf_convert_index_timestamp(struct
>> bt_trace_descriptor *tdp)
>> }
>>
>> static
>> +int ctf_shows_interest(const char *fpath, int is_dir)
>> +{
>> + int dirfd, metafd;
>> + int closeret;
>> +
>> + if (!is_dir)
>> + return 0;
>> +
>> + dirfd = open(fpath, 0);
>> + if (dirfd < 0) {
>> + fprintf(stderr, "[error] [Context] Unable to open trace "
>> + "directory file descriptor.\n");
>> + return 0; /* partial error */
>> + }
>> + metafd = openat(dirfd, "metadata", O_RDONLY);
>> + if (metafd < 0) {
>> + closeret = close(dirfd);
>> + if (closeret < 0) {
>> + perror("close");
>> + return -1;
>> + }
>> + /* No meta data, just return */
>> + return 0;
>> + } else {
>> + int err_close = 0;
>> +
>> + closeret = close(metafd);
>> + if (closeret < 0) {
>> + perror("close");
>> + err_close = 1;
>> + }
>> + closeret = close(dirfd);
>> + if (closeret < 0) {
>> + perror("close");
>> + err_close = 1;
>> + }
>> + if (err_close) {
>> + return -1;
>> + }
>> + }
>> +
>> + /* This path is interesting. */
>> + return 1;
>> +}
>> +
>> +static
>> int ctf_close_file_stream(struct ctf_file_stream *file_stream)
>> {
>> int ret;
>> diff --git a/include/babeltrace/format.h b/include/babeltrace/format.h
>> index 07e854f..b039abc 100644
>> --- a/include/babeltrace/format.h
>> +++ b/include/babeltrace/format.h
>> @@ -77,6 +77,7 @@ struct bt_format {
>> uint64_t (*timestamp_end)(struct bt_trace_descriptor *descriptor,
>> struct bt_trace_handle *handle, enum bt_clock_type type);
>> int (*convert_index_timestamp)(struct bt_trace_descriptor *descriptor);
>> + int (*shows_interest)(const char *path, int is_dir);
>> };
>>
>> extern struct bt_format *bt_lookup_format(bt_intern_str qname);
>> --
>> 1.8.3.2
>>
>>
>> _______________________________________________
>> lttng-dev mailing list
>> lttng-dev at lists.lttng.org
>> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>>
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
More information about the lttng-dev
mailing list