* Amer Alhalabi ([email protected]) 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 <[email protected]> > > > 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 > [email protected] > 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 _______________________________________________ ltt-dev mailing list [email protected] http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
