753116 - softoken needs to split out common components to util

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Robert Relyea]

There are several things in softoken that were put there when we split util out which need to go to util or return to their original usage. Many of the header files moved to make softoken build, when really only one or declarations needed to be moved.

Also, there is some duplicated argument parsing code that really needs to live in util.

Finally, with the new pkcs11.txt file, there is no reason for softoken to parse the modules database any more (was in softoken to be able to access libdbm).

We should fix all of these issues.

Comment 1

[Robert Relyea]

Attachment: Split out argument parsing and reading pkcs11.txt.

Comment 2

[Elio Maldonado]

I am still reviewing but noticed something problematic, at least for us on RHEL and Fedora where were we have the factored softoken out as its' own package.

You moved sechash to a higher level area
From mozilla/security/nss/lib/freebl/sechash.h to mozilla/security/nss/lib/cryptohi/sechash.h

I have a source tree were I emulate what we do on nss-softoken (higher layers of nss removed) and when I tried to build I got:

gcc -o Linux3.3_x86_64_glibc_PTH_64_DBG.OBJ/pkcs11c.o -c -g -ansi -D_POSIX_SOURCE -D_BSD_SOURCE -D_XOPEN_SOURCE -fPIC -DLINUX2_1  -Wall -Werror-implicit-function-declaration -Wno-switch -pipe -DLINUX -Dlinux -DHAVE_STRERROR -DXP_UNIX -DSHLIB_SUFFIX=\"so\" -DSHLIB_PREFIX=\"lib\" -DSOFTOKEN_LIB_NAME=\"libsoftokn3.so\" -DSHLIB_VERSION=\"3\" -DDEBUG -UNDEBUG -DDEBUG_emaldona -D_REENTRANT -DUSE_UTIL_DIRECTLY -I../../../../dist/Linux3.3_x86_64_glibc_PTH_64_DBG.OBJ/include -I../../../../dist/public/nss -I../../../../dist/private/nss  pkcs11c.c
pkcs11c.c:27:21: fatal error: sechash.h: No such file or directory
compilation terminated.

If sechash were moved to util, like other files, there would be no problems. Could you do that instead?

Another thing is that in nssutil.def you have
;+NSSUTIL_3.13.3 {         # NSS Utilities 3.13.3 release 
... the new functions ....
;+};

Please change it to
;+NSSUTIL_3.14 {         # NSS Utilities 3.14 release

Must be the hazards of starting something, getting pulled to work something urgent and coming back to it a year or more later :-)

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Please ensure that the file "moves" are done in a way that preserves the history in CVS.

Comment 4

[Robert Relyea]

> I have a source tree were I emulate what we do on nss-softoken (higher layers of nss removed)
> and when I tried to build I got:

Good catch. I just commented out the header and pkcs11c.c compiled fine. Any other issues? with the softoken build..

> Another thing is that in nssutil.def you have

good catch. That mistake will likely be in all the .def files.

bob

Comment 5

[Robert Relyea]

> Please ensure that the file "moves" are done in a way that preserves the history in CVS.

Most of the 'moves' are moves back to where they were when we did the softoken/freebl build split.

Instead of just moving a few defines, full headers were moved (where there should be the original history still there).

The rest are real splits. (where the data was moved to multiple new files)

Comment 6

[Robert Relyea]

> Please ensure that the file "moves" are done in a way that preserves the history in CVS.

It looks like hasht.h (from freebl to util) does warrant special copying of the cvs history files. (I presume the copies remove the tags in the copied locations). The original location of hasht.h was lib/cryptohi.

bob

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Robert Relyea from comment #6)
> > Please ensure that the file "moves" are done in a way that preserves the history in CVS.
> 
> It looks like hasht.h (from freebl to util) does warrant special copying of
> the cvs history files. (I presume the copies remove the tags in the copied
> locations). The original location of hasht.h was lib/cryptohi.

It is OK to avoid doing the extra work if a file doesn't have any interesting history, especially if it hasn't changed since it was originally moved, if the original move already lost its history. But, especially as a newcomer to the project, I find CVS blame and CVS log to be invaluable for tracking down why things are done the way they are. Mostly I just want to make sure that this is taken into consideration.

