On Fri 2014-11-07 13:56:09, Steven Rostedt wrote:
> 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.
I like it.
> >
> > > +{
> > > + 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.
I agree that one more character does not make much difference but
it might save someones day :-)
> >
> > > + 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);
Yup
> 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.
Yes, the comment is worth having.
> 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.
Perfect
Thanks a lot for working on it. Please, resend this patch once you are
happy with it.
Best Regards,
Petr
--
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/
