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) {