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
