modutil -list doesn't list all modules in secmod.db

RESOLVED FIXED in 3.9

Status

NSS
Tools
P2
normal
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Robert Relyea)

Tracking

x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

56.88 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Wan-Teh Chang
: superreview+
Details | Diff | Splinter Review
5.45 KB, patch
Details | Diff | Splinter Review
The modutil -list command lists only the modules in secmod.db that were 
succesfully loaded.  

It should list ALL modules in the DB, whether they are loaded or not. 

The present behavior is VERY misleading.
(Reporter)

Updated

14 years ago
Priority: -- → P1
Target Milestone: --- → 3.8.1
(Reporter)

Comment 1

14 years ago
The same applies to modutil's -rawlist command.  
(Reporter)

Updated

14 years ago
Blocks: 200215
(Reporter)

Comment 2

14 years ago
I retract comment 1 above.  It is false.  
This problem does not apply to -rawlist.
However, there is another problem with -rawlist, 
about which I will file a separate bug report.
(Reporter)

Updated

14 years ago
No longer blocks: 200215
(Reporter)

Comment 3

14 years ago
Downgrading to P2, since -rawlist is a workaround.
Retargetting to NSS 3.9, since it wasn't fixed in 3.8.1
Priority: P1 → P2
Target Milestone: 3.8.1 → 3.9

Comment 4

14 years ago
Assigned the bug to Bob.
Assignee: wchang0222 → rrelyea0264
(Assignee)

Comment 5

14 years ago
Created attachment 132933 [details] [diff] [review]
Handle Unloaded modules better.

This patch makes unloaded modules work like loaded modules for administrative
purposes. The unloaded modules can now be found, looked up by name, and
deleted.
(Assignee)

Comment 6

14 years ago
*** Bug 203868 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

14 years ago
Attachment #132933 - Flags: superreview?(wchang0222)
Attachment #132933 - Flags: review?(MisterSSL)
(Assignee)

Comment 7

14 years ago
Created attachment 132940 [details] [diff] [review]
modutil portion of the patch..

Ooops I forgot the modutil portion of the patch.
(Assignee)

Updated

14 years ago
Attachment #132940 - Flags: superreview?(MisterSSL)
Attachment #132940 - Flags: review?(wchang0222)

Comment 8

14 years ago
Comment on attachment 132933 [details] [diff] [review]
Handle Unloaded modules better.

r=wtc.

Some suggested changes.

1. Please add comments in secmod.h explaining what the new functions
do.  In particular please document that like SECMOD_GetDefaultModuleList,
they need to be called with the "default module list lock" held.

2. In pk11pars.c, function SECMOD_LoadModule, you can do the following
right after the SECMOD_CreateModule call:

>+	if (parent) {
>+    	    module->parent = SECMOD_ReferenceModule(parent);
>+	}

Then you'd only need to do this once in that function.	By the way,
should we add code to handle the failure of the SECMOD_CreateModule
call?

3. In pk11util.c, you added two pieces of code like this:

    if (!module)
     for(mlp = modulesUnload; mlp != NULL; mlp = mlp->next) {
	...
    }

This indentation style is unusual.
Attachment #132933 - Flags: superreview?(wchang0222) → superreview+

Comment 9

14 years ago
Comment on attachment 132940 [details] [diff] [review]
modutil portion of the patch..

This patch looks good.	It has only one error:

