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

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

The same applies to modutil's -rawlist command.  

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Downgrading to P2, since -rawlist is a workaround.
Retargetting to NSS 3.9, since it wasn't fixed in 3.8.1

Comment 4

[Wan-Teh Chang]

Assigned the bug to Bob.

Comment 5

[Robert Relyea]

Attachment: 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.

Comment 6

[Robert Relyea]

*** Bug 203868 has been marked as a duplicate of this bug. ***

Comment 7

[Robert Relyea]

Attachment: modutil portion of the patch..

Ooops I forgot the modutil portion of the patch.

Comment 8

[Wan-Teh Chang]

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.

Comment 9

[Wan-Teh Chang]

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.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

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 * ?

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 12

[Robert Relyea]

Attachment: 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.

Comment 13

[Robert Relyea]

Attachment: same patch as above for pk11.c except diff -b was used .

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 15

[Wan-Teh Chang]

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.

Comment 16

[Robert Relyea]

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