Re: [Crash-utility] [PATCH RFC 00/14] Minor code cleanups, round zero
----- Original Message -----
Hi, Dave.
Thanks for your intensive review and accepting contribution.
Nevertheless, I'd appreciate if you explicitly comment on the following
series:
lkcd_v8: address of an array always evaluates to true
lkcd_v7: address of an array always evaluates to true
lkcd_v5: address of an array always evaluates to true
lkcd_v2_v3: address of an array always evaluates to true
lkcd_v1: address of an array always evaluates to true
Let me explain why I think this should be fixed.
dump_header_t is a struct that contains the following member:
/* the panic string, if available */
char dh_panic_string[DUMP_PANIC_LEN];
This struct is statically initialized with zeroes:
static dump_header_t dump_header_v8 = { 0 };
and assigned to dh via pointer. Thus, in the following expression:
dh && dh->dh_panic_string && strstr(dh->dh_panic_string,
"\n") ? "" : "\n"
once dh is non-NULL, dh->dh_panic_string also has some address within
a struct, but its value may be NULL. IIRC, passing NULL to strstr is
an undefined behaviour, so, looks like that this statement should be
rewritten to check whether dh_panic_string is NULL:
dh && *dh->dh_panic_string && strstr(dh->dh_panic_string,
"\n") ? "" : "\n"
or
dh && dh->dh_panic_string[0] && strstr(dh->dh_panic_string,
"\n") ? "" : "\n"
or even
dh && strlen(dh->dh_panic_string) && strstr(dh->dh_panic_string,
"\n")
? "" : "\n"
to avoid any confusion for potential code reviewer.
Am I correct?
Yes you are correct that the dh->dh_panic_string test will always
return TRUE, but that it's harmless. And it would never pass a NULL
pointer to strstr(), but rather a pointer to a string of NULL characters,
i.e., a legitimate string whose terminating NULL is the first character.
And FWIW, it's also meaningless to even make the "dh" check as well,
so the two-part "dh && dh->dh_panic_string" validation could simply
be removed.
But practically speaking -- and I don't mean to be flippant -- when
was the last time you worked with an LKCD dumpfile? It's a dead dumpfile
format, that was abandoned in ~2006. I think SUSE last used it in their
2.6.5-based SLES9 kernels, but thankfully kdump was accepted in the kernel,
and LKCD (and Red Hat's netdump) were finally and thankfully laid to rest.
Again, what I'm interested in is just keeping crash working/in-sync
with the constant onslaught of upstream kernel changes, fixing any existing
functional bugs, and adding new useful features.
Thanks,
Dave