Ali Bahrami writes: > I have written a heads up message that describes all of this in more > detail: > > http://cr.opensolaris.org/~alib/mapfilechk.heads_up > > Webrev for the change is available on SWAN at: > > http://linkers/webrev/6785284/index.html
Don't torture us non-CA people with domain squatters like "linkers.com". ;-} http://linkers.sfbay/webrev/6785284/index.html General: There should be a prototype mapfile in usr/src/prototypes/. I read through all 515 files. Those not listed here are ok by me. In looking over all of the changes, it seems to me like there *was* a naming convention already in place -- 'mapfile-vers' was used to name mapfiles that were used specifically for symbol versioning. All other mapfiles are for special hacks unrelated to versioning. I guess I'm a little surprised that this naming convention couldn't have been used to identify which files needed to be checked for versioning ... but I'm happy enough with the design. Also, I note that you've changed the copyright on all of these files. Maybe it's too late now, but my previous understanding was that copyright dates are not supposed to be changed if only trivial changes (such as changes to comments) are made, and *most* of these changes are in fact trivial. usr/src/cmd/cmd-inet/usr.bin/pppd/plugins/mapfile-minconn usr/src/cmd/cmd-inet/usr.bin/pppd/plugins/mapfile-passprompt usr/src/cmd/cmd-inet/usr.bin/pppd/plugins/mapfile-pppoe Should these perhaps be on the exception list? These "mapfiles" are for a plugin system, not for a versioned library, and they're oddly applied to the plugin components themselves rather than the framework. If this bit were done right, I *think* it's actually pppd that would have a mapfile rather than this component. (Mapfiles were introduced well after pppd was integrated, and this was retrofitted on top. I'm not sure it's right.) Maybe filing a bug saying that the pppd plugins mapfiles smell funny would be a better answer than trying to track them. usr/src/cmd/csh/mapfile-intf [... et cetera ...] These are other slightly weird mapfiles. They don't seem to be versioning symbols that are defined by these objects, but rather working around some odd issues with interposers (and an unusual mech_krb5 design problem). I _guess_ tracking them is ok, though it's slightly surprising and may mean some needless build errors for those doing future maintenance in these areas. usr/src/lib/README.mapfiles 346-347: nit in existing text: "not allowed" is just too strong here. Scope reduction of a public interface is equivalent to EOF, and thus requires specific ARC review and approval. True, many such proposals would simply be rejected by the ARC, but that doesn't mean that it can't _ever_ be done. Maybe more importantly: the ARC manages the rules for public interfaces, not README.mapfiles or the tools community. usr/src/lib/libdscfg/common/mapfile-vers 26: nit: would have put this local comment after the mapfile header. usr/src/lib/libkrb5/common/mapfile 25-30: ditto. usr/src/lib/libndmp/common/mapfile-vers 6-18: inserting this block *before* the BSD license notice doesn't seem right to me. I _think_ we have legal rules about where copyright notices and licenses go in a file ... and they're supposed to be first. usr/src/lib/libnsctl/common/mapfile-vers usr/src/lib/libpcp/common/mapfile-vers 24: position. usr/src/lib/librdc/common/mapfile-vers 25: position. usr/src/lib/libsqlite/mapfile-sqlite 6: position. usr/src/lib/libtsalarm/common/mapfile-vers usr/src/lib/libunistat/common/mapfile-vers 24: position. usr/src/lib/libwrap/mapfile 3: what license would that be? 5-6: position. usr/src/lib/nsswitch/dns/common/mapfile-vers usr/src/lib/nsswitch/files/common/mapfile-vers usr/src/lib/nsswitch/ldap/common/mapfile-vers usr/src/lib/nsswitch/mdns/common/mapfile-vers usr/src/lib/nsswitch/nis/common/mapfile-vers usr/src/lib/nsswitch/nisplus/common/mapfile-vers usr/src/lib/nsswitch/user/common/mapfile-vers 24: position. usr/src/lib/print/libhttp-core/common/mapfile usr/src/lib/print/libipp-core/common/mapfile usr/src/lib/print/libipp-listener/common/mapfile usr/src/lib/print/libpapi-common/common/mapfile usr/src/lib/print/libpapi-dynamic/common/mapfile usr/src/lib/print/libpapi-ipp/common/mapfile usr/src/lib/print/libpapi-lpd/common/mapfile I don't think the RCS identifiers in these files should have been removed. They're probably used for syncing upstream. Please restore. usr/src/lib/print/libprint/common/mapfile-vers 25: position. usr/src/lib/print/mod_ipp/mapfile Restore RCS. usr/src/lib/rpcsec_gss/mapfile-vers 24: position. usr/src/lib/storage/liba5k/common/mapfile-vers 27: position. usr/src/lib/storage/libg_fc/common/mapfile-vers 26: position. usr/src/tools/onbld/Checks/Cddl.py 3-21: besides the duplication, the somewhat unfortunate part of this change is that now there's nothing that actually checks the CDDL block used for checking. I would suggest doing this instead: CDDL = ''' CDDL HEADER START ... CDDL HEADER END ''' CDDL = CDDL.splitlines[2:-2] That last expression trims out the initial blank line, the CDDL HEADER START, and the CDDL HEADER END from the string. 52: this can go away if the above is done. usr/src/tools/onbld/Checks/CmtBlk.py 60,79, et cetera: should probably be "in_cmt" rather than "in_cddl". usr/src/tools/onbld/hgext/cdm.py 1031-1032: nit: should probably be in alpha order, somewhere around line 1042. usr/src/tools/scripts/check_rtime.pl 1085: this will also skip path names that happen to have "private" in them. I would suggest doing this instead on line 1088: OutMsg($$RefTtl++, $RelPath, "${tab}VERDEF=$2") unless $2 =~ /private/i; 1093,1094: nit: random blank lines. usr/src/tools/scripts/mapfilechk.1 47-49: nit: I'd probably use .TP for this. 65-66: what "command-line help functions?" usr/src/tools/scripts/mapfilechk.py 57-62: what handles the "syntax: glob" in your exception list? 76: nit: missing "return False" 104,107: 'if not' ... 'else' negative logic is hard to follow; try reversing. usr/src/tools/scripts/nightly.sh 2997-2999: nit: suggest putting '|' at end of line. (So '\' isn't needed.) -- James Carlson, Solaris Networking <james.d.carlson at sun.com> Sun Microsystems / 35 Network Drive 71.232W Vox +1 781 442 2084 MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
