286439 - Remove PKCS11_USE_THREADS and PK11_USE_THREADS

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Wan-Teh Chang]

The softoken source code uses the PKCS11_USE_THREADS
and PK11_USE_THREADS macros so it can be built with
or without threads.

We haven't built and tested the softoken without
threads for a long time.  So we should remove these
two macros to make the source code easier to work
with.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch

Hmm... who should I inflict the pain of reviewing this
patch on?

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 177657 [details] [diff] [review]
Proposed patch

r=nelson

This patch appears to be correct.  I didn't verify that every use
of PK*11_USE_THREADS was removed, but if it compiles, that must be
so.  So this patch can be checked in as-is, but I have one 
suggestion for additional improvement.	

There are several locks in this code that are cast to (PZLock *) 
or (NSSRWLock *) in each and every use.  I will show them below.  
I think that it would be better to change the types of those lock 
variables from void * (or whatever they are now) to PZLock * or
NSSRWLock *, so that all those casts can be removed.  That could 
be done as a separate checkin, but This patch touches every one 
of those uses, so doing the cast removal in a separate checking 
just touches all those lines twice.

Here are some examples:

1. SECMODListLock * really as NSSRWLock *.

> void SECMOD_DestroyListLock(SECMODListLock *lock) 
> {
>-    PK11_USE_THREADS(NSSRWLock_Destroy((NSSRWLock *)lock);)
>+    NSSRWLock_Destroy((NSSRWLock *)lock);

2. SECMODMOdule.refLock

>     newMod->refLock = (void *)PZ_NewLock(nssILockRefLock);

Why is this a void *?  It's always used as a PZLock *, I think.

> SECMODModule *
> SECMOD_ReferenceModule(SECMODModule *module) 
> {
>-    PK11_USE_THREADS(PZ_Lock((PZLock *)module->refLock);)
>+    PZ_Lock((PZLock *)module->refLock);

>-    PK11_USE_THREADS(PZ_Unlock((PZLock*)module->refLock);)
>+    PZ_Unlock((PZLock*)module->refLock);


3. PK11SlotList.lock

> SECStatus
> PK11_DeleteSlotFromList(PK11SlotList *list,PK11SlotListElement *le)
> {
>-    PK11_USE_THREADS(PZ_Lock((PZLock *)(list->lock));)
>+    PZ_Lock((PZLock *)(list->lock));

>-    PK11_USE_THREADS(PZ_Unlock((PZLock *)(list->lock));)
>+    PZ_Unlock((PZLock *)(list->lock));


> pk11_initSlotList(PK11SlotList *list)
> {
>-#ifdef PKCS11_USE_THREADS
>     list->lock = PZ_NewLock(nssILockList);

Not cast when created, but cast for every use.

>     if (list->lock) {
>     	PZ_DestroyLock((PZLock *)(list->lock));

Comment 3

[Robert Relyea]

Comment on attachment 177657 [details] [diff] [review]
Proposed patch

On the guy who was half a world away at the time you did the inflicting, and
whose still 61 buzilla message way from clearing that mail (down from over
500).;)

Comment 4

[Robert Relyea]

re comment 2 from nelson..

I think they were void * to allow them to be declared in the case
PKCS11_USE_THREADS was not defined, so declaring the locks appropriately and
removing the casts could reasonably be made a condition of removing
PKCS11_USE_THREADS.

bob

Comment 5

[Wan-Teh Chang]

Attachment: Proposed patch v2

I define those lock members to have the type PZLock *
and get rid of the (PZLock *) casts in this patch.

I didn't do much about SECMODListLock and the
(NSSRWLock *) casts.  I merely define the SECMODListLockStr
structure as a dummy structure to show it is not being
used.

An alternative is to get rid of the SECMODListLockStr
structure and directly define SECMODListLock as NSSRWLock
in secmodt.h:

-typedef struct SECMODListLockStr SECMODListLock;
+typedef NSSRWLock SECMODListLock;

Let me know which solution you prefer.

Comment 6

[Wan-Teh Chang]

Attachment: Alternate proposed patch v2

The only difference in this patch is the solution to
the SECMODListLock problem.  In this patch, I simply
define SECMODListLock to be NSSRWLock and get rid of
the SECMODListLockStr structure and the casts.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 178502 [details] [diff] [review]
Proposed patch v2

r=nelson

I am persuaded that it is best to keep the SECMODListLockStr,
rather than eliminating it, even though it does mean more casting.

Comment 8

[Robert Relyea]

Comment on attachment 178514 [details] [diff] [review]
Alternate proposed patch v2

r+ for this patch.

Comment 9

[Wan-Teh Chang]

I checked in "alternate proposed patch v2" on the
NSS trunk (NSS 3.10).  I understand that Nelson and
Bob chose different solutions to the SECMODListLock
problem, and I decided to go with choice of the
original author of pk11wrap and softoken (Bob).

Checking in pk11wrap/pk11list.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11list.c,v  <--  pk11list.c
new revision: 1.7; previous revision: 1.6
done
Checking in pk11wrap/pk11pars.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pars.c,v  <--  pk11pars.c
new revision: 1.18; previous revision: 1.17
done
Checking in pk11wrap/pk11skey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11skey.c,v  <--  pk11skey.c
new revision: 1.96; previous revision: 1.95
done
Checking in pk11wrap/pk11slot.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.83; previous revision: 1.82
done
Checking in pk11wrap/pk11util.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11util.c,v  <--  pk11util.c
new revision: 1.48; previous revision: 1.47
done
Checking in pk11wrap/secmod.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/secmod.h,v  <--  secmod.h
new revision: 1.21; previous revision: 1.20
done
Checking in pk11wrap/secmodi.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/secmodi.h,v  <--  secmodi.h
new revision: 1.20; previous revision: 1.19
done
Checking in pk11wrap/secmodt.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/secmodt.h,v  <--  secmodt.h
new revision: 1.25; previous revision: 1.24
done
Checking in pk11wrap/secmodti.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/secmodti.h,v  <--  secmodti.h
new revision: 1.23; previous revision: 1.22
done
Checking in softoken/pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.98; previous revision: 1.97
done
Checking in softoken/pkcs11i.h;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11i.h,v  <--  pkcs11i.h
new revision: 1.35; previous revision: 1.34
done
Checking in softoken/pkcs11u.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11u.c,v  <--  pkcs11u.c
new revision: 1.64; previous revision: 1.63
done

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Thanks Wan-Teh for doing this large cleanup.  I don't mind alternative 2.