466042 - (pk11parser) Remove function definitions from pk11pars.h

Status: RESOLVED DUPLICATE of bug 753116

(NSS :: Libraries, defect)

Description

[Elio Maldonado]

Attachment: code refactoring to move function definitions out of pk11parse header

The header mozilla/security/nss/lib/softtoken/pk11pars.h includes actual function definitions ( well as static variables) and shoudln't. This header is shared among by p11wrap/p11parse.c and softtoken/legacydb/pk11db.c and exports to them the otherwise private modsec_ pk11 parsing functions. All function implementations should be moved to a source file, preferably in utils which is accessible to both p11wrap and softtoken.

The attached patch and file is a refactoring of the pertinent code as follows:
Changed Files (all paths relative to mozilla/security/nss/lib/)
softtoken/pk11pars.h
- replace functions definitions with function declarations
- moved function definitions and static variables definitions to a source file - added header guards
softtoken/legacydb/pk11db.c
- added #include "utilrename.h" before the #include "pk11parse.h"
util/manifest.mn
- added pk11parse.c to the CSRCS list
util/nssutil.def
- exported the _secmod_xxx functions (with leading _)
util/rename.h
- added a renames of secmod_xxx functions of the form #define secmod_SomeFunction _secmod_SomeFunction
util/pk11parse.c (new)
- moved here the secmod_xxx functions code from pk11wrap.h + variables
- rename all functions by adding a _ suffix to their names
note: only parsing functions were moved, constructor type functions from pk11wrap/pk11parse.c remain there

Comment 1

[Elio Maldonado]

Attachment: secmod_ parsing functions now live here

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 349277 [details] [diff] [review]
code refactoring to move function definitions out of pk11parse header

You cannot put new symbols into a .def file in the middle of a block of 
symbols defined in a previous release.  

I may have more comments about this patch.

Comment 3

[Robert Relyea]

My main comments:

1. ditto nelson's comments about the .def file. Make you put it in the correct release/subrelease section of the .def file.

2. When attaching the patch, use CVS add to add any new files and use cvs diff -Nup to create the patch file. (this will include your new file in the patch)

3. We should move pk11pars.h to util. It should be exported NSS private.(Which means changes to the softoken and util manifest.mn.

4. We should just bite the bullet and rename the functions in their respective usage locations rather than do a nssrenam.h trick for them.

Question for Nelson... These are meant to be private functions. They are exported so that both pk11wrap and softoken could access them. How does how naming convention applies (The fact that they are exports would lean to SECMOD_ while the fact that they are private leans to secmod_. Previous examples were things like __CERT_DecodeCertificate().

bob

Comment 4

[Elio Maldonado]

Now I understand items 1 and 2 and will act on them in the next revision of the patch. On item 3, it does belong in utils. On item 4: I shouldn't rely on the nssrename.h trick and renaming wouldn't be a problem. Currently the functions are exported for use by p11wrap and softoken but the PEM module could make good use of them well. Guidance on naming is welcome.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

In reply to Bob's question in comment 3, 
Are we talking about functions exported from a shared lib? 
If so, our convention for private functions that are exported from a shared 
lib is to start the names with underscores and use a capitalized prefix.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

This change is complicated in that it involves changes to code inside and
outside the FIPS boundary.  I think we must ensure that the modifications
to the code outside the FIPS boundary do not make that code dependent on
the modified softoken, and that the unmodified softoken can still be built
with the modified libutil.

Comment 7

[Robert Relyea]

I agree with Nelson's comment 6.

It's probably too late for this change now. (what ever changed that's made should not require any changes in softoken).

bob

Comment 8

[Elio Maldonado]

Yes, this will have to wait until after the fips validation. softoken/legacydb/keydb.c is a client of this code. A textual renaming would have to be made top it. Invocations such as secmod_argGetParamValue(...) would become __SECMOD_argGetParamValue. On top of that the proposal involves moving a header from softoken to util. softoken that is from inside to outside the fips boundary and making the header have only declarations and implementing the function in a new file in utils. Currently the client code (both inside and outside the fips boundary) ends up carrying the implementation and softoken doesn't export any of this symbols but it would be a change nonetheless.

Comment 9

[Elio Maldonado]

Attachment: refactoring to move pk11 parse code: moves declarations and code to utils

This version of the patch implements Nelson's and Bob's recommendations. This is strict re-factoring so no functionality has been changed. No need to review it right away. I am attaching it as a way of saving the work done so far. It builds and the all tests pass. Hopefully we can revisit this post 3.12.4.

Comment 10

[Elio Maldonado]

Defer to 3.12.7.

Comment 11

[Elio Maldonado]

Bug 753116 takes care of this as part of changes of a broader scope.