[ltt-dev] Babeltrace PATCH
Mathieu Desnoyers
compudj at krystal.dyndns.org
Tue Aug 2 14:06:20 EDT 2011
* Amer Alhalabi (lord1298 at gmail.com) wrote:
> Babeltrace PATCH: Multiple Directory Conversion
>
> Code added/modified to merge and convert many CTF binary files
> located in different directories into one ASCII file
Hi Amer,
The code is getting in very good shape. A few more comments below,
>
> Signed-off-by: Amer Alhalabi <amer.alhalabi at ericsson.com>
>
>
> Index: babeltrace/converter/babeltrace-lib.c
> ===================================================================
> --- babeltrace.orig/converter/babeltrace-lib.c 2011-07-25 10:42:26.000000000 -0400
> +++ babeltrace/converter/babeltrace-lib.c 2011-07-28 17:39:20.248116000 -0400
> @@ -56,38 +56,54 @@
> return 0;
> }
>
> -int convert_trace(struct trace_descriptor *td_write,
> - struct trace_descriptor *td_read)
> +int convert_trace(struct trace_descriptor *td_write)
> {
> - struct ctf_trace *tin = container_of(td_read, struct ctf_trace, parent);
> - struct ctf_text_stream_pos *sout =
> - container_of(td_write, struct ctf_text_stream_pos, trace_descriptor);
> - int stream_id;
> + struct ptr_heap *stream_heap;
> + struct ctf_text_stream_pos *sout;
> + int i, stream_id;
> int ret = 0;
>
> - tin->stream_heap = g_new(struct ptr_heap, 1);
> - heap_init(tin->stream_heap, 0, stream_compare);
> + stream_heap = g_new(struct ptr_heap, 1);
> + heap_init(stream_heap, 0, stream_compare);
> + sout = container_of(td_write, struct ctf_text_stream_pos,
> + trace_descriptor);
> +
> + for (i = 0; i < td_read_arr->len; i++) {
> + struct ctf_trace *tin;
> + struct trace_descriptor *td_read;
> +
> + td_read = g_ptr_array_index(td_read_arr, i);
> + tin = container_of(td_read, struct ctf_trace, parent);
> +
> + /* Populate heap with each stream */
> + for (stream_id = 0; stream_id < tin->streams->len;
> + stream_id++) {
> + struct ctf_stream_class *stream;
> + int filenr;
>
> - /* Populate heap with each stream */
> - for (stream_id = 0; stream_id < tin->streams->len; stream_id++) {
> - struct ctf_stream_class *stream = g_ptr_array_index(tin->streams, stream_id);
> - int filenr;
> -
> - if (!stream)
> - continue;
> - for (filenr = 0; filenr < stream->streams->len; filenr++) {
> - struct ctf_file_stream *file_stream = g_ptr_array_index(stream->streams, filenr);
> - ret = read_event(file_stream);
> - if (ret == EOF) {
> - ret = 0;
> + stream = g_ptr_array_index(tin->streams, stream_id);
> + if (!stream)
> continue;
I prefer to do :
if () {
} else {
}
(brackets for both branches)
Even if the first branch is a single-liner. This makes the code less
error-prone when we modify it later on.
> - } else if (ret)
> - goto end;
> - /* Add to heap */
> - ret = heap_insert(tin->stream_heap, file_stream);
> - if (ret) {
> - fprintf(stdout, "[error] Out of memory.\n");
> - goto end;
> + for (filenr = 0; filenr < stream->streams->len;
> + filenr++) {
> + struct ctf_file_stream *file_stream;
> +
> + file_stream = g_ptr_array_index(stream->streams,
> + filenr);
> +
> + ret = read_event(file_stream);
> + if (ret == EOF) {
> + ret = 0;
> + continue;
> + } else if (ret)
> + goto end;
> + /* Add to heap */
> + ret = heap_insert(stream_heap, file_stream);
> + if (ret) {
> + fprintf(stdout,
> + "[error] Out of memory.\n");
> + goto end;
> + }
> }
> }
> }
> @@ -96,7 +112,7 @@
> for (;;) {
> struct ctf_file_stream *file_stream, *removed;
>
> - file_stream = heap_maximum(tin->stream_heap);
> + file_stream = heap_maximum(stream_heap);
> if (!file_stream) {
> /* end of file for all streams */
> ret = 0;
> @@ -109,19 +125,19 @@
> }
> ret = read_event(file_stream);
> if (ret == EOF) {
> - removed = heap_remove(tin->stream_heap);
> + removed = heap_remove(stream_heap);
> assert(removed == file_stream);
> ret = 0;
> continue;
> } else if (ret)
> goto end;
> /* Reinsert the file stream into the heap, and rebalance. */
> - removed = heap_replace_max(tin->stream_heap, file_stream);
> + removed = heap_replace_max(stream_heap, file_stream);
> assert(removed == file_stream);
> }
>
> end:
> - heap_free(tin->stream_heap);
> - g_free(tin->stream_heap);
> + heap_free(stream_heap);
> + g_free(stream_heap);
> return ret;
> }
> Index: babeltrace/converter/babeltrace.c
> ===================================================================
> --- babeltrace.orig/converter/babeltrace.c 2011-07-25 10:42:26.000000000 -0400
> +++ babeltrace/converter/babeltrace.c 2011-07-28 18:02:20.251183000 -0400
> @@ -27,7 +27,10 @@
> #include <sys/stat.h>
> #include <sys/types.h>
> #include <fcntl.h>
> +#include <ftw.h>
> +#include <dirent.h>
>
> +#define DEFAULT_FILE_ARRAY_SIZE 1
> static char *opt_input_format;
> static char *opt_output_format;
>
> @@ -37,6 +40,10 @@
> int babeltrace_verbose, babeltrace_debug;
> int opt_field_names;
>
> +/* array of trace_descriptor pointers */
> +GPtrArray *td_read_arr;
> +static struct format *fmt_read;
> +
> void strlower(char *str)
> {
> while (*str) {
> @@ -149,11 +156,71 @@
> return ret;
> }
>
> +/*
> + * destroy_array() closes the opened traces for read
> + * and free the memory allocated for td_read_arr
> + */
> +static void destroy_array(void)
> +{
> + int i;
Please add a newline between variable declaration and code.
> + for (i = 0; i < td_read_arr->len; i++) {
> + struct trace_descriptor *temp =
> + g_ptr_array_index(td_read_arr, i);
> + if (temp)
Why are you checking if temp is NULL or not ? I don't think it's
required. Please check the g_ptr_array_sized_new() documentation for
difference between "size" (allocated) and "length".
> + fmt_read->close_trace(temp);
> + }
> + if (td_read_arr)
This if seems unnecessary. td_read_arr is always allocated, no ?
If you create a "destroy_array" abstraction, then you should also have
the symmetric "create_array" abstraction.
> + g_ptr_array_free(td_read_arr, TRUE);
> +}
> +
> +/*
> + * traverse_dir() is the callback functiion for File Tree Walk (nftw).
> + * it receives the path of the current entry (file, dir, link..etc) with
> + * a flag to indicate the type of the entry.
> + * if the entry being visited is a directory and contains a metadata file,
> + * then open it for reading and save a trace_descriptor to that directory
> + * in td_read_arr[]
> + */
> +static int traverse_dir(const char *fpath, const struct stat *sb,
> + int tflag, struct FTW *ftwbuf)
> +{
> + int dirfd;
> + int fd;
> + struct trace_descriptor *td_read;
> + int ret = 0;
> +
> + if (tflag != FTW_D)
> + return 0;
> + dirfd = open(fpath, 0);
> + if (dirfd < 0) {
> + fprintf(stdout, "[error] unable to open trace "
> + "directory file descriptor.\n");
> + return -1;
> + }
> + fd = openat(dirfd, "metadata", O_RDONLY);
> + if (fd < 0) {
> + close(dirfd);
> + } else {
> + close(fd);
> + close(dirfd);
> + td_read = fmt_read->open_trace(fpath, O_RDONLY);
> + if (!td_read) {
> + fprintf(stdout, "Error opening trace \"%s\" "
> + "for reading.\n\n", fpath);
> + return -1; /* error */
> + }
> + g_ptr_array_add(td_read_arr, (gpointer) td_read);
> + }
> + return 0; /* success */
> +}
> +
> int main(int argc, char **argv)
> {
> int ret;
> - struct format *fmt_read, *fmt_write;
> - struct trace_descriptor *td_read, *td_write;
> + struct format *fmt_write;
> + struct trace_descriptor *td_write;
> +
> + td_read_arr = g_ptr_array_sized_new(DEFAULT_FILE_ARRAY_SIZE);
>
> ret = parse_options(argc, argv);
> if (ret < 0) {
> @@ -196,12 +263,26 @@
> exit(EXIT_FAILURE);
> }
>
> - td_read = fmt_read->open_trace(opt_input_path, O_RDONLY);
> - if (!td_read) {
> + /*
> + * first allocate memory for td_read_arr .
> + * pass the input path to nftw() .
> + * specify traverse_dir() as the callback function.
> + * depth = 10 which is the max number of file descriptors
> + * that nftw() can open at a given time.
> + * flags = 0 check nftw documentation for more info .
> + */
> + td_read_arr = g_ptr_array_new();
Hrm, this is the second time you allocate td_read_arr ?
> + ret = nftw(opt_input_path, traverse_dir, 10, 0);
> + if (ret != 0) {
> fprintf(stdout, "[error] opening trace \"%s\" for reading.\n\n",
> opt_input_path);
> goto error_td_read;
> }
> + if (td_read_arr->len == 0) {
> + fprintf(stdout, "[warning] no metadata file was found."
> + " no output was generated\n");
> + return 0;
> + }
>
> td_write = fmt_write->open_trace(opt_output_path, O_RDWR);
> if (!td_write) {
> @@ -210,21 +291,23 @@
> goto error_td_write;
> }
>
> - ret = convert_trace(td_write, td_read);
> + ret = convert_trace(td_write);
> if (ret) {
> fprintf(stdout, "Error printing trace.\n\n");
> goto error_copy_trace;
> }
>
> fmt_write->close_trace(td_write);
> - fmt_read->close_trace(td_read);
> + destroy_array();
> + fprintf(stdout, "finished converting. output written to:\n%s\n",
> + opt_output_path);
Hrm, I'm not sure we want to print those messages in non-verbose mode.
The idea is that the quick way to use babeltrace is to output the
human-readable output to stdout, which can then be piped into another
tool to perform treatment on the text data. So adding non-event
information to stdout in every cases would pollute stdout. So would
printing this with printf_verbose() be fine with you ?
Thanks,
Mathieu
> exit(EXIT_SUCCESS);
>
> /* Error handling */
> error_copy_trace:
> fmt_write->close_trace(td_write);
> error_td_write:
> - fmt_read->close_trace(td_read);
> + destroy_array();
> error_td_read:
> exit(EXIT_FAILURE);
> }
> Index: babeltrace/include/babeltrace/babeltrace.h
> ===================================================================
> --- babeltrace.orig/include/babeltrace/babeltrace.h 2011-07-25 10:46:11.000000000 -0400
> +++ babeltrace/include/babeltrace/babeltrace.h 2011-07-28 17:35:00.320708000 -0400
> @@ -2,11 +2,13 @@
> #define _BABELTRACE_H
>
> #include <stdio.h>
> +#include <glib.h>
>
> #define BABELTRACE_VERSION_MAJOR 0
> #define BABELTRACE_VERSION_MINOR 1
>
> extern int babeltrace_verbose, babeltrace_debug;
> +extern GPtrArray *td_read_arr;
Why are you exporting td_read_arr ? It should instead be passed as
parameter to convert_trace().
Thanks,
Mathieu
>
> #define printf_verbose(fmt, args...) \
> do { \
> @@ -22,8 +24,7 @@
>
> struct trace_descriptor;
>
> -int convert_trace(struct trace_descriptor *td_write,
> - struct trace_descriptor *td_read);
> +int convert_trace(struct trace_descriptor *td_write);
>
> extern int opt_field_names;
>
>
> _______________________________________________
> ltt-dev mailing list
> ltt-dev at lists.casi.polymtl.ca
> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
More information about the lttng-dev
mailing list