On Tuesday 08 of January 2013 18:32:09 Nikola Pajkovsky wrote:

> Jakub Filak <jfilak@redhat.com> writes:

>

> > - closes rhbz#892230

> >

> > Signed-off-by: Jakub Filak <jfilak@redhat.com>

> > ---

> > src/lib/kernel.c | 145 ++++++++++++++++++++++++++++---------------------------

> > 1 file changed, 74 insertions(+), 71 deletions(-)

>

> ok, this definitely needs to be as two separate patches. first, and it

> could be vice versa, could be to moving suspicious string out of

> koops_print_suspicious_strings to s_suspicious_strings.

 

Sorry, I was lazy. I'll split it.

 

> Why did you use

> *s_* prefix in s_suspicious_strings anyway? Correct name should be

> koops_suspicious_string_arr. And the second could be adding new strings

> to array of suspicious strings.

 

"s_" prefix means static global. We usually use "g_" prefix for global

variables.

 

>

> > diff --git a/src/lib/kernel.c b/src/lib/kernel.c

> > index 6b13362..32d05a0 100644

> > --- a/src/lib/kernel.c

> > +++ b/src/lib/kernel.c

> > @@ -74,25 +74,58 @@ static void record_oops(GList **oops_list, struct line_info* lines_info, int oop

> > VERB3 if (rv == 0) log("Dropped oops: too short");

> > }

> >

> > +/* In some comparisons, we skip 1st letter, to avoid dealing with

> > + * changes in capitalization in kernel. For example, I see that

> > + * current kernel git (at 2011-01-01) has both "kernel BUG at ..."

> > + * and "Kernel BUG at ..." messages, and I don't want to change

> > + * the code below whenever kernel is changed to use "K" (or "k")

> > + * uniformly.

> > + */

> > +static const char *const s_suspicious_strings[] = {

> > + "BUG:",

> > + "WARNING: at",

> > + "INFO: possible recursive locking detected",

> > + /*k*/"ernel BUG at",

> > + "list_del corruption",

> > + "list_add corruption",

> > + "do_IRQ: stack overflow:",

> > + /*n*/"ear stack overflow (cur:",

> > + /*g*/"eneral protection fault",

> > + /*u*/"nable to handle kernel",

> > + /*d*/"ouble fault:",

> > + "RTNL: assertion failed",

> > + /*e*/"eek! page_mapcount(page) went negative!",

> > + /*b*/"adness at",

> > + "NETDEV WATCHDOG",

> > + /*s*/"ysctl table check failed",

> > + ": nobody cared",

> > + "IRQ handler type mismatch",

> > +

> > + /* X86 TRAPs */

> > + "divide error:",

> > + "bounds:",

> > + "coprocessor segment overrun:",

> > + "invalid TSS:",

> > + "segment not present:",

> > + "invalid opcode:",

> > + "alignment check:",

> > + "stack segment:",

> > + "fpu exception:",

> > + "simd exception:",

> > + "iret exception:",

> > +

> > + /* Termination */

> > + NULL

> > +};

> > +

> > void koops_print_suspicious_strings(void)

> > {

> > - fputs(

> > - "BUG" "\n"

> > - "corruption" "\n"

> > - "stack overflow" "\n"

> > - "protection fault" "\n"

> > - "WARNING: at" "\n"

> > - /*u*/ "nable to handle" "\n"

> > - /*d*/ "ouble fault:" "\n"

> > - "RTNL: assertion failed" "\n"

> > - /*e*/ "eek! page_mapcount(page) went negative!" "\n"

> > - /*b*/ "adness at" "\n"

> > - "NETDEV WATCHDOG" "\n"

> > - /*s*/ "ysctl table check failed" "\n"

> > - "INFO: possible recursive locking detected" "\n"

> > - ": nobody cared" "\n"

> > - "IRQ handler type mismatch" "\n"

> > - , stdout);

> > + const char *const *str = s_suspicious_strings;

> > + while (*str)

> > + {

> > + puts(*str);

> > + ++str;

> > + }

> > }

>

> why not use

>

> for (; *str; ++str) {}

 

ok

 

>

> or may be even better (and you have here remove NULL from array)

>

> for (int i = 0; i < ARRAY_SIZE(arr); i++) {}

 

I was thinking about this construction but I found it not so clear.

