[Crash-utility] [PATCH RFC 00/14] Minor code cleanups, round zero

[Dave Anderson]

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