On Tuesday 08 of January 2013 18:32:09 Nikola Pajkovsky wrote:
Jakub Filak <jfilak(a)redhat.com> writes:
> - closes rhbz#892230
>
> Signed-off-by: Jakub Filak <jfilak(a)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)
> {