Last Comment Bug 447563 - modutil -add prints no error explanation on failure
: modutil -add prints no error explanation on failure
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Tools (show other bugs)
: 3.2
: All All
: P2 normal (vote)
: 3.12.2
Assigned To: Elio Maldonado
:
:
Mentors:
http://bonsai.mozilla.org/cvsblame.cg...
: 454716 (view as bug list)
Depends on: 454716
Blocks:
  Show dependency treegraph
 
Reported: 2008-07-22 20:41 PDT by Nelson Bolyard (seldom reads bugmail)
Modified: 2008-09-11 10:47 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Use NSS's functions for error reporting (1.03 KB, patch)
2008-07-30 10:41 PDT, Elio Maldonado
nelson: review+
Details | Diff | Splinter Review
Use NSS error functions yet preserve detailed error. (1.62 KB, patch)
2008-07-30 20:09 PDT, Elio Maldonado
no flags Details | Diff | Splinter Review
resending renamed as previous doesn't show good diff (1.62 KB, patch)
2008-07-30 20:18 PDT, Elio Maldonado
nelson: review+
Details | Diff | Splinter Review
Preserve the values of error codes (860 bytes, patch)
2008-09-10 21:16 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review
Preserve the values of error codes, v2 (1.84 KB, patch)
2008-09-10 21:32 PDT, Wan-Teh Chang
emaldona: review+
Details | Diff | Splinter Review
Patch as checked in (1.94 KB, patch)
2008-09-11 10:34 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review

Description Nelson Bolyard (seldom reads bugmail) 2008-07-22 20:41:37 PDT
When the modutil -add command fails, modutil tells the user that it failed,
but does not print the text of any NSS or NSPR error code that might explain
the failure.  This is because the code in that one error path in modutil uses
a technique to get the error text that simply does not work and never has.  See
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/cmd/modutil/pk11.c&rev=1.26&mark=295-297#286

The short term solution is to use one of NSS's working functions that prints 
NSS and NSPR error codes here, rather than this non-working technique.
Modutil should print error codes the same way that all other NSS programs do.
Comment 1 Nelson Bolyard (seldom reads bugmail) 2008-07-22 20:48:58 PDT
Bug present since rev 1.10 in NSS 3.2
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/cmd/modutil/pk11.c&rev=1.10&mark=244-247#236
Comment 2 Elio Maldonado 2008-07-28 08:37:20 PDT
The problem seems to be shared among modutil and p11wrap.  modutil's pk11.c relies on PR_GetErrorText(errtxt) after calling SECMOD_AddNewModuleEx which which in turns uses PR_SetErrorText(0, NULL);  Searching on _SetError nowhere else inside nss/lib I see that SetErrorText is used only by SECMOD_AddNewModuleEx. The rest of nss relies on either nss_SetError or on PORT_SetError. I am not sure I understand when one is supposed to nss_SetError and when PORT_SetError.
Comment 3 Elio Maldonado 2008-07-30 10:41:21 PDT
Created attachment 331746 [details] [diff] [review]
Use NSS's functions for error reporting

Proposed simple fix attached.
Comment 4 Nelson Bolyard (seldom reads bugmail) 2008-07-30 14:31:57 PDT
Comment on attachment 331746 [details] [diff] [review]
Use NSS's functions for error reporting

Elio, In email, you expressed a desire to retain the use of the 
PR_GetErrorText code path, because it may give slightly better errors in 
certain cases.  If you want to do that, you could just change the patch 
so that it merely replaces the code in the path where PR_GetErrorText 
produced no output:

>-	    PR_fprintf(PR_STDERR, errStrings[ADD_MODULE_FAILED_ERR], 
>-		       moduleName);
>+	    PR_fprintf(PR_STDERR, errStrings[ADD_MODULE_FAILED_STATUS_ERR],
>+		       moduleName, SECU_Strerror(PORT_GetError()));

If you want to do that, please submit another patch that does that.

Whether you do that, or leave this patch as it is, the patch will remove 
one use of the error string ADD_MODULE_FAILED_ERR, replacing it with a 
use of the error string ADD_MODULE_FAILED_STATUS_ERR.
If there are no remaining uses of ADD_MODULE_FAILED_ERR, then we should 
get rid of that string, too, IMO.  In that case, you could attach an 
additional patch to do that.
Comment 5 Elio Maldonado 2008-07-30 20:09:17 PDT
Created attachment 331822 [details] [diff] [review]
Use NSS error functions yet preserve detailed error.

