390527 - get rid of pkixErrorMsg variable in PKIX_Error

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Alexei Volkov]

The library provides only a description of an occurred error, not it's numeric code. PKIX_UInt32 code is actually the code of type of a component where an error  happened.   

 53 struct PKIX_ErrorStruct {
 54         PKIX_UInt32 code;
 55         PKIX_Error *cause;
 56         PKIX_PL_Object *info;
 57         PKIX_PL_String *desc;
 58 };

Having numeric error code is essential for pkix to nss error conversion.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Alexei, I agree that this should be fixed.  It's probably pretty easy to do.
But there is going to be a HUGE merge conflict between any patch written 
for this bug, and the existing patches for the libpkix bloat bug, and for 
the CRLDP bug.  So, please give priority to reviewing the patches for those
bugs, so that they can get checked in BEFORE you write the patch for this 
bug.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

libPKIX has two enumerations that appear to be enumerations of error numbers.
They are PKIX_ERRORNUM and PKIX_ERRSTRINGNUM.  But this is deceptive!
PKIX_ERRORNUM is *NOT* !! an set of error code numbers.  

PKIX_ERRSTRINGNUM is the one and only enumeration of libPKIX error numbers.

Note that in order to create a table that maps libPKIX's PKIX_ERRSTRINGNUM
numbers onto NSS/NSPR style error numbers, it will be necessary to FIX 
(make permanent) the numbers of each and every member of PKIX_ERRSTRINGNUM. 
Any patch to do that will conflict with the outstanding CRLDP patch. 
So, let's please get the CRLDP patch reviewed and checked in.  
THEN we can work on making the PKIX_ERRSTRINGNUM values permanent.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Actually, we need to get rid of strings in the libPKIX shared library.
NSS shared libraries do NOT do UI, and do NOT provide error strings.

  PKIX_PL_String *desc;     should go away entirely.

  PKIX_UInt32 code;         should become 
  PKIX_ERRORNUM code;

and  I propose to change 
  PKIX_PL_String *desc;     into
  PKIX_ERRSTRINGNUM desc;

Comment 4

[Alexei Volkov]

Attachment: initial patch v1 (checked in)

replacing usage of strings in PKIX_Error structure with numeric error codes.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 279008 [details] [diff] [review]
initial patch v1 (checked in)

I'm giving this r+ because it seems good as far as it goes, but there is more
to be done after this.  This is a good first step.  

Review comments.

1. There are several places where code was commented out by surrounding it with 
c style comment symbols, /* and */.  Please delete those lines of code, rather
than preserving them in that fashion. 

2. Why does the file lib/libpkix/include/pkix_errorstrings.h still exist?
Aren't we getting rid of all strings?
I expected to see the definition of the macro PKIX_ERRORENTRY be changed,
but I didn't see any such change in this patch.  
Hopefully there will be no more string definitions left.

3. We discussed changing the names of certain typedefs.
Two that I remember are:
PKIX_ERRORNUM       -> PKIX_ExceptionClassNum
PKIX_ERRSTRINGNUM   -> PKIX_ErrorNum

These should not be all caps since they are not macro names, but rather 
are typedefs.

Let's make those two global search-and-replace changes in a separate patch,
after this patch is committed.

Comment 6

[Alexei Volkov]

patch v1 is integrated.

> Review comments.
> 
> 1. There are several places where code was commented out by surrounding it with 
> c style comment symbols, /* and */.  Please delete those lines of code, rather
> than preserving them in that fashion. 

Done.

> 
> 2. Why does the file lib/libpkix/include/pkix_errorstrings.h still exist?
> Aren't we getting rid of all strings?
> I expected to see the definition of the macro PKIX_ERRORENTRY be changed,
> but I didn't see any such change in this patch.  
> Hopefully there will be no more string definitions left.
I think the file pkix_errorstrings.h will exist in some was. We still need
define numeric error codes. But we will be of the error description. I would like to postpone doing anything at this point, before the bug 391183 is fixed(convert libpkix error code to nss error code).

