286685 - Rename all softoken private functions and types from PK11 to SFTK

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement)

Description

[Nelson Bolyard (seldom reads bugmail)]

cmd/bltest/blapitest.c
lib/pk11wrap/pk11pars.c
lib/softoken/dbinit.c
lib/softoken/dbmshim.c
Presently all the code having anything to do with PKCS11 has function
names and type names that begin with PK11_ or pk11_, whether inside
the PKCS11 softoken module (where all symbols not defined by the PKCS11 
spec should be private) or outside the softken (e.g. in pk11wrap).

This has been a great cause of confusion for developers who haven't
memorized this code.  When one is debugging, one cannot tell by looking
at the source files, or the names of the functions on the stacks, or 
the names of the types, whether one is looking at code below the 
PKCS11 "line" (inside a PKCS11 module) or above that line (outside the
module).

IMO, the PK11_ functions should all be used for the pk11wrapper layer
exclusively.  The softoken module should use some (ANY) other prefix.

Therefore I propose to rename all functions in softoken to begin 
with SFTK_ or sftk_, and rename all types and defined symbols 
defined in NSS-private header files in lib/softoken to begin with 
SFTK instead of PK11.

I have already done this work.  I have changed all (or nearly all)
the NSS-private header files in softoken, and all (or nearly) the .c 
files in softoken.  I did not change the PUBLIC header files in 
softoken that define the PKCS11 public API.  

The ONLY problems were due to the code in softoken/pk11pars.h (which 
is really a .c file!) which gets compiled and linked into both 
softoken and pk11wrap.  The source uses symbols from both softoken
and from pk11wrap.  Maybe the many functions defined in this file 
should have another prefix that is neither PK11 nor SFTK, but rather
SMOD or SECM or SECMOD or secmod.  But what I did was to rename all
these functions from pk11_ to sftk_.  

Only 2 files outside of lib/softoken needed to be changed.  They were:

