Thank you, especially for the heroic examination of all those
mapfiles.
Comments are inline below.
I've updated the webrevs:
Externally:
http://cr.opensolaris.org/~alib/mapfilechk.heads_up
On SWAN:
http://linkers.central/webrev/6785284
and Heads Up:
http://cr.opensolaris.org/~alib/mapfilechk.heads_up
James Carlson wrote:
> 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
Sorry, yes. This usually happens when I cut/paste from my
browser and don't notice.
In fact, there are two of these, linkers.central and linkers.sfbay,
either of which will work. I'm in .central myself, so I know what
you're saying...
> General:
>
> There should be a prototype mapfile in usr/src/prototypes/.
I created usr/src/prototypes/prototype.mapfile-vers
> 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.
It's not entirely so. Even within the mapfile-vers convention, there
are variations (e.g. mapfile-64-vers) that I'd rather not have to track,
and there are some mapfiles that do, or potentially could, specify versions,
but which are not named mapfile-vers. Although we have had the mapfile-vers
convention, I don't think it's been widely known (hopefully we're doing
something about that here).
There are also borderline cases, such as
usr/src/uts/sparc/krtld/mapfile
which do not technically specify a version, but which are *very*
close. I feel that a person modifying such a file really needs to
have read README.mapfile.
Also, I drew a line at renaming existing mapfiles, mainly for scope
creep reasons. Having looked at all these files, I think that the
right policy is not to put too much faith in the precise name of
the mapfile.
I went on about this in my response to Mark as well, so I'll stop
here.
>
> 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.
I've not heard that. My understanding is, if you touch it, you
update the copyright. I think copyrightchk enforces this, and
isn't it the rule that 'hg pbchk' needs to be clean?
In any event, I would certainly appreciate not having to go
back and edit these dates...
> 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.
Without disagreeing about the specifics of pppd, I want to limit exclusions
as much as possible, and hit everything that even remotely approaches the
material in README.mapfiles. It would be good for anyone touching pppd to
read README.mapfiles, and the comment does no harm.
>
> 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.
Or maybe 'interface'? That was my guess as to what 'intf' means.
I largely agree, but think they are in the "close enough"
category. I'm not worried about the future maintenance, because
it's so easy to do, and because the odds are that you start a new
mapfile by making a copy of an existing one (almost all of which have
the comment).
If experience proves me wrong, and this policy is deemed too noisy, we
can always dial up the exclusions later.
>
> 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.
More commonly though, it's a bad error.
>
> Maybe more importantly: the ARC manages the rules for public
> interfaces, not README.mapfiles or the tools community.
I've changed it to:
Scope reduction of Public interfaces is not allowed without
specific ARC review and approval.
>
> usr/src/lib/libdscfg/common/mapfile-vers
>
> 26: nit: would have put this local comment after the mapfile header.
done
>
> usr/src/lib/libkrb5/common/mapfile
>
> 25-30: ditto.
done
>
> 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.
This was all done with a script that simply located the
Sun copyright and put the comment directly after.
Fixed.
>
> usr/src/lib/libnsctl/common/mapfile-vers
> usr/src/lib/libpcp/common/mapfile-vers
>
> 24: position.
done
>
> usr/src/lib/librdc/common/mapfile-vers
>
> 25: position.
done
>
> usr/src/lib/libsqlite/mapfile-sqlite
>
> 6: position.
done
>
> usr/src/lib/libtsalarm/common/mapfile-vers
> usr/src/lib/libunistat/common/mapfile-vers
>
> 24: position.
done
>
> usr/src/lib/libwrap/mapfile
>
> 3: what license would that be?
There is a file named DISCLAIMER there that has a license,
and also a rather useless file named THIRDPARTYLICENSE.descrip.
As my project doesn't actually have anything to do with licenses,
and as this isn't my change (it was approved and has integrated
previously), I'm going to leave it as is.
On the other hand, if this is obviously wrong, and you know what
should be done to fix it, I'd be happy to put that in with my
putback --- let me know what to do.
>
> 5-6: position.
done
>
> 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.
done
>
> 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.
OK. There was some conversation about this, and we decided
to yank them, but I was uneasy about it. They're back.
>
> usr/src/lib/print/libprint/common/mapfile-vers
>
> 25: position.
done
>
> usr/src/lib/print/mod_ipp/mapfile
>
> Restore RCS.
done
>
> usr/src/lib/rpcsec_gss/mapfile-vers
>
> 24: position.
done
>
> usr/src/lib/storage/liba5k/common/mapfile-vers
>
> 27: position.
done
>
> usr/src/lib/storage/libg_fc/common/mapfile-vers
>
> 26: position.
done
>
> 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.
Nice! done.
>
> 52: this can go away if the above is done.
done
>
> usr/src/tools/onbld/Checks/CmtBlk.py
>
> 60,79, et cetera: should probably be "in_cmt" rather than "in_cddl".
yes, fixed.
>
> usr/src/tools/onbld/hgext/cdm.py
>
> 1031-1032: nit: should probably be in alpha order, somewhere around
> line 1042.
fixed
>
> 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;
Keying off $2 is definitely better. I also note that I never use
$1, so there's no reason to capture it with ()s like I was.
>
> 1093,1094: nit: random blank lines.
removed
>
> usr/src/tools/scripts/mapfilechk.1
>
> 47-49: nit: I'd probably use .TP for this.
This is carried from cddlchk.1. I'm not very deep with *roff,
but I copied the .TP use from one of our other manpages
(elfedit.1). and got rid of the .nf. I also did the
same for cddlchk.1.
>
> 65-66: what "command-line help functions?"
Also a carry-over from cddlchk.1. I have no idea what it means.
The code uses it for syntax errors. I've changed the text,
in both cddlchk.1 and mapfilechk.1 to be
2 Invalid command-line arguments were specified to the command.
>
> usr/src/tools/scripts/mapfilechk.py
>
> 57-62: what handles the "syntax: glob" in your exception list?
Nothing: Both 'hg mapfilechk' and the mapfilechk command line
interface will process any file that you explicitly point them at.
The command line version of mapfilechk requires an explicit file
argument, so it never uses the exception list.
However, when 'hg mapfilechk' is called without a file argument,
then it uses the active list for your workspace, filtered against
the exception list. That happens inside Cadmium (cdm.py).
>
> 76: nit: missing "return False"
>
> 104,107: 'if not' ... 'else' negative logic is hard to follow; try
> reversing.
Both of these are existing code taken from cddlchk.py. Unless you feel
strongly, I'm going to leave them as they are.
>
> usr/src/tools/scripts/nightly.sh
>
> 2997-2999: nit: suggest putting '|' at end of line. (So '\' isn't
> needed.)
>
I see your point. Have you noticed all the places in nightly.sh that
do the same thing though?
% grep '| \\$' nightly.sh | wc -l
94
I think I'll leave it like this and stick to the common style
of the existing script. Let me know if you disagree.
- Ali