> 
> 3. We discussed changing the names of certain typedefs.
> Two that I remember are:
> PKIX_ERRORNUM       -> PKIX_ExceptionClassNum
> PKIX_ERRSTRINGNUM   -> PKIX_ErrorNum

Have no problem of doing it now.
> 
> Let's make those two global search-and-replace changes in a separate patch,
> after this patch is committed.

Nelson, since you have the magic script that does this, could you please do the replacements.

Comment 7

[Alexei Volkov]

Additional cleanup is needed to get rid of pkixErrorMsg variable from multiple pkix macros and through out pkix library code.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

I have attached a patch to bug 391183 to do the renaming.  
When that is reviewed and committed, I will be happy to do the additional
work to get rid of the pkixErrorMsg variable.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Alexei, this bug has a patch with r+ that is over 1 year old.
It was blocked by bug 391183 but that bug is now resolved/fixed.
So, can this patch be landed now?

Comment 10

[Alexei Volkov]

Nelson, the patch was committed. We kept the bug to flag, that we need to get rid of the pkixErrorMsg variable in the PKIX_Error structure.
Do you have a patch that would remove pkixErrorMsg? I think we should get rid of the member of the PKIX_Error structure before next 3.12 release.

Comment 11

[Alexei Volkov]

Attachment: Patch v2. Remove references to PKIX_ErrorText and pkixErrorMsg variable

nss dll blob goes from 1923k to 1870k when compiled with this patch.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 346145 [details] [diff] [review]
Patch v2. Remove references to PKIX_ErrorText and pkixErrorMsg variable

r+ with a few recommended changes...

>+#ifndef PKIX_ERROR_DESCRIPTION
>+        char errorStr[32];
>+#endif

>+#ifndef PKIX_ERROR_DESCRIPTION
>+        sprintf(errorStr, "Error code: %d", error->errCode);
>+#endif

Please change all the sprintf calls that fill a fixed-size 
pre-allocated buffer to use PR_snprintf instead.

>+    char error[32];

>+#else
>+    sprintf(error, "%d", errorCode);

here too.  (and any others that I missed)


>@@ -680,11 +678,7 @@ pkix_pl_CollectionCertStoreContext_Popul
>         if ((prError != 0) && (prError != PR_NO_MORE_FILES_ERROR)) {
>                 PKIX_COLLECTIONCERTSTORECONTEXT_DEBUG
>                         ("\t\t Calling PR_GetErrorText.\n");
>-                prErrorTextLen = PR_GetErrorText(prErrorText);

I believe the above macro just logs that string in debug builds, and
does nothing in optimized builds. Since the comment string is no longer
true, you might as well remove the above two lines, too.

>@@ -857,12 +848,7 @@ pkix_pl_CollectionCertStoreContext_Popul
>         if ((prError != 0) && (prError != PR_NO_MORE_FILES_ERROR)) {
>                 PKIX_COLLECTIONCERTSTORECONTEXT_DEBUG
>                         ("\t\t Calling PR_GetErrorText.\n");
>-                prErrorTextLen = PR_GetErrorText(prErrorText);

same here.

Comment 13

[Wan-Teh Chang]

Attachment: Include "prprf.h" for PR_snprintf

We need to include "prprf.h", otherwise the "implicit function
declaration" function warnings break the Linux build.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 349111 [details] [diff] [review]
Include "prprf.h" for PR_snprintf

All the problems occur when PKIX_ERROR_DESCRIPTION is defined.
WHY is that defined?  
(note: I'm not asking HOW or WHERE is it defined.  
I'm asking for what purpose or motive is it defined?)

Do we need it? 
Can we just remove the definition of that symbol?

Comment 15

[Alexei Volkov]

Comment on attachment 349111 [details] [diff] [review]
Include "prprf.h" for PR_snprintf

Wan-Teh, thank you for the patch and checking!