Comment 8

[Elio Maldonado]

(In reply to Robert Relyea from comment #4)
> > I have a source tree were I emulate what we do on nss-softoken (higher layers of nss removed)
> > and when I tried to build I got:
> 
> Good catch. I just commented out the header and pkcs11c.c compiled fine. Any
> other issues? with the softoken build..

I found two more 
$ grep -r sechash mozilla/security/nss/lib/softoken/
mozilla/security/nss/lib/softoken/sftkpwd.c:#include "sechash.h"
mozilla/security/nss/lib/softoken/pkcs11c.c:#include "sechash.h"
mozilla/security/nss/lib/softoken/rsawrapr.c:#include "sechash.h"

sftkpwd.c and rsawrapr.c also include sechash and shouldn't.

Not exactly related with the softoken but indirectly via blapitest is https://bugzilla.mozilla.org/show_bug.cgi?id=746842#c5
Any ideas as to were to move typedef SECItem SECKEYECParams;?
It seems to fit nicely with the goals of this bug.

> good catch. That mistake will likely be in all the .def files.
I turns out that nssutil.def is the only one affected.

Comment 9

[Robert Relyea]

> I found two more 

yeah, I just did the same grep. The other two also build find without the header.

> Not exactly related with the softoken but indirectly via blapitest is https://bugzilla.mozilla.org/show_bug.cgi?id=746842#c5
> Any ideas as to were to move typedef SECItem SECKEYECParams;?
> It seems to fit nicely with the goals of this bug.

Why not change the 2 occurances of SECKEYECParams to SECItem in blapitest.c. It's clearly not used by any inteface blapi calls. In both cases the variable is implicitly casted to and from SECItem anyway.

bob

Comment 10

[Robert Relyea]

> I find CVS blame and CVS log to be invaluable for tracking down why things are done the way the
> are. Mostly I just want to make sure that this is taken into consideration.

I'll make sure hasht.h get's it's CVS history when it moves then.

bob

Comment 11

[Elio Maldonado]

(In reply to Robert Relyea from comment #9)
> Why not change the 2 occurances of SECKEYECParams to SECItem in blapitest.c.
> It's clearly not used by any inteface blapi calls. In both cases the
> variable is implicitly casted to and from SECItem anyway.
> 
I did just that for the same reasons you give but thought of it as a temporary solution.

Comment 12

[Elio Maldonado]

Comment on attachment 622149 [details] [diff] [review]
Split out argument parsing and reading pkcs11.txt.

r- for the reasons we already discussed in the comments.

Comment 13

[Robert Relyea]

Attachment: Split out argument parsing and reading pkcs11.txt. V2

I was hoping for more comments before I submitted another patch, but here's the update.

Comment 15

[Robert Relyea]

Checking in cryptohi/manifest.mn;
/cvsroot/mozilla/security/nss/lib/cryptohi/manifest.mn,v  <--  manifest.mn
new revision: 1.10; previous revision: 1.9
done
Checking in cryptohi/sechash.h;
/cvsroot/mozilla/security/nss/lib/cryptohi/sechash.h,v  <--  sechash.h
new revision: 1.10; previous revision: 1.9
done
Removing freebl/hasht.h;
/cvsroot/mozilla/security/nss/lib/freebl/hasht.h,v  <--  hasht.h
new revision: delete; previous revision: 1.9
done
Checking in freebl/manifest.mn;
/cvsroot/mozilla/security/nss/lib/freebl/manifest.mn,v  <--  manifest.mn
new revision: 1.64; previous revision: 1.63
done
Checking in freebl/nsslowhash.c;
/cvsroot/mozilla/security/nss/lib/freebl/nsslowhash.c,v  <--  nsslowhash.c
new revision: 1.9; previous revision: 1.8
done
Checking in freebl/rawhash.c;
/cvsroot/mozilla/security/nss/lib/freebl/rawhash.c,v  <--  rawhash.c
new revision: 1.10; previous revision: 1.9
done
Removing freebl/sechash.h;
/cvsroot/mozilla/security/nss/lib/freebl/sechash.h,v  <--  sechash.h
new revision: delete; previous revision: 1.10
done
Checking in freebl/tlsprfalg.c;
/cvsroot/mozilla/security/nss/lib/freebl/tlsprfalg.c,v  <--  tlsprfalg.c
new revision: 1.9; previous revision: 1.8
done
Checking in nss/nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.117; previous revision: 1.116
done
Checking in pk11wrap/manifest.mn;
/cvsroot/mozilla/security/nss/lib/pk11wrap/manifest.mn,v  <--  manifest.mn
new revision: 1.24; previous revision: 1.23
done
Checking in pk11wrap/pk11load.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11load.c,v  <--  pk11load.c
new revision: 1.36; previous revision: 1.35
done
Checking in pk11wrap/pk11pars.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pars.c,v  <--  pk11pars.c
new revision: 1.29; previous revision: 1.28
done
Checking in pk11wrap/pk11slot.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.111; previous revision: 1.110
done
Checking in pk11wrap/pk11util.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11util.c,v  <--  pk11util.c
new revision: 1.63; previous revision: 1.62
done
Checking in pk11wrap/secmodi.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/secmodi.h,v  <--  secmodi.h
new revision: 1.36; previous revision: 1.35
done
Checking in pk11wrap/secmodt.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/secmodt.h,v  <--  secmodt.h
new revision: 1.41; previous revision: 1.40
done
Checking in pk11wrap/secmodti.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/secmodti.h,v  <--  secmodti.h
new revision: 1.27; previous revision: 1.26
done
Checking in softoken/manifest.mn;
/cvsroot/mozilla/security/nss/lib/softoken/manifest.mn,v  <--  manifest.mn
new revision: 1.41; previous revision: 1.40
done
Removing softoken/pk11init.h;
/cvsroot/mozilla/security/nss/lib/softoken/pk11init.h,v  <--  pk11init.h
new revision: delete; previous revision: 1.7
done
Removing softoken/pk11pars.h;
/cvsroot/mozilla/security/nss/lib/softoken/pk11pars.h,v  <--  pk11pars.h
new revision: delete; previous revision: 1.26
done
Checking in softoken/pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.178; previous revision: 1.177
done
Checking in softoken/pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.126; previous revision: 1.125
done
Checking in softoken/pkcs11i.h;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11i.h,v  <--  pkcs11i.h
new revision: 1.60; previous revision: 1.59
done
Checking in softoken/rsawrapr.c;
/cvsroot/mozilla/security/nss/lib/softoken/rsawrapr.c,v  <--  rsawrapr.c
new revision: 1.21; previous revision: 1.20
done
Checking in softoken/sdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/sdb.c,v  <--  sdb.c
new revision: 1.22; previous revision: 1.21
done
Checking in softoken/sdb.h;
/cvsroot/mozilla/security/nss/lib/softoken/sdb.h,v  <--  sdb.h
new revision: 1.7; previous revision: 1.6
done
Removing softoken/secmodt.h;
/cvsroot/mozilla/security/nss/lib/softoken/secmodt.h,v  <--  secmodt.h
new revision: delete; previous revision: 1.42
done
Checking in softoken/sftkdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/sftkdb.c,v  <--  sftkdb.c
new revision: 1.31; previous revision: 1.30
done
Checking in softoken/sftkdb.h;
/cvsroot/mozilla/security/nss/lib/softoken/sftkdb.h,v  <--  sftkdb.h
new revision: 1.12; previous revision: 1.11
done
Checking in softoken/sftkdbt.h;
/cvsroot/mozilla/security/nss/lib/softoken/sftkdbt.h,v  <--  sftkdbt.h
new revision: 1.6; previous revision: 1.5
done
Removing softoken/sftkmod.c;
/cvsroot/mozilla/security/nss/lib/softoken/sftkmod.c,v  <--  sftkmod.c
new revision: delete; previous revision: 1.12
done
Checking in softoken/sftkpars.c;
/cvsroot/mozilla/security/nss/lib/softoken/sftkpars.c,v  <--  sftkpars.c
new revision: 1.14; previous revision: 1.13
done
Checking in softoken/sftkpwd.c;
/cvsroot/mozilla/security/nss/lib/softoken/sftkpwd.c,v  <--  sftkpwd.c
new revision: 1.13; previous revision: 1.12
done
Checking in softoken/legacydb/lginit.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/lginit.c,v  <--  lginit.c
new revision: 1.18; previous revision: 1.17
done
Checking in softoken/legacydb/pk11db.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/pk11db.c,v  <--  pk11db.c
new revision: 1.5; previous revision: 1.4
done
Checking in sysinit/nsssysinit.c;
/cvsroot/mozilla/security/nss/lib/sysinit/nsssysinit.c,v  <--  nsssysinit.c
new revision: 1.6; previous revision: 1.5
done
Checking in util/hasht.h;
/cvsroot/mozilla/security/nss/lib/util/hasht.h,v  <--  hasht.h
new revision: 1.10; previous revision: 1.9
done
Checking in util/manifest.mn;
/cvsroot/mozilla/security/nss/lib/util/manifest.mn,v  <--  manifest.mn
new revision: 1.26; previous revision: 1.25
done
Checking in util/nssutil.def;
/cvsroot/mozilla/security/nss/lib/util/nssutil.def,v  <--  nssutil.def
new revision: 1.18; previous revision: 1.17
done
Checking in util/secerr.h;
/cvsroot/mozilla/security/nss/lib/util/secerr.h,v  <--  secerr.h
new revision: 1.32; previous revision: 1.31
done
RCS file: /cvsroot/mozilla/security/nss/lib/util/utilmod.c,v
done
Checking in util/utilmod.c;
/cvsroot/mozilla/security/nss/lib/util/utilmod.c,v  <--  utilmod.c
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/lib/util/utilmodt.h,v
done
Checking in util/utilmodt.h;
/cvsroot/mozilla/security/nss/lib/util/utilmodt.h,v  <--  utilmodt.h
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/lib/util/utilpars.c,v
done
Checking in util/utilpars.c;
/cvsroot/mozilla/security/nss/lib/util/utilpars.c,v  <--  utilpars.c
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/lib/util/utilpars.h,v
done
Checking in util/utilpars.h;
/cvsroot/mozilla/security/nss/lib/util/utilpars.h,v  <--  utilpars.h
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/lib/util/utilparst.h,v
done
Checking in util/utilparst.h;
/cvsroot/mozilla/security/nss/lib/util/utilparst.h,v  <--  utilparst.h
initial revision: 1.1
done

Comment 16

[Robert Relyea]

Tinderbox is showing new memory leaks, reopen bug.

Comment 17

[Robert Relyea]

Attachment: Fix leaks introduced by main patch.

Comment 18

[Robert Relyea]

Checking in util/utilmod.c;
/cvsroot/mozilla/security/nss/lib/util/utilmod.c,v  <--  utilmod.c
new revision: 1.3; previous revision: 1.2
done

Comment 19

[Wan-Teh Chang]

Comment on attachment 623001 [details] [diff] [review]
Split out argument parsing and reading pkcs11.txt. V2

Review of attachment 623001 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/util/utilpars.c
@@ +358,5 @@
> +
> +int
> +NSSUTIL_EscapeSize(const char *string, char quote)
> +{
> +     return nssutil_escapeQuotesSize(string, quote, PR_TRUE);

Bob, I believe NSSUTIL_EscapeSize should pass addquotes=PR_FALSE
to nssutil_escapeQuotesSize. See the NSSUTIL_Escape function
below, which passes addquotes=PR_FALSE to nssutil_escapeQuotes.

Comment 20

[Wan-Teh Chang]

Attachment: Fix error code collision of SEC_ERROR_LEGACY_DATABASE

This is most likely a patch merging error.

Patch checked in on the NSS trunk (NSS 3.14).

Checking in secerr.h;
/cvsroot/mozilla/security/nss/lib/util/secerr.h,v  <--  secerr.h
new revision: 1.33; previous revision: 1.32
done

Comment 21

[Wan-Teh Chang]

Attachment: NSSUTIL_EscapeSize should pass addquotes=PR_FALSE

This fixes the problem I asked about in comment 19. I also
fixed a typo in a comment.

If this fix is correct, please feel free to check this in.
Thanks.

Comment 22

[Wan-Teh Chang]

Attachment: Add include guard macro to utilpars.h

Also fix typos and style nits.

Comment 23

[Wan-Teh Chang]

Attachment: Fix softoken nits

Remove an unused variable and a commented-out line (the sdb_type
member of the SDB structure is gone now).

Comment 24

[Robert Relyea]

Comment on attachment 663696 [details] [diff] [review]
Fix error code collision of SEC_ERROR_LEGACY_DATABASE

r+ rrelyea

Comment 25

[Robert Relyea]

Comment on attachment 663697 [details] [diff] [review]
NSSUTIL_EscapeSize should pass addquotes=PR_FALSE

r+ rrelyea

Comment 26

[Robert Relyea]

Comment on attachment 663698 [details] [diff] [review]
Add include guard macro to utilpars.h

r+ rrelyea

Comment 27

[Robert Relyea]

Comment on attachment 663702 [details] [diff] [review]
Fix softoken nits

r+ rrelyea

Comment 28

[Wan-Teh Chang]

Comment on attachment 663697 [details] [diff] [review]
NSSUTIL_EscapeSize should pass addquotes=PR_FALSE

Patch checked in on the NSS trunk (NSS 3.14).

Checking in utilpars.c;
/cvsroot/mozilla/security/nss/lib/util/utilpars.c,v  <--  utilpars.c
new revision: 1.3; previous revision: 1.2
done

Comment 29

[Wan-Teh Chang]

Comment on attachment 663698 [details] [diff] [review]
Add include guard macro to utilpars.h

Patch checked in on the NSS trunk (NSS 3.14).

Checking in utilmod.c;
/cvsroot/mozilla/security/nss/lib/util/utilmod.c,v  <--  utilmod.c
new revision: 1.4; previous revision: 1.3
done
Checking in utilmodt.h;
/cvsroot/mozilla/security/nss/lib/util/utilmodt.h,v  <--  utilmodt.h
new revision: 1.2; previous revision: 1.1
done
Checking in utilpars.h;
/cvsroot/mozilla/security/nss/lib/util/utilpars.h,v  <--  utilpars.h
new revision: 1.2; previous revision: 1.1
done

Comment 30

[Wan-Teh Chang]

Comment on attachment 663702 [details] [diff] [review]
Fix softoken nits

Patch checked in on the NSS trunk (NSS 3.14).

Checking in pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.182; previous revision: 1.181
done
Checking in sdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/sdb.c,v  <--  sdb.c
new revision: 1.23; previous revision: 1.22
done

Comment 31

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Add error message for SEC_ERROR_LEGACY_DATABASE to SECerrs.h

I am having some trouble with my CVS access so I cannot check in this fix.

Comment 32

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Add error message for SEC_ERROR_LEGACY_DATABASE to SECerrs.h [v2]

Comment 33

[Robert Relyea]

Comment on attachment 666101 [details] [diff] [review]
Add error message for SEC_ERROR_LEGACY_DATABASE to SECerrs.h  [v2]

r+ rrelyea

Comment 34

[Robert Relyea]

bobslaptop.local(110) cvs commit
cvs commit: Examining .
Checking in SECerrs.h;
/cvsroot/mozilla/security/nss/lib/util/SECerrs.h,v  <--  SECerrs.h
new revision: 1.26; previous revision: 1.25
done

Comment 35

[Elio Maldonado]

Attachment: Repacement includes as we can't include lgglue.h

Something that I missed in my review but eventually I caught as it broke my build of nss-util in fedopra Rawhide. We can't include lgglue.h because it belongs to softoken/legacydb. Replacling with pkcs11t.h and sectitem.h to get the needed types.

Comment 36

[Robert Relyea]

Comment on attachment 667517 [details] [diff] [review]
Repacement includes as we can't include lgglue.h

r+ rrelyea

Comment 37

[Elio Maldonado]

Checked in to the trunk for NSS 3.14:
Checking in ./mozilla/security/nss/lib/util/utilmod.c;
/cvsroot/mozilla/security/nss/lib/util/utilmod.c,v  <--  utilmod.c
new revision: 1.5; previous revision: 1.4
done