* Julien Desfossez ([email protected]) wrote: > Hi Mathieu, > > This is a first attempt at exporting some of babeltrace internals. > For this first iteration, I just split the convert_trace function in various > functions that I think will be useful for other readers. > But now I'm stuck with the babeltrace.h file, there is circular dependency > between the .h files and I'd like your insight to solve it : if you look at > the > new functions prototypes, you will see that we need to include more .h files > in > babeltrace.h (types, metadata, format) but these require printf_debug and > printf_verbose.
First off, printf_verbose and printf_debug are macros, not static inline functions, so there is no dependency problem (the declaration order does not matter). But given they are not needed outside of the babeltrace realm, I would be tempted to move them to e.g. babeltrace-printf.h, which would not be installed system-side. More comments below, > > I'm not sure how you plan to export this lib, so before getting into deep > modifications, I'd like your opinion on the function I exported. > > Thanks, > > Julien > > Signed-off-by: Julien Desfossez <[email protected]> > --- > converter/babeltrace-lib.c | 90 > +++++++++++++++++++++++++++++++++----------- > 1 files changed, 68 insertions(+), 22 deletions(-) > > diff --git a/converter/babeltrace-lib.c b/converter/babeltrace-lib.c > index 6cc2b7b..d557c30 100644 > --- a/converter/babeltrace-lib.c > +++ b/converter/babeltrace-lib.c > @@ -29,7 +29,7 @@ > #include <babeltrace/ctf-text/types.h> > #include <babeltrace/prio_heap.h> > > -static int read_event(struct ctf_file_stream *sin) > +int read_event(struct ctf_file_stream *sin) > { > int ret; > > @@ -43,6 +43,17 @@ static int read_event(struct ctf_file_stream *sin) > return 0; > } > > +int write_event(struct ctf_text_stream_pos *sout, > + struct ctf_file_stream *file_stream) > +{ > + int ret; > + ret = sout->parent.event_cb(&sout->parent, &file_stream->parent); > + if (ret) { > + fprintf(stdout, "[error] Writing event failed.\n"); > + } > + return ret; > +} > + > /* > * returns true if a < b, false otherwise. > */ > @@ -56,27 +67,25 @@ int stream_compare(void *a, void *b) > return 0; > } > > -int convert_trace(struct trace_descriptor *td_write, > - struct trace_descriptor *td_read) > +int init_heap(struct ctf_trace *tin, int gt(void *a, void *b)) > { > - 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; > int ret = 0; > + struct ctf_stream_class *stream; > + struct ctf_file_stream *file_stream; > + int filenr; > > tin->stream_heap = g_new(struct ptr_heap, 1); > - heap_init(tin->stream_heap, 0, stream_compare); > + heap_init(tin->stream_heap, 0, gt); > > /* 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; > + stream = g_ptr_array_index(tin->streams, stream_id); > > if (!stream) > continue; > for (filenr = 0; filenr < stream->streams->len; filenr++) { > - struct ctf_file_stream *file_stream = > g_ptr_array_index(stream->streams, filenr); > + file_stream = g_ptr_array_index(stream->streams, > filenr); > ret = read_event(file_stream); > if (ret == EOF) { > ret = 0; > @@ -92,36 +101,73 @@ int convert_trace(struct trace_descriptor *td_write, > } > } > > +end: > + return ret; > +} > + > +struct ctf_file_stream *get_next(struct ptr_heap *stream_heap) > +{ > + return heap_maximum(stream_heap); > +} > + Hrm, the "get_next" should take care of calling all of remove_file_stream/rebalance_heap for you. I think you don't need to expose the heap per se to the lib user. This could be wrapped into the concept of a trace iterator. So we can have: /* * struct babeltrace_iter: data structure representing an iterator on an * input trace (and eventually on a collection of traces too). */ /* * Initialization/teardown. */ struct babeltrace_iter *babeltrace_iter_create(struct trace_descriptor *td); void babeltrace_iter_destroy(struct babeltrace_iter *iter); /* * Move within the trace. */ /* * babeltrace_iter_next: Move stream position to the next event. * * Does *not* read the event. * Return EOF if reached end of trace. (EOF value is usually -1) * Any other negative return value: error. * 0: success, event is ready. */ int babeltrace_iter_next(struct babeltrace_iter *iter); /* Get the current position for each stream of the trace */ struct babeltrace_iter_pos * babeltrace_iter_get_pos(struct babeltrace_iter *iter); /* The position needs to be freed after use */ void babeltrace_iter_free_pos(struct babeltrace_iter_pos *pos); /* Seek the trace to the position */ int babeltrace_iter_seek_pos(struct babeltrace_iter *iter, struct babeltrace_iter_pos *pos); /* * babeltrace_iter_seek_time: Seek the trace to the given timestamp. * * Return EOF if timestamp is after the last event of the trace. * Return other negative value for other errors. * Return 0 for success. */ int babeltrace_iter_seek_time(struct babeltrace_iter *iter, uint64_t timestamp); /* * babeltrace_iter_read_event: Read the current event data. * * @iter: trace iterator (input) * @stream: stream containing event at current position (output) * @event: current event (output) * Return 0 on success, negative error value on error. */ int babeltrace_iter_read_event(struct babeltrace_iter *iter, struct ctf_stream **stream, struct ctf_stream_event **event); And yes, to use the information returned by babeltrace_iter_read_event(), you'll need: babeltrace/ctf-ir/metadata.h which needs: babeltrace/types.h babeltrace/format.h babeltrace/ctf/types.h But the rest (struct babeltrace_iter and struct babeltrace_iter_pos) should keep their layout internal to babeltrace. Thoughts ? Thanks, Mathieu > +void remove_file_stream(struct ctf_trace *tin, struct ctf_file_stream > *file_stream) > +{ > + struct ctf_file_stream *removed; > + removed = heap_remove(tin->stream_heap); > + assert(removed == file_stream); > +} > + > +void rebalance_heap(struct ctf_trace *tin, struct ctf_file_stream > *file_stream) > +{ > + struct ctf_file_stream *removed; > + > + /* Reinsert the file stream into the heap, and rebalance. */ > + removed = heap_replace_max(tin->stream_heap, file_stream); > + assert(removed == file_stream); > +} > + > +void cleanup_heap(struct ctf_trace *tin) > +{ > + heap_free(tin->stream_heap); > + g_free(tin->stream_heap); > +} > + > +int convert_trace(struct trace_descriptor *td_write, > + struct trace_descriptor *td_read) > +{ > + struct ctf_file_stream *file_stream; > + 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 ret = 0; > + > + ret = init_heap(tin, stream_compare); > + if (ret) > + goto end; > + > /* Replace heap entries until EOF for each stream (heap empty) */ > for (;;) { > - struct ctf_file_stream *file_stream, *removed; > - > - file_stream = heap_maximum(tin->stream_heap); > + file_stream = get_next(tin->stream_heap); > if (!file_stream) { > /* end of file for all streams */ > ret = 0; > break; > } > - ret = sout->parent.event_cb(&sout->parent, > &file_stream->parent); > + ret = write_event(sout, file_stream); > if (ret) { > - fprintf(stdout, "[error] Writing event failed.\n"); > goto end; > } > ret = read_event(file_stream); > if (ret == EOF) { > - removed = heap_remove(tin->stream_heap); > - assert(removed == file_stream); > + remove_file_stream(tin, 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); > - assert(removed == file_stream); > + rebalance_heap(tin, file_stream); > } > > end: > - heap_free(tin->stream_heap); > - g_free(tin->stream_heap); > + cleanup_heap(tin); > return ret; > } > -- > 1.7.4.1 > -- 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
