On Wednesday, November 24, 2010 05:20:52 pm Denys Vlasenko wrote:
On Wed, 2010-11-24 at 15:29 +0100, Nikola Pajkovsky wrote:
> - char tainted[2] = {'x', '\0'};
> + char tainted_str[sizeof(unsigned long) * 3 + 1] = {'\0'};
The code later does not try to convert the contents in any way,
so having exactly sizeof(unsigned long) * 3 + 1 bytes is unimpotant,
and is even a bit misleading - the reader of the code will be let
to think we plan to parse the content. We don't.
Therefore I'd just use "char tainted_str[SOME_NUM];"
or even use FILE-based IO - it has a nice "read one line, however long
it is" function:
FILE *fp = fopen...
if (fp) {
char *tainted_str = xmalloc_fgetline(fp);
...
free(tainted_str);
fclose(fp);
}
No need to think "how big char tainted_str[SOME_NUM] should be?"
nice, I will use it...
> /* once tainted flag is set to 1, only restart can reset
the flag to
> 0 */ int tainted_fd = open("/proc/sys/kernel/tainted", O_RDONLY);
> if (tainted_fd >= 0)
> {
>
> - /* contain only 0 or 1 */
> - if (read(tainted_fd, &tainted, 1) != 1)
> + /* this file contains a decimal representation of a bitmask */
This comment is not needed. Yes, file contains a number.
But it is not relevant *for this piece of code*.
> + if (read(tainted_fd, tainted_str, sizeof(tainted_str)) == -1)
There will be no NUL byte if read gets exactly sizeof(tainted_str)
bytes. Use sizeof(tainted_str)-1.
> error_msg("Unable to read one byte from
> /proc/sys/kernel/tainted");
>
> -
The error message needs changing too.
I'd go with simple "Can't read /proc/sys/kernel/tainted".
> @@ -158,8 +157,8 @@ int save_oops_to_debug_dump(GList **oopsList)
>
> strchrnul(second_line, '\n')[0] = '\0';
> dd_save_text(dd, FILENAME_REASON, second_line);
>
> - if (tainted[0] == '1')
> - dd_save_text(dd, FILENAME_TAINTED, tainted);
> + if (tainted_str[0])
> + dd_save_text(dd, FILENAME_TAINTED, tainted_str);
Might also do not write the file if tainted_str == "0":
if (tainted_str[0] && tainted_str[0] != '0') ...
> diff --git a/src/plugins/abrt-action-bugzilla.cpp
> b/src/plugins/abrt-action-bugzilla.cpp index 073d534..cdbe344 100644
> --- a/src/plugins/abrt-action-bugzilla.cpp
> +++ b/src/plugins/abrt-action-bugzilla.cpp
> @@ -382,6 +382,68 @@ int ctx::add_comment(xmlrpc_int32 bug_id, const
> char* comment, bool is_private)
>
> return result ? 0 : -1;
>
> }
>
> +/* From RHEL6 kernel/panic.c:
> + * { TAINT_PROPRIETARY_MODULE, 'P', 'G' },
> + * { TAINT_FORCED_MODULE, 'F', ' ' },
> + * { TAINT_UNSAFE_SMP, 'S', ' ' },
> + * { TAINT_FORCED_RMMOD, 'R', ' ' },
> + * { TAINT_MACHINE_CHECK, 'M', ' ' },
> + * { TAINT_BAD_PAGE, 'B', ' ' },
> + * { TAINT_USER, 'U', ' ' },
> + * { TAINT_DIE, 'D', ' ' },
> + * { TAINT_OVERRIDDEN_ACPI_TABLE, 'A', ' ' },
> + * { TAINT_WARN, 'W', ' ' },
> + * { TAINT_CRAP, 'C', ' ' },
> + * { TAINT_FIRMWARE_WORKAROUND, 'I', ' ' },
> + * entries 12 - 27 are unused
> + * { TAINT_HARDWARE_UNSUPPORTED, 'H', ' ' },
> + * entries 29 - 31 are unused
> + */
> +
> +static const char *taint_warnings[] = {
static const char *const taint_warnings[]
fixed
> + "Proprietary Module",
> + "Forced Module",
> + "Unsafe SMP",
> + "Forced rmmod",
> + "Machine Check",
> + "Bad Page",
> + "User",
> + "Die",
> + "Overriden ACPI Table",
> + "Warning Issued",
> + "Experimental Module Loaded",
> + "Firmware Workaround",
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + "Hardware Unsupported",
> + NULL,
> + NULL,
> +};
> +
> +static const char *tainted_string(unsigned long tainted)
> +{
> + unsigned long idx = 0;
Why "long"?
hmm, right, too big, changed to _unsigned_
> + while (tainted >>= 1)
> + idx++;
> +
> + return taint_warnings[idx];
> +}
> +
>
> xmlrpc_int32 ctx::new_bug(const map_crash_data_t& pCrashData, int
> depend_on_bugno) {
>
> const char *package =
> get_crash_data_item_content_or_NULL(pCrashData, FILENAME_PACKAGE);
>
> @@ -392,7 +454,7 @@ xmlrpc_int32 ctx::new_bug(const map_crash_data_t&
> pCrashData, int depend_on_bugn
>
> const char *reason =
> get_crash_data_item_content_or_NULL(pCrashData, FILENAME_REASON);
> const char *function =
> get_crash_data_item_content_or_NULL(pCrashData,
> FILENAME_CRASH_FUNCTION); const char *analyzer =
> get_crash_data_item_content_or_NULL(pCrashData, FILENAME_ANALYZER);
>
> - const char *tainted =
> get_crash_data_item_content_or_NULL(pCrashData, FILENAME_TAINTED); +
> const char *tainted_str =
> get_crash_data_item_content_or_NULL(pCrashData, FILENAME_TAINTED);
>
> struct strbuf *buf_summary = strbuf_new();
> strbuf_append_strf(buf_summary, "[abrt] %s", package);
>
> @@ -403,11 +465,13 @@ xmlrpc_int32 ctx::new_bug(const map_crash_data_t&
> pCrashData, int depend_on_bugn
>
> if (reason != NULL)
>
> strbuf_append_strf(buf_summary, ": %s", reason);
>
> - if (tainted && analyzer
> - && (tainted[0] == '1')
> + if (tainted_str && analyzer
>
> && (strcmp(analyzer, "Kerneloops") == 0)
>
> ) {
>
> - strbuf_append_str(buf_summary, ": TAINTED");
> + unsigned long tainted = xatoi_u(tainted_str);
> + const char *tainted_warning = tainted_string(tainted);
> + if (tainted_warning)
> + strbuf_append_strf(buf_summary, ": TAINTED %s",
> tainted_warning);
>
> }
>
> char *status_whiteboard = xasprintf("abrt_hash:%s", duphash);
Next patch will be in a sec.
--
Nikola