Re: [PATCH 3/3] ftrace: Add debug_dump trace to dump binary data from kernel to userspace

[Frédéric Weisbecker]

2008/11/14 Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxxxxxxx>:
> +struct trace_array *trace_get_global_trace(void)
> +{
> +       return &global_trace;
> +}


I'm not sure this is necessary to export the global_trace.
It is transmitted through init callback of a tracer (struct trace_array *tr).
You have a ctx_trace that you make pointing to global_trace using tr.
And after that you never use it.
That would be better to use your ctx_trace instead of exporting such function.

> +int debug_dump(void *bin_data, int len)
> +{
> +       struct ring_buffer_event *event;
> +       struct trace_array *tr;
> +       struct trace_array_cpu *data;
> +       struct dump_entry *entry;
> +       unsigned long flags, irq_flags;
> +       int cpu, size, pc;
> +
> +       if (!tracer_enabled)
> +               return 0;
> +       tr = trace_get_global_trace();
> +       pc = preempt_count();
> +       preempt_disable_notrace();
> +       cpu = raw_smp_processor_id();
> +       data = tr->data[cpu];
> +
> +       local_irq_save(flags);
> +       size = sizeof(*entry) + len;
> +       event = ring_buffer_lock_reserve(tr->buffer, size, &irq_flags);
> +       if (!event)
> +               goto out;
> +       entry = ring_buffer_event_data(event);
> +       tracing_generic_entry_update(&entry->ent, flags, pc);
> +       entry->ent.type = TRACE_BIN_DUMP;
> +       entry->len      = len;
> +
> +       memcpy(&entry->buf, bin_data, len);
> +       ring_buffer_unlock_commit(tr->buffer, event, irq_flags);
> +
> +out:
> +       local_irq_restore(flags);
> +       preempt_enable_notrace();
> +
> +       return len;
> +}
> +EXPORT_SYMBOL_GPL(debug_dump);


I don't think this is necessary here to protect against preemption or
interrupts.
IE: it is necessary for function tracing to avoid recursion. But for
other usual cases
I think this it is not needed. If you thought about protecting the
ring buffer, it is already
able to protect itself :-)


> +static void dd_trace_ctrl_update(struct trace_array *tr)
> +{
> +       /* When starting a new trace, reset the buffers */
> +       if (tr->ctrl)
> +               start_dd_trace(tr);
> +       else
> +               stop_dd_trace(tr);
> +}


As I said, this callback recently disappeared.


> +
> +#ifdef CONFIG_NOP_TRACER
> +int
> +trace_selftest_startup_dd(struct tracer *trace, struct trace_array *tr)
> +{
> +       /* What could possibly go wrong? */
> +       return 0;
> +}
> +#endif


NOP_TRACER...copy-pasting is not always our friend... :-)


> +static enum print_line_t debug_dump_entry(struct trace_iterator *iter)
> +{
> +       struct trace_seq *s = &iter->seq;
> +       struct trace_entry *entry;
> +
> +       entry = iter->ent;
> +       switch (entry->type) {
> +       case TRACE_BIN_DUMP: {
> +               struct dump_entry *field;
> +               trace_assign_type(field, entry);
> +               trace_seq_putmem(s, field->buf, field->len);
> +               break;


Don't forget the TRACE_TYPE_PRINT_LINE...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/