Re: [Patch 3/6] statistics infrastructure - prerequisite: timestamp

[Martin Peschke]

Tim Bird wrote:
> Martin Peschke wrote:
> > And I think that I have fixed a printed_len related miscalculation.
> > printed_len needs to be increased if no valid log level has been found
> > and a log level prefix has been added by printk(). Otherwise, printed_len
> > must not be increased. The old code did it the other way around (in the
> > timestamp case).
>
> I'm not following your change here.  I can't find the problem you
> mention in the original code.  And your fix appears to mess up the
> printed_len.

I am sorry for not responding sooner ... and I am sorry for introducing
a bug here. I think you are right.

> > +		if (new_line) {
> > +			/* The log level token is first. */
> > +			int loglev_char;
> > +			if (p[0] == '<' && p[1] >='0' &&
> > +			    p[1] <= '7' && p[2] == '>') {
> > +				loglev_char = p[1];
> > +				p += 3;
> > +				printed_len -= 3;
> Here you subtract from the printed_len to account for skipping
> the submitted log level.
>
> > +			} else	{
> > +				loglev_char = default_message_loglevel + '0';
> > +			}
> > +			emit_log_char('<');
> > +			emit_log_char(loglev_char);
> > +			emit_log_char('>');
>
> But here you don't re-count the chars for the log-level
> you are adding back in.  There's a discrepancy here.

Correct. My print_len loses 3 in the 'got log level'-case due to a surplus
substraction. It also loses 3 in the other case due to adding a log level
substring that is not entered in the books.

This is how I would fix it:

        printed_len = vscnprintf(printk_buf, sizeof(printk_buf), fmt, args);

        ...

        if (new_line) {
                /* The log level token is first. */
                int loglev_char;
                if (p[0] == '<' && p[1] >='0' &&
                    p[1] <= '7' && p[2] == '>') {
                        loglev_char = p[1];
                        p += 3;
                } else  {
                        loglev_char = default_message_loglevel + '0';
                        print_len += 3;
                }
                emit_log_char('<');
                emit_log_char(loglev_char);
                emit_log_char('>');


Could you confirm, please. I will send a patch to Andrew then.


Or, Andrew, do you prefer a replacement patch for my
statistics-infrastructure-prerequisite-timestamp.patch
that introduces this miscalculation. I could put
statistics-infrastructure-make-printk_clock-a-generic-kernel-wide-nsec-resolution.patch
into that one as well, so that all the timestamp related printk-changes are in
one place.

-
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/