Removing function names from errors (PR 9058)

[Richard Levitte]

On Thu, 13 Jun 2019 22:04:17 +0200,
Roumen Petrov wrote:
> 
> 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).

Perhaps, but that's not saying they aren't...  The reason codes,
specifically, have been fairly stable over time.  However, we can get
better.  Even better, we could get better at documenting them.

> If code is rewritten not only functions are changed. More or less
> reasons are changed as well.

Sometimes, that's true, and especially as new functionality has come
up.  We could certainly do a better job there by getting better at
giving consistent and more well defined reason codes.

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

I'm not talking about display for the moment.

> Bad practice is to check that reason code encoded into 'error' code
> has value of A or B.

Why?  Why would it be bad practice to check if the reason for an error
is ERR_R_MALLOC_FAILURE, for example?

You can make the exact same analogy with errno and checking its
values programatically.

    git grep 'errno *[!=]='

Considering the result of the above command, would you say that we're
having bad practice in OpenSSL code?

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

Err, yeah?  Of course they can!

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

Maybe you should look at the WHATEVERerr macros, here's one:

    # define SYSerr(f,r)  ERR_PUT_error(ERR_LIB_SYS,(f),(r),OPENSSL_FILE,OPENSSL_LINE)

OPENSSL_FILE and OPENSSL_LINE are macros that are aliases for
__FILE__ and __LINE__, if those are available.

So we already do expose that "private information".  How other
applications call ERR_PUT_error() is none of our business.

> __FUNC__ does not look portable  - __func__ vs __FUNCTION__ vs "not
> defined".

It was just an example, to give you an idea...  I didn't test it or
check it for correctness.

> Also going into this direction question is why to use "reason code".

It's to allow calling applications to react differently depending on
the reason for an error to occur.  How else would you have them know?

> Functionality similar to errno, sys_errlist  is also outdated.

Ok, so what kind of functionality do you want to offer?  Remember,
we're talking about a language that doesn't have much error handling
per se.  More advanced languages have an exception system, but even
there, the applicatin can catch them and react differently depending
on what the error is, so apart from the out-of-band nature of an
exception system (and automated unwinding), the difference isn't
really that big.

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

Because I wasn't arguing that.  I was arguing that what you were
saying about OpenSSL code is incorrect.

Cheers,
Richard

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