And please, don't ask me why. :)

 

And with terminating NULL, you can easily pass the array to a function

as argument without any additional argument for length.

 

>

> in any case of mentioned above, code will be more readable and the rest

> of the code is in the same flavor.

>

> >

> > void koops_extract_oopses(GList **oops_list, char *buffer, size_t buflen)

> > @@ -216,55 +249,16 @@ next_line:

> > if (oopsstart < 0)

> > {

> > /* Find start-of-oops markers */

> > - /* In some comparisons, we skip 1st letter, to avoid dealing with

> > - * changes in capitalization in kernel. For example, I see that

> > - * current kernel git (at 2011-01-01) has both "kernel BUG at ..."

> > - * and "Kernel BUG at ..." messages, and I don't want to change

> > - * the code below whenever kernel is changed to use "K" (or "k")

> > - * uniformly.

> > - */

> > - if (strstr(curline, /*g*/ "eneral protection fault:"))

> > - oopsstart = i;

> > - else if (strstr(curline, "BUG:"))

> > - oopsstart = i;

> > - else if (strstr(curline, /*k*/ "ernel BUG at"))

> > - oopsstart = i;

> > - /* WARN_ON() generated message */

> > - else if (strstr(curline, "WARNING: at "))

> > - oopsstart = i;

> > - else if (strstr(curline, /*u*/ "nable to handle kernel"))

> > - oopsstart = i;

> > - else if (strstr(curline, /*d*/ "ouble fault:"))

> > - oopsstart = i;

> > - else if (strstr(curline, "do_IRQ: stack overflow:"))

> > - oopsstart = i;

> > - else if (strstr(curline, "RTNL: assertion failed"))

> > - oopsstart = i;

> > - else if (strstr(curline, /*e*/ "eek! page_mapcount(page) went negative!"))

> > - oopsstart = i;

> > - else if (strstr(curline, /*n*/ "ear stack overflow (cur:"))

> > - oopsstart = i;

> > - else if (strstr(curline, /*b*/ "adness at"))

> > - oopsstart = i;

> > - else if (strstr(curline, "NETDEV WATCHDOG"))

> > - oopsstart = i;

> > - else if (strstr(curline, /*s*/ "ysctl table check failed"))

> > - oopsstart = i;

> > - else if (strstr(curline, "INFO: possible recursive locking detected"))

> > - oopsstart = i;

> > - // Not needed: "--[ cut here ]--" is always followed

> > - // by "Badness at", "kernel BUG at", or "WARNING: at" string

> > - //else if (strstr(curline, "------------[ cut here ]------------"))

> > - // oopsstart = i;

> > - else if (strstr(curline, "list_del corruption"))

> > - oopsstart = i;

> > - else if (strstr(curline, "list_add corruption"))

> > - oopsstart = i;

> > - /* "irq NN: nobody cared..." */

> > - else if (strstr(curline, ": nobody cared"))

> > - oopsstart = i;

> > - else if (strstr(curline, "IRQ handler type mismatch"))

> > - oopsstart = i;

> > + const char *const *str = s_suspicious_strings;

> > + while (*str)

> > + {

> > + if (strstr(curline, *str))

> > + {

> > + oopsstart = i;

> > + break;

> > + }

> > + ++str;

> > + }

> >

> > if (oopsstart >= 0)

> > {

> > @@ -335,14 +329,23 @@ next_line:

> > oopsend = i-1;

> > else if (strstr(curline, "Instruction dump:"))

> > oopsend = i;

> > - /* if a new oops starts, this one has ended */

> > - else if (strstr(curline, "WARNING: at ") && oopsstart != i) /* WARN_ON() generated message */

> > - oopsend = i-1;

> > - else if (strstr(curline, "Unable to handle") && oopsstart != i)

> > - oopsend = i-1;

> > /* kernel end-of-oops marker (not including marker itself) */

> > else if (strstr(curline, "---[ end trace"))

> > oopsend = i-1;

> > + else

> > + {

> > + /* if a new oops starts, this one has ended */

> > + const char *const *str = s_suspicious_strings;

> > + while (*str)

> > + {

> > + if (strstr(curline, *str))

> > + {

> > + oopsend = i-1;

> > + break;

> > + }

> > + ++str;

> > + }

> > + }

> >

> > if (oopsend <= i)

> > {

>

>