Yes, is good to preserve the better error message. Thanks. New patch attached.
Comment 6 Elio Maldonado 2008-07-30 20:18:13 PDT
Created attachment 331825 [details] [diff] [review]
resending renamed as previous doesn't show good diff
Comment 7 Nelson Bolyard (seldom reads bugmail) 2008-07-31 10:12:18 PDT
Comment on attachment 331825 [details] [diff] [review]
resending renamed as previous doesn't show good diff

r=nelson
Comment 8 Elio Maldonado 2008-09-09 14:53:51 PDT
Nelson, r=nelson, was this an r+?
Comment 9 Nelson Bolyard (seldom reads bugmail) 2008-09-10 13:00:43 PDT
This checkin broke the automated tests on several platforms. 
See http://tinderbox.mozilla.org/showbuilds.cgi?tree=NSS
Comment 10 Elio Maldonado 2008-09-10 13:19:52 PDT
Changed: pk11.c v1.26, error.h v 1.6. Investigating tests that started failing.
Comment 11 Elio Maldonado 2008-09-10 18:48:30 PDT
The new test failures are caused by hard-coded values in the fips dbtests scripts. With this fix pk11 now returns a different error code and one unused error code was removed from err.h. The dbtest application in turn returns a different error code (one less than before) but the dbtests.sh and fips.sh scripts which call this app have the expected error code hard-coded to 46. It's now 45.  I have a suggested patch for dbtests.sh fips.sh that should fix this failures.  I'll open a bug on the other two shell scripts and attach the patches. We could also back off this fix for tonight until the other solution is in.
Comment 12 Wan-Teh Chang 2008-09-10 21:16:38 PDT
Created attachment 338036 [details] [diff] [review]
Preserve the values of error codes

We should avoid changing the exit codes of our command-line
tools.  In this case, we can accomplish this with this simple
patch.
Comment 13 Nelson Bolyard (seldom reads bugmail) 2008-09-10 21:24:35 PDT
OK, so the problem is actually the removal of an error code from 
modutil/error.h.  Backing that out will fix the tests and preserve the
good change to pk11.c.  

Wan-Teh pointed out that by not deleting members from that enumeration,
we assure that the values of the enumeration constants will not change.

Another way to fix the problem in error.h is to assign hard-coded numbers
to the members of the enumeration, so that when items are deleted from the
enumeration, the values of the other enumeration constants don't change, e.g.

 typedef enum {
   NO_ERR                         = 0,
   INVALID_USAGE_ERR              = 1,
   UNEXPECTED_ARG_ERR             = 2,
   UNKNOWN_OPTION_ERR             = 3,
   MULTIPLE_COMMAND_ERR           = 4,
   OPTION_NEEDS_ARG_ERR           = 5,

and so on.  Then when we delete an entry such as ADD_MODULE_FAILED_ERR
the values of the subsequent entries will not change, e.g .

   INVALID_CONSTANT_ERR           = 44,
   ADD_MODULE_FAILED_STATUS_ERR   = 46,
Comment 14 Wan-Teh Chang 2008-09-10 21:32:05 PDT
Created attachment 338041 [details] [diff] [review]
Preserve the values of error codes, v2

I did another cleanup.

Since there are no longer two flavors of the
"add module failed" error, we don't need the
_STATUS in the error code name to tell them
apart.
Comment 15 Nelson Bolyard (seldom reads bugmail) 2008-09-10 21:39:50 PDT
There should also be a block comment at the beginning of the enumeration explaining that the values of these enumerated constants are immutable,
and must not be changed.
Comment 16 Nelson Bolyard (seldom reads bugmail) 2008-09-10 21:57:33 PDT
Backed out change to cmd/modutil/error.h
Checking in error.h; new revision: 1.7; previous revision: 1.6
Comment 17 Elio Maldonado 2008-09-11 07:01:20 PDT
*** Bug 454716 has been marked as a duplicate of this bug. ***
Comment 18 Wan-Teh Chang 2008-09-11 10:34:25 PDT
Created attachment 338136 [details] [diff] [review]
Patch as checked in

This patch was diff'ed against the versions of the files
before Elio's checkin, so that we can see the overall changes
in one place.

I checked it in on the NSS trunk (NSS 3.12.2).

Checking in error.h;
/cvsroot/mozilla/security/nss/cmd/modutil/error.h,v  <--  error.h
new revision: 1.8; previous revision: 1.7
done
Checking in pk11.c;
/cvsroot/mozilla/security/nss/cmd/modutil/pk11.c,v  <--  pk11.c
new revision: 1.29; previous revision: 1.28
done
Comment 19 Wan-Teh Chang 2008-09-11 10:47:00 PDT
I also added the block comment that Nelson suggested in comment 15.

Note You need to log in before you can comment on or make changes to this bug.