Removing function names from errors (PR 9058)

[Roumen Petrov]

Richard Levitte wrote:
> On Thu, 13 Jun 2019 20:23:05 +0200,
> Roumen Petrov wrote:
>> Hello,
>>
>> First I did not understand what is wrong to use function or
>> reasons. All of them are subject to particular "library".
> We didn't say anything against reason codes.  As a matter of fact, I
> do find those useful.  Function codes, on the other hand, are unstable
> and thereby quite useless, at least for figuring out errors.
I'm not aware of OpenSSL documentation that describes reason codes as 
stable (fixed).
If code is rewritten not only functions are changed. More or less 
reasons are changed as well.


>> To parse openssl error code in an application code is bad practice.
> Is it?  Would you say it's bad practice to look at errno codes too?
No as already in one of my post I wrote that  errsrt utility is very useful.
Bad practice is to check that reason code encoded into 'error' code has 
value of A or B.



>
> Error *text* is a different matter.
>
>> Richard Levitte wrote:
>>> On Thu, 13 Jun 2019 12:01:46 +0200,
>>> Matt Caswell wrote:
>>>>>    The
>>>>> additional information you're talking about is something we currently
>>>>> provide the ERR_add_error_data() function for, and that together with
>>>>> the reason text (derived from the reason code) is the data the end
>>>>> user can reasonably get.  It's up to whoever writes the error raising
>>>>> code to provide enough useful information.
>>>> Yes, although in practice we don't currently do this (we very rarely add
>>>> additional explanatory text). Not sure if that is a problem with the API, our
>>>> coding standards, or our culture.
>>> This is partly historical...  ERR_add_error_data() has been around
>>> since the beginning of time, and it looks to me like it was designed
>>> in a time where varargs hadn't universally caught on yet (yes, there
>>> was a time before varargs, and it's appropriate to call that "the
>>> stone age").
>> But developer could  format "extra message" for instance :
>>          NSSerr(NSS_F_RSA_SIGN, NSS_R_UNSUPPORTED_NID);
>>          {/*add extra error message data*/
>>              char msgstr[10];
>>              BIO_snprintf(msgstr, sizeof(msgstr), "%d", dtype);
>>              ERR_add_error_data(2, "NID=", msgstr);
>>          }
> I'll counter with a (yet fictitious):
>
>      ERR_raise_error(NSS_R_UNSUPPORED_NID, "NID=%d", dtype);
Reason code could be shared between functions :
$ grep CAPI_R_FILE_OPEN_ERROR *.c
e_capi.c:        CAPIerr(CAPI_F_CAPI_CTRL, CAPI_R_FILE_OPEN_ERROR);
e_capi.c:        CAPIerr(CAPI_F_CAPI_VTRACE, CAPI_R_FILE_OPEN_ERROR);
e_capi_err.c:    {ERR_PACK(0, 0, CAPI_R_FILE_OPEN_ERROR), "file open 
error"},

NSS engine has similar case:
$ grep NSS_R_MISSING_PVTKEY *.c
e_nss_dsa.c:        NSSerr(NSS_F_DSA_DO_SIGN, NSS_R_MISSING_PVTKEY);
e_nss_ec.c:        NSSerr(NSS_F_ECDSA_DO_SIGN, NSS_R_MISSING_PVTKEY);
e_nss_err.c:#define NSS_R_MISSING_PVTKEY                                 133
e_nss_err.c:    { ERR_REASON(NSS_R_MISSING_PVTKEY)              , 
"Missing private key" },
e_nss_key.c:            NSSerr(NSS_F_LOAD_KEY, NSS_R_MISSING_PVTKEY);
e_nss_rsa.c:        NSSerr(NSS_F_RSA_PRIV_DEC, NSS_R_MISSING_PVTKEY);
e_nss_rsa.c:        NSSerr(NSS_F_RSA_SIGN, NSS_R_MISSING_PVTKEY);

>
> or if you want to add information that helps debugging:
>
>      ERR_raise_error_debug(NSS_R_UNSUPPORED_NID,
>                            __FILE__, __LINE__, __FUNC__, "$Format:%H",
>                            "NID=%d", dtype);

This look like complete different solution. __FILE__ exposes some 
'private'  information (build related) and is some cases is not acceptable .
__FUNC__ does not look portable  - __func__ vs __FUNCTION__ vs "not 
defined".

Also going into this direction question is why to use "reason code".
Functionality similar to errno, sys_errlist  is also outdated.


> (since this is just an idea so far, it's perfectly possible to throw
> in a library code as well, but like I said already, dynamic library
> codes have their own issue)
>
> I know which I find easier to write.
>
>> There is no reason OpenSSL code to use ERR_add_error_data as usually
>> library packs whole functionality.
> That's not really true, there are places where OpenSSL code does use
> ERR_add_error_data(), and for good reason.
Hmm, you cut my note for externals like IO CAPI ;)
>   BIO_lookup and friends add
> the extra resolver error on error, the CONF code adds information on
> exactly what value causes a configuration file parsing error, the DSO
> code adds information on exactly what file it attempted to load (full
> path if possible), etc etc etc...  those are things that can't be part
> of the error code.
>
> Cheers,
> Richard
>

Roumen