----- Original Message ----- > From: "Dave Anderson" <anderson@xxxxxxxxxx> > To: "Discussion list for crash utility usage, maintenance and development" <crash-utility@xxxxxxxxxx> > Sent: Tuesday, 21 May, 2013 7:36:57 PM > Subject: Re: [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 -- Crash-utility mailing list Crash-utility@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/crash-utility