> 	list = SECMOD_GetDefaultModuleList();
>+	deadlist = SECMOD_GetDeadModuleList();
> 	if(!list) {

The test should say "if (!list && !deadlist)".

Please declare printModule as a static function.
Attachment #132940 - Flags: review?(wchang0222) → review-
(Reporter)

Comment 10

14 years ago
Comment on attachment 132933 [details] [diff] [review]
Handle Unloaded modules better.

r=MisterSSL with two suggested changes to this patch:

>--- nss/nss.def	7 Oct 2003 01:29:32 -0000	1.113
>+++ nss/nss.def	9 Oct 2003 16:35:44 -0000
>@@ -765,6 +765,8 @@
> PK11_ExportEncryptedPrivKeyInfo;
> PK11_FindSlotsByAliases;
> SEC_DupCrl;
>+SECMOD_GetDeadModuleList;
>+SECMOD_GetDBModuleList;
> ;+    local:

In the ASCII collating sequence, "_" comes after all capital letters.
So, to keep the entries in proper sorted order, the two new SECMOD_
functions should come before SEC_DupCrl.

>--- pk11wrap/pk11util.c	26 Mar 2003 00:31:10 -0000	1.40
>+++ pk11wrap/pk11util.c	9 Oct 2003 16:35:44 -0000

>@@ -256,16 +266,16 @@
>  * optionally remove it from secmod.db.
>  */
> SECStatus
>-SECMOD_DeleteModuleEx(char *name, SECMODModule *mod, int *type, PRBool permdb) {
>+SECMOD_DeleteModuleEx(char *name, SECMODModule *mod, int *type, PRBool permdb) 
>+{

While you're changing this, how about making name be const char * ?
Attachment #132933 - Flags: review?(MisterSSL) → review+
(Reporter)

Comment 11

14 years ago
Comment on attachment 132940 [details] [diff] [review]
modutil portion of the patch..

Issues with this patch:

1. This source file seems to use an indentation quantum of 8, using tabs
   for most indents.  That's not the normal NSS style, which uses an 
   indentation quantum of 4, and assumes tabstops are 8, but this file is 
   self consistent.  Because it consistently uses tabs for indents, if 
   one changes one's editor to set tabstops to 4, then this code appears
   to match the NSS style (indentation quantum of 4).

   However, it appears to me that the new function printModule uses the 
   NSS style (indentation quantum == 4) and does not follow the indentation 
   style of the rest of the file.  The code was apparently changed to NSS
   style when it was moved into this separate funciton.  I think the style 
   of this file should remain consistent.  

   Several parts of this patch have this problem.  Please keep the file
   consistent.

> 
>-	if(PK11_GetModInfo(module, &modinfo) != SECSuccess) {
>+	if (module->loaded) {
>+	   if (PK11_GetModInfo(module, &modinfo) != SECSuccess) {
> 		PR_fprintf(PR_STDERR, errStrings[MOD_INFO_ERR], moduleName);
> 		return MOD_INFO_ERR;
>+	   }
> 	}

2. Two issues with that change:

a. It uses NSS style indentation.
b. I'd prefer to see a single compound if, e.g. 
>+	if (module->loaded && PK11_GetModInfo(module, &modinfo) != SECSuccess) {
and avoid two levels of indentation.
Attachment #132940 - Flags: superreview?(MisterSSL) → superreview-
(Assignee)

Comment 12

14 years ago
Created attachment 132966 [details] [diff] [review]
Incorporate Review comments.

This patch includes pretty much all of wan-teh and Nelson's comments.

I went ahead and did a quick conversion ls most of the char * pointers
arguments in pk11util.c to const char *. A more complete conversion should
happen on a separate bug.

I also fixed Nelsons's consistancy point by converted pk11.c in modutil to use
NSS style indentation.
(Assignee)

Updated

14 years ago
Attachment #132933 - Attachment is obsolete: true
Attachment #132940 - Attachment is obsolete: true
(Assignee)

Comment 13

14 years ago
Created attachment 132967 [details] [diff] [review]
same patch as above for pk11.c except diff -b was used .
(Assignee)

Updated

14 years ago
Attachment #132966 - Flags: superreview?(wchang0222)
Attachment #132966 - Flags: review?(MisterSSL)
(Reporter)

Comment 14

14 years ago
Comment on attachment 132966 [details] [diff] [review]
Incorporate Review comments.

That's a much bigger change than I was expecting, but it looks pretty good to
me.
Attachment #132966 - Flags: review?(MisterSSL) → review+

Comment 15

14 years ago
Comment on attachment 132966 [details] [diff] [review]
Incorporate Review comments.

r=wtc.

It's not necessary to fix the nonstandard indentation
style.	All we want is consistency within the same file.
But, since you already did the work, :-)

Please remember to make the suggested changes for pk11wrap
in comment 8 and comment 10.
Attachment #132966 - Flags: superreview?(wchang0222) → superreview+
(Assignee)

Comment 16

14 years ago
Checking in pk11pars.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pars.c,v  <--  pk11pars.c
new revision: 1.13; previous revision: 1.12
done
Checking in pk11util.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11util.c,v  <--  pk11util.c
new revision: 1.41; previous revision: 1.40
done
Checking in secmod.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/secmod.h,v  <--  secmod.h
new revision: 1.16; previous revision: 1.15
done
Checking in secmodi.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/secmodi.h,v  <--  secmodi.h
new revision: 1.14; previous revision: 1.13
done
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.114; previous revision: 1.113
done
Checking in pk11.c;
/cvsroot/mozilla/security/nss/cmd/modutil/pk11.c,v  <--  pk11.c
new revision: 1.20; previous revision: 1.19
done
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.