On Thu, 6 Nov 2014 19:41:55 +0100
Petr Mladek <[email protected]> wrote:
> > /* "in progress" flag of arch_trigger_all_cpu_backtrace */
> > static unsigned long backtrace_flag;
> >
> > +static void print_seq_line(struct nmi_seq_buf *s, int last, int pos)
>
> I would rename the arguments:
>
> "last -> first"
> "pos -> last"
>
> or maybe better would be to pass first positon and len.
I switched it to "start" and "end" to not be confused by the last_i
that is being passed in.
>
> > +{
> > + const char *buf = s->buffer + last;
> > +
> > + printk("%.*s", (pos - last) + 1, buf);
> > +}
>
> >
> > + /*
> > + * Now that all the NMIs have triggered, we can dump out their
> > + * back traces safely to the console.
> > + */
> > + for_each_cpu(cpu, printtrace_mask) {
> > + int last_i = 0;
> > +
> > + s = &per_cpu(nmi_print_seq, cpu);
> > + len = s->seq.len;
>
> If there is an seq_buf overflow, the len might be size + 1, so we need to do:
>
> len = min(s->seq.len, s->size);
>
> Well, we should create a function for this in seq_buf.h.
> Alternatively, we might reconsider the overflow state,
> use len == size and extra "overflow" flag in the seq_buf struct.
>
>
> > + if (!len)
> > + continue;
> > +
> > + /* Print line by line. */
> > + for (i = 0; i < len; i++) {
> > + if (s->buffer[i] == '\n') {
> > + print_seq_line(s, last_i, i);
> > + last_i = i + 1;
> > + }
> > + }
> >
> > + if (last_i < i - 1) {
>
> IMHO, this should be:
>
> if (last_i < i)
>
> because last_i = i + 1. Otherwise, we would ignore state when there is
> one character after a new line. For example, imagine the following:
>
> buffer = "a\nb";
> len = 3;
>
> it will end with:
>
> last_i = 2;
> i = 3;
>
> and we still need to print the "b".
Well, we really don't *need* to ;-)
But for correctness sake, I agree, it should be last_i < i.
>
> > + print_seq_line(s, last_i, i);
>
> If I get it correctly, (i == len) here and "printk_seq_line"
> print_seq_line() prints the characters including "pos" value.
> So, we should call:
>
> print_seq_line(s, last_i, i - 1)
Right that was wrong. Actually, I think the best answer would be:
print_seq_line(s, last_i, len - 1);
This removes the variable 'i'. Probably should add a comment here too
that reminds the reviewer that print_seq_line() prints up to and
including the last index.
Note, my current code also has:
len = seq_buf_used(&s->seq);
where we don't need to worry about the semantics of seq_buf internals.
-- Steve
>
> > + pr_cont("\n");
> > + }
> > + }
> > +
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
