246130 - pk11wrap NSS source files are too large and should be broken up

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

Some of the NSS source files have gotten really huge.  Here are line counts:
   4003    certhigh/ocsp.c
   4228    pk11wrap/pk11cert.c
   4540    fortcrypt/fortpk11.c
   4801    softoken/pkcs11.c
   4868    pk11wrap/pk11slot.c
   5333    softoken/pcertdb.c
   5419    softoken/pkcs11c.c
   5716    pk11wrap/pk11skey.c
   8995    ssl/ssl3con.c

Some of those files really are all about one thing, such as ssl3con.c.
But others are simply collections of code, much of which is not all related 
to a common theme.  For example: pk11skey.c was originally supposed to be 
for Symmetric key encryption functions.  But now it contains many functions 
that have nothing to do with symmetric key encryption, and more such new
functions continue to be added.

So, I'd like to see the files with 4000+ lines that are not all related
to a common theme be broken up into separate files, none of which is 
larger than 2-3 k lines, initially.

Comment 1

[Robert Relyea]

I mostly agree with this bug. I think this would be a good opportunity to get 
*ALL* the files to the 2-3k code size, recardless of the claim that the 
files "are about one thing". pkcs11.c and pkcs11c.c are really files with a 
common theme that were split because they had to be already (at one point they 
gave some compilers problems). Together they are the logical equivalent of 
ssl3con.c. Even pk11skey.c from day one was really "handling pkcs11 objects" of 
which the majority were about symkeys.

Anyway I agree with the statement sans the "that are not all related
to a common theme" phrase. That caveat is a hole we can drive a truck through. 
If the source files have gotten this big, it's time to pick new themes and 
refactor.

bob

Comment 2

[Robert Relyea]

Attachment: Refactoring code in pk11wrap - patch

This patch contains the diff -u for the pk11util directory. I left some of the
larger diffs in, though for review purposes I'll include the complete source
for all the major refactored files.

Comment 3

[Robert Relyea]

Attachment: pk11akey.c

pk11akey.c - asymetric keys constructed from pk11cert.c and pk11skey.c

Comment 4

[Robert Relyea]

Attachment: pk11auth.c

pk11auth.c - authentication/password management factored from pk11slot.c

Comment 5

[Robert Relyea]

Attachment: pk11cert.c

pk11cert.c - cert code with private key, crls and trust factored out.

Comment 6

[Robert Relyea]

Attachment: pk11cxt.c

pk11ctx.c -- pkcs11 context code, factored out of pk11skey.c

Comment 7

[Robert Relyea]

Attachment: pk11func.h

new pk11func.h -- for backward compatibility.

Comment 8

[Robert Relyea]

Attachment: pk11mech.c

pk11mech.c - mechanism mapping code, factored mostly from pk11slot.c

Comment 9

[Robert Relyea]

Attachment: pk11nobj.c

pk11nobj.c - netscape objects (crls and trust), factored mostly from pk11cert.c

Comment 10

[Robert Relyea]

Attachment: pk11obj.c

pk11obj.c - generic object support, factored from pk11skey.c pk11slot.c and
pk11cert.c

Comment 11

[Robert Relyea]

Attachment: pk11priv.h

pk11priv.h -- private functions factored from pk11func.h

NOTE: as a separate step, some of these functions should be reviewed to
determine whether they should actually be public functions. The current
criteria is whether or not the function appears in nss.def

Comment 12

[Robert Relyea]

Attachment: pk11pub.h

pk11pub.h -- public functions factored from pk11func.h

NOTE: as a separate step, this header file should be reorganized with better
descriptions for each of the functions in it. The current criteria is whether
or not the function appears here is if it is in nss.def

Comment 13

[Robert Relyea]

Attachment: pk11skey.c

pk11skey.c - now only symetric key ops; private, public key ops, generic ops
and crypto contexs have been factored out.

Comment 14

[Robert Relyea]

Attachment: pk11slot.c

pk11slot.c - still slot operations. Authentication, generic object ops,
mechanism mapping has been factored out.

Comment 15

[Robert Relyea]

Attachment: Contributer's research

This file contains the results of the research to get the contributer's portion
of the comment correct. This information was derived from CVS logs and
comments.

Comment 16

[Robert Relyea]

Here is the wc -l in the pk11wrap directory after the refactoring
   2212    debug_module.c
    316    dev3hack.c
     66    dev3hack.h
   2267    pk11akey.c  *new
    702    pk11auth.c  *new
   2762    pk11cert.c  *refactored
   1051    pk11cxt.c   *new
    150    pk11err.c
     46    pk11func.h  *refactored
     55    pk11init.h
    234    pk11kea.c
    171    pk11list.c
    337    pk11load.c
   1787    pk11mech.c  *new
    796    pk11nobj.c  *new
   1579    pk11obj.c   *new
    416    pk11pars.c
    845    pk11pbe.c
    558    pk11pk12.c
    383    pk11pqg.c
    155    pk11pqg.h
    217    pk11priv.h  * new
    583    pk11pub.h   * new
    322    pk11sdr.c
     59    pk11sdr.h
   1996    pk11skey.c  * refactored
   2282    pk11slot.c  * refactored
    766    pk11util.c
    153    secmod.h
    159    secmodi.h
    290    secmodt.h
    223    secmodti.h
     90    secpkcs5.h
  24028    total

Comment 17

[Robert Relyea]

Comment on attachment 153196 [details] [diff] [review]
Refactoring code in pk11wrap - patch

Please review.

Consider the attached '.c' and '.h' files as part of the review.

The patch includes files which have been totally refactored, which you can
optionally review by looking at the complete refactored file.

Comment 18

[Wan-Teh Chang]

This is an open-ended bug.  Bob broke up the large
files in lib/pk11wrap in NSS 3.10, so I'm going to
mark the bug fixed.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

changed bug summary to reflect recently narrowed scope.