119368 - SECU_PrintError should not print an empty error message when the error code is unknown.

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Wan-Teh Chang]

SECU_PrintError maintains a table error messages for the error
codes that it knows about.  If a new error code is added but
the error message table for SECU_PrintError is not updated,
SECU_PrintError will print nothing about the error.  This makes
debugging difficult.

I suggest that if SECU_PrintError can't find an error message
for the error code, it should print the error code in decimal.
For example, "error -5928".

Comment 1

[Wan-Teh Chang]

Assigned the bug to Bishakha.

Comment 2

[Bishakha Banerjee]

Attachment: patch

Created an attachment: patch

Comment 3

[Wan-Teh Chang]

Comment on attachment 68052 [details] [diff] [review]
patch


>-	fprintf(stderr, "\n");
>+	fprintf(stderr, ": %d\n", err);

It is better to day:
	fprintf(stderr, ": error %d\n", err);

Please attach an updated patch.  Thanks!

Comment 4

[Bishakha Banerjee]

Attachment: updated patch based on Wan-Teh's comments

Comment 5

[Wan-Teh Chang]

Comment on attachment 68161 [details] [diff] [review]
updated patch based on Wan-Teh's comments

r=wtc.

>-	fprintf(stderr, "\n");
>+	fprintf(stderr, ": error %d\n", err);

A nit: it is better to add a (int) cast in front of err:
	fprintf(stderr, ": error %d\n", (int)err);

This is because err is of the type PRErrorCode, which
is a PRInt32.  PRInt32 is not necessarily an int.
The %d format specifier requires an int, and compilers
don't do automatic type conversion for functions like
fprintf() and fscanf(), so it is safer to cast it to
an int to match the %d.

So, add the (int) cast and you can go ahead and check
it in.	Set the target milestone to 3.4, and mark the
bug RESOLVED.

Thanks.

Comment 6

[Bishakha Banerjee]

Attachment: updated patch after furthur review by Wan-Teh

According to the checkin instructions on Mozilla, I waited till the tree was
green across all platforms, so I had to wait till this afternoon. When I went
to check in my change, I could not. I did not have permission to check into
mozilla/security. I have sent email against my CVS account creation bug
regarding that, but do not know when I will get a response. Meanwhile, to be
able to check this into target NSS 3.4:

Wan-Teh, Nelson,
could either of you please check in my change. I have built and verified
nothing breaks on Windows and Linux.

I will modify the bug appropriately after that and mark it RESOLVED
-Bishakha

Comment 7

[Wan-Teh Chang]

Comment on attachment 68449 [details] [diff] [review]
updated patch after furthur review by Wan-Teh

r=wtc.	Bishakha, please go ahead and check it in.
Remember to set target milestone to 3.4 and mark
the bug fixed.

To answer your questions:
1. Checkins to NSS are not subject to the Mozilla
client tree closure.  We are an independent group.
We only share the cvs repository and bug database
with them, but not much else.

Mozilla client pulls a stable snapshot of NSS (the
cvs tag is NSS_CLIENT_TAG), so the tip of NSS is
owned by our team.

2. There is cvs commit access control on the
mozilla/security directory.  I forgot to add you
to the access control list.  Sorry.  I just fixed
that.  You should be able to check in this patch
now.

Thanks.

Comment 8

[Bishakha Banerjee]

Checked in mozilla/security/nss/cmd/lib/secutil.c;
new revision: 1.32; previous revision: 1.31

Marking the bug "FIXED" and setting target to NSS 3.4