Removing function names from errors (PR 9058)

[Richard Levitte]

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.

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

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);

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);

(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.  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

-- 
Richard Levitte         levitte at openssl.org
OpenSSL Project         http://www.openssl.org/~levitte/