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

RESOLVED FIXED in 3.10

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
14 years ago
11 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Wan-Teh Chang)

Tracking

3.10
x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(14 attachments)

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.
(Reporter)

Updated

14 years ago
Priority: -- → P2
Target Milestone: --- → 3.10

Comment 1

14 years ago
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

14 years ago
Created attachment 153196 [details] [diff] [review]
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

14 years ago
Created attachment 153199 [details]
pk11akey.c

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

Comment 4

14 years ago
Created attachment 153201 [details]
pk11auth.c

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

Comment 5

14 years ago
Created attachment 153202 [details]
pk11cert.c

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

Comment 6

14 years ago
Created attachment 153203 [details]
pk11cxt.c

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

Comment 7

14 years ago
Created attachment 153204 [details]
pk11func.h

new pk11func.h -- for backward compatibility.

Comment 8

14 years ago
Created attachment 153205 [details]
pk11mech.c

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

Comment 9

14 years ago
Created attachment 153206 [details]
pk11nobj.c

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

Comment 10

14 years ago
Created attachment 153207 [details]
pk11obj.c

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

Comment 11

14 years ago
Created attachment 153208 [details]
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

14 years ago
Created attachment 153209 [details]
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

14 years ago
Created attachment 153210 [details]
pk11skey.c

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

Comment 14

14 years ago
Created attachment 153211 [details]
pk11slot.c

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

Comment 15

14 years ago
Created attachment 153212 [details]
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

14 years ago
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

14 years ago
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.
Attachment #153196 - Flags: review?(nelson)

Updated

14 years ago
Attachment #153196 - Flags: review?(nelson)
(Reporter)

Updated

13 years ago
QA Contact: bishakhabanerjee → jason.m.reid
(Assignee)

Comment 18

13 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Reporter)

Comment 19

13 years ago
changed bug summary to reflect recently narrowed scope.
Summary: Some NSS source files are too large and should be broken up → pk11wrap NSS source files are too large and should be broken up

Updated

11 years ago
Duplicate of this bug: 172682

Updated

11 years ago
Duplicate of this bug: 172681
You need to log in before you can comment on or make changes to this bug.