cmd/bltest/blapitest.c   (which calls softoken's fips selftest function)
lib/pk11wrap/pk11pars.c  (which calls the functions in pk11pars.h 
                          mentioned above.)

The files changed in lib/softoken were:

lib/softoken/softoken.h
lib/softoken/pkcs11i.h
lib/softoken/pk11pars.h

lib/softoken/fipstest.c
lib/softoken/fipstokn.c
lib/softoken/pcertdb.c
lib/softoken/pk11db.c
lib/softoken/pkcs11.c
lib/softoken/pkcs11c.c
lib/softoken/pkcs11u.c
lib/softoken/tlsprf.c

I will attach a zip file containing the diffs.  The diffs are HUGE, which 
is why I'm zipping them up.  They were all made with a "global search and
replace" script (gsr), except for some of the ones in these files:

cmd/bltest/blapitest.c   changed one func from pk11_ to sftk_
lib/pk11wrap/pk11pars.c  changed numerous pk11pars.h funcs from pk11_ to sftk_
lib/softoken/pk11pars.h  after gsr, had to change some symbols back to PK11_
lib/softoken/pk11db.c    ditto 

So, I suggest the reviewers should focus on those 4 files, and maybe the
header files.  If the headers are right and those 4 are right, the rest
should also be right because of the use of the gsr tool.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: zip file of huge patch

Must have had a cut-n-paste error with the list of files above.
The attached zip file is 100KB.  It expands to abour 500KB.
and contains about 17K lines of patch.	

The complete list of files modified is (in order):

cmd/bltest/blapitest.c
lib/pk11wrap/pk11pars.c
lib/softoken/dbinit.c
lib/softoken/dbmshim.c
lib/softoken/fipstest.c
lib/softoken/fipstokn.c
lib/softoken/pcertdb.c
lib/softoken/pk11db.c
lib/softoken/pk11pars.h
lib/softoken/pkcs11.c
lib/softoken/pkcs11c.c
lib/softoken/pkcs11i.h
lib/softoken/pkcs11u.c
lib/softoken/softoken.h
lib/softoken/tlsprf.c

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

With the above patch in place, the only remaining occurrences of the strings
PK11 or pk11 in lib/softoken are these:

manifest.mn:    pk11pars.h \
manifest.mn:    pk11db.c \
pk11db.c:#include "pk11pars.h"
pk11db.c:    PK11PreSlotInfo *slotInfo = NULL;
pk11pars.h:#include "pk11init.h"
pk11pars.h:sftk_argDecodeSingleSlotInfo(char *name,char *params,PK11PreSlotInfo
*slotInfo)
pk11pars.h:     slotInfo->defaultFlags |= PK11_OWN_PW_DEFAULTS;
pk11pars.h:static PK11PreSlotInfo *
pk11pars.h:    PK11PreSlotInfo *slotInfo = NULL;
pk11pars.h:     slotInfo = (PK11PreSlotInfo *)
pk11pars.h:                     PORT_ArenaAlloc(arena,count*sizeof(PK11PreSlotIn
fo));
pk11pars.h:     PORT_Memset(slotInfo,0,count*sizeof(PK11PreSlotInfo));
pk11pars.h:     slotInfo = (PK11PreSlotInfo *)
pk11pars.h:                     PORT_ZAlloc(count*sizeof(PK11PreSlotInfo));
pk11pars.h:    if (defaultFlags & PK11_OWN_PW_DEFAULTS) {

The type "PK11PreSlotInfo" is defined in a header outside of softoken, 
and is used only in that one file whose source is compiled in both softoken
and in pk11wrap, so I left that type name intact.  PK11_OWN_PW_DEFAULTS is
a similar situation, defined outside softoken, used only in shared code.

Comment 3

[Wan-Teh Chang]

could you attach the script you used?

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

This script is gsr.  It is invoked as 
   gsr  oldstring  newstring  filenames...
e.g. 
   gsr  PK11 SFTK *.[ch] 
or
   find . -name \*.[ch] -print | xargs gsr PK11 SFTK 

#! /bin/sh
OLD_STRING="$1"
shift
NEW_STRING="$1"
shift
while [ $# -gt 0 ]
do
    echo ================== $1 ==================
    ex $1 <<__EOF__
g/$OLD_STRING/s//$NEW_STRING/g
wq
__EOF__
    shift
done


This script is gsrword.  same as gsr, but only changes strings
that meet vi/ex's definition of a whole word.

#! /bin/sh
OLD_STRING="$1"
shift
NEW_STRING="$1"
shift
while [ $# -gt 0 ]
do
    echo ================== $1 ==================
    ex $1 <<__EOF__
g/\<$OLD_STRING\>/s//$NEW_STRING/g
wq
__EOF__
    shift
done

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

As I recall, I used gsr to change all PK11 to SFTK and all pk11 to sftk 
in lib/softoken, then I used gsrword to change back some symbols, e.g.
   sftkpars -> pk11pars
   sftkdb   -> pk11db
   sftkinit -> pk11init
   SFTKPreSlotInfo -> PK11PreSlotInfo
   SFTK_OWN_PW_DEFAULTS -> PK11_OWN_PW_DEFAULTS
Finally I did a gmake clean export libs, and fixed the problems,
which were in 
   cmd/bltest/blapitest.c
   lib/pk11wrap/pk11pars.c

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch revised for removal of PK11_USE_THREADS

This patch is a revision of the previous patch.  
It should be the same as the previous patch, except for the removal
of the PK11_USE_THREADS and related macros that was just checked in.
It's zipped because it's too big for bugzilla to take as a single patch.

The two patched files outside of lib/softken were edited by hand.
All the changes to lib/softoken in this patch were generated with the
following shell commands:

grep -li pk11 *.* > files2modify.txt
gsr PK11 SFTK $(cat files2*txt)
gsr pk11 sftk $(cat files2*txt)
gsrword sftkdb pk11db $(cat files2*txt)
gsrword sftkpars pk11pars $(cat files2*txt)
gsrword sftkinit pk11init $(cat files2*txt)
gsrword SFTKPreSlotInfo PK11PreSlotInfo $(cat files2*txt)
gsrword SFTK_OWN_PW_DEFAULTS PK11_OWN_PW_DEFAULTS  $(cat files2*txt)

Because the changes were made mechanically, I suggest that reviewers
review the header files that changed, and the two files outside of 
lib/softken that changed, and only skim the rest.  

all.sh passes on windows with this patch in place.  
(Haven't tested on other boxes.)

I'd like to get this fix in ASAP.  The longer we wait, the more patch
collisions will be created.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 177831 [details]
zip file of huge patch

cancelling review requests for old megapatch that was superseded by the new
megapatch.

Comment 8

[Robert Relyea]

Comment on attachment 178859 [details]
patch revised for removal of PK11_USE_THREADS

The only non-mechanical part of the patch is the pk11pars stuff. I'm not sure
if that should have it's own prefix since it strattles the pk11wrap and
softoken. On the other hand it's mostly in softoken, so the existing patch is
at least better then the existing code, and perhaps the best choice.

I noticed the patch does not yet change file names (good, since it makes it
easier to review).

I also notice softoken has some secmod_ functions (a prefix which also exists
in pk11wrap).

All the problems I identified are pre-existing to the patch, and the patch
makes large strides forward, so I see no reason not to approve it

r+ rrelyea

Comment 9

[Wan-Teh Chang]

Comment on attachment 178859 [details]
patch revised for removal of PK11_USE_THREADS

It doesn't seem right to use the sftk prefix in pk11pars.h.  Such a big patch
is also risky after Beta 2.

But I've verified that no public header files are modified, so if there are no
compiler warnings and the code still compiles, this should be a correct patch.

Comment 10

[Wan-Teh Chang]

Comment on attachment 178859 [details]
patch revised for removal of PK11_USE_THREADS

In lib/pk11wrap/pk11pars.c,  you made this change:

 static char *
-pk11_mkModuleSpec(SECMODModule * module)
+secmod_mkModuleSpec(SECMODModule * module)
 {

Why don't you use the sftk prefix for this function?

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #10)

> Why don't you use the sftk prefix for this function?

This function is not part of softoken.  It is defined in PK11wrap, but
It does not make use of the PKCS11 API.  It is not a wrapper for
the PKCS11 API.  It is part of the code that loads PKCS11 modules
using the secmod DB.  So, since this code is clearly related to 
secmod.db, I thought the secmod prefix was more fitting.  

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

cmd/bltest/blapitest.c:  new revision: 1.41; previous revision: 1.40
lib/pk11wrap/pk11pars.c: new revision: 1.19; previous revision: 1.18
lib/softoken/dbinit.c:   new revision: 1.25; previous revision: 1.24
lib/softoken/dbmshim.c:  new revision: 1.11; previous revision: 1.10
lib/softoken/fipstest.c: new revision: 1.8;  previous revision: 1.7
lib/softoken/fipstokn.c: new revision: 1.8;  previous revision: 1.7
lib/softoken/pcertdb.c:  new revision: 1.49; previous revision: 1.48
lib/softoken/pk11db.c:   new revision: 1.31; previous revision: 1.30
lib/softoken/pk11pars.h: new revision: 1.17; previous revision: 1.16
lib/softoken/pkcs11.c:   new revision: 1.99; previous revision: 1.98
lib/softoken/pkcs11c.c:  new revision: 1.60; previous revision: 1.59
lib/softoken/pkcs11i.h:  new revision: 1.36; previous revision: 1.35
lib/softoken/pkcs11u.c:  new revision: 1.65; previous revision: 1.64
lib/softoken/softoken.h: new revision: 1.7;  previous revision: 1.6
lib/softoken/tlsprf.c:   new revision: 1.5;  previous revision: 1.4

Before marking this fixed, I'd like to discuss doing something more 
about pk11pars.h, which is really a .c file that gets compiled as part 
of softoken AND as part of pk11wrap.    I'd like that file to be named
with a .c suffix, since it is not a header in any conventional sense. 
I think it was named .h simply because it is included by another file,
but we have other examples of .c files including other .c files.  

Perhaps the functions there should be renamed secmod_ ?
or some other prefix to convey their shared nature?  
How about tknstr_ to show they are related to strings for softoken?
(Sorry, no really good ideas for that prefix)

Comment 13

[Wan-Teh Chang]

The two .c files that include pk11pars.h (pk11wrap/pk11pars.c
and softoken/pk11db.c) use secmod as the prefix of their
functions.  So secmod seems like an appropriate prefix for
the identifiers in pk11pars.h.

Comment 14

[Robert Relyea]

I concur...

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Attachment: rename pk11pars.h symbols to secmod*

This patch affects only 3 files.  I mechanically changed the sftk and SFTK 
symbols defined in pk11pars.h to begin with secmod or SECMOD, with two 
exceptions:  SFTK_DEFAULT_CIPHER_ORDER and SFTK_DEFAULT_TRUST_ORDER
which I left named SFTK because they really seem like defaults for softoken
and not for other PKCS11 modules.  If you think it right to change those two
symbols also, please advise.  

It compiles and passes all.sh on my machine.

Comment 16

[Robert Relyea]

yes, those only apply to softoken, so the SFTK names are good.

bob

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Bob, Is your comment 15 meant to imply r+ for the patch (attachment 179003 [details] [diff] [review]) ?

Comment 18

[Robert Relyea]

Comment on attachment 179003 [details] [diff] [review]
rename pk11pars.h symbols to secmod*

It was meant to approve the design. I hadn't reviewed the patch itself. r+.

BTW, looking at the patch, I mistook SFTK_DEFAULT_CIPHER for a different
default. DEFAULT_CIPHER and DEFAULT_TRUST is the default for non-internal
tokens. The defaults for the softtoken is defined in pk11db.c
(PK11_DEFAULT_INTERN_INIT,  cipherOrder = 50 trustOrder=75).

bob

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #18)
> BTW, looking at the patch, I mistook SFTK_DEFAULT_CIPHER for a different
> default. DEFAULT_CIPHER and DEFAULT_TRUST is the default for non-internal
> tokens. 

Bab, are you saying that you want me to rename SFTK_DEFAULT_CIPHER_ORDER and
SFTK_DEFAULT_TRUST_ORDER to begin with SOFTOKEN before checking in?

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

I asked: 

> Bob, are you saying that you want me to rename SFTK_DEFAULT_CIPHER_ORDER 
> and SFTK_DEFAULT_TRUST_ORDER to begin with SOFTOKEN before checking in?

I meant SECMOD, not SOFTOKEN.  sorry.

Comment 21

[Robert Relyea]

I wasn't making it a prereq for checkin, but it's certainly something I'd like
to see.

bob

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 179003 [details] [diff] [review]
rename pk11pars.h symbols to secmod*

Checked in this patch.	
lib/pk11wrap/pk11pars.c; new revision: 1.20; previous revision: 1.19
lib/softoken/pk11db.c;	 new revision: 1.32; previous revision: 1.31
lib/softoken/pk11pars.h; new revision: 1.18; previous revision: 1.17

Will do one more patch for those two remaining SFTK symbols.

Comment 23

[Julien Pierre]

Attachment: remaining macros

Bob, I think this is what you wanted. It should be a very quick review.

Comment 24

[Robert Relyea]

Comment on attachment 202450 [details] [diff] [review]
remaining macros

r+ yes, that will do.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 202450 [details] [diff] [review]
remaining macros

groovy

Comment 26

[Julien Pierre]

Bob, Nelson, thanks for the reviews. I checked this patch in to the tip. I believe it was the only thing left in this bug, so I'm closing it.

Comment 27

[Wan-Teh Chang]

The majority of this work was done in NSS 3.10.
It was completed in NSS 3.11 (the "remaining macros"
patch).