----- Original Message -----
 From: "Dave Anderson" <anderson(a)redhat.com>
 To: "Discussion list for crash utility usage, maintenance and development"
<crash-utility(a)redhat.com>
 Sent: Tuesday, 21 May, 2013 7:36:57 PM
 Subject: Re: [Crash-utility] [Patch v4] Show module taint flags
 
 
 
 Hi Aaron,
 
 With respect to your v4 version of the patch, I just found a few
 issues that need addressing.
 
 The module.gpgsig_ok "unsigned" check was only made if module.taints
 exists, and not checked if module.license_gplok exists.  Earlier
 kernels have license_gplok/gpgsig_ok combinations.
 
 I'm not sure how you were able to get this sample display from your
 help page:
 
    crash> mod -T
      NAME                 TAINT
      dm_mod               G
      scsi_tgt             G
      serio_raw            G
      dm_log               G
      ata_generic          G
      qla2xxx              P(U)
      dm_region_hash       G
      enclosure            G
      pata_acpi            G
  
Yes, this is a relic from an older patch, which wasn't updated.
 because the module.taints field would be 0 for all but the
 qla2xxx module, and your code gates the tnts[] true/false display
 by "if (taints)"?  In my tests of your patch, the 'G' would only
 be shown if some *other* bit in the bitfield were set.
 
 Anway, I don't feel that it's necessary to print the 'G' for GPL'd
 modules.  We could probably just drop the "false" bit check entirely,
 but perhaps there will be something put there in the future that
 would be worthy of displaying?  But it would only get picked up if
 some *other* bit in the mask were set, so for now it's pretty much
 useless code.
  
Agreed.
 The module maxnamelen should be restricted to the modules that
 are actually tainted or unsigned.
 
 Another issue -- up until 2.6.19, the module->license_gplok was
 a boolean, where "1" meant it was GPL.  RHEL5 completely turned
 that around, and made license_gplok a bitmask, whereas upstream
 2.6.18 still has it as a boolean.  So since we're keying on non-zero
 values in that field, it confuses the issue because, say on a RHEL4
 kernel, it will show *all* GPL modules as having a hexadecimal value
 of 1.  But I can't find an obvious way of determining what's in play
 for a given kernel, so the "consult the kernel sources" reference
 in the help page will have to suffice.
 
 Anyway, in order to get some traction in getting this patch into
 place, I've reworked it a bit, and have attached my counter-proposal
 patch.  Here's what I've changed:
 
 (1) The '-t' and '-T' difference is too confusing.  I've changed it
     to simply be 't', and the display will be symbolic (the letter)
     if tnts[] exists, and hexadecimal if not.  The unsigned indication
     will always be shown if gdbsig_ok exists and is 0.
 
 (2) If module.taints exists, the header shows "TAINTS".  For older
     kernels, it shows "LICENSE_GPLOK".
     
 (3) I moved the module structure's gpgsig_ok, taints and license_gplok
     members, and the tnt structure's bit, true, false members into
     the offset_table, and tnt structure size into the size_table.
     This prevents having to dive into gdb each time MEMBER_EXISTS()
     and MEMBER_OFFSET are called, and will catch any changes in
     the structure names in the future.
     
 (4) When walking through the modules, readmem() each module's module
     struct into a buffer and then access the relevant fields with the
     UINT() and INT() macros, making things a bit less verbose.
 
 (5) Never show 'G'.
     
 (6) Set the maxnamelen only according to tainted/unsigned modules.
 
 (7) The help page has been changed to only have '-t' as an option,
     as well as indicating that the relevant taints/license_gplok
     field will only be shown if they are non-zero.  The kernel
     source reference is especially relevant given that old
     kernels with boolean license_gplok fields will show "1"
     for all modules.
 
 Let me know if you have any problems with, or find any bugs in,
 the attached patch, and if not, let's queue it for crash-7.0.1.
  
This looks fine.
Thanks for showing interesting in this feature. I'm positive that it
will be a welcomed addition to an already extensive command set.
Cheers,
Aaron