-1 so we can take this to a vote for a decision on this change

See also #1415.

We did this in BoringSSL years ago. It's worked out very well. In practice, anyone using these were doing so incorrectly. It reduces binary size, reduces maintenance effort, and avoids a large class of backwards compatibility issues where callers incorrectly depend on function codes and break as code moves around internally.

I think its a good idea - the removed value seems to convey very little.
Would this potentially break things for end users that may be checking these codes?

You have left the function stuff in place for the SYSerr level - so there are still SYS_F_ usages in place.
I was expecting, given the description, that ERR_GET_FUNC (X) would return 0 - but it still returns the function code.

I kept those because in this case it shows the actual syscall that failed. If you (or the OMC since you moved for a discussion/vote), prefer that I get rid of them I will. I suppose, pedantically, the right thing to do is to call ERR_add_error_data with the true syscall name, which I can also do if the OMC wants.

Also, if the OMC wants to move forward on this, I would be willing to edit all XXXerr functions to remove the _F_ parameter. The underlying API, of course, still has to have the function code parameter which it ignores.

I'm actually unsure how interesting it is to encode what syscall returned with an error. Isn't the error itself the interesting part? If, say, open() returned with an error, would you naturally code things like this?

    if ((fd = open("foo.txt", O_CREAT)) < 0)
        perror("Can't create \"foo.txt\"");

or like this?

    if ((fd = open("foo.txt", O_CREAT)) < 0)
        perror("open() doesn't let me create \"foo.txt\"");

If we feel the need to display the syscalls that were the origin of an error, maybe that's an indication that would could provide better error messages to start with? Also, if we absolutely feel the need and go with an error call like ERR_raise_error that I mentioned in #9003, it could always be part of additional text for the error.

I agree with @t-j-h

Not sure what you agree with... the way I read his input was just a state of fact that your works appears incomplete.

@levitte, I re-read things and you are right that I misunderstood @t-j-h 's comment.

Unlike other errors however, the SYSerr() stuff is used to report that something that OpenSSL asked the local system to do failed. It's not self-contained like most of the errors in include/openssl/*err.h (for example, BN_R_PRIVATE_KEY_TOO_LARGE and over a thousand others), and it's not like "malloc failure" which we call out explicitly with ERR_R_MALLOC_FAILURE as one of eight "global errors" in err.h. Rather the SYSerr() errors come after a system or library call, and if we don't want our users to dig into the source, we have to at least tell them what OpenSSL was doing that failed. This is probably not the final word in better error diagnosis, but it is a (big?) step forward in my view.

And, having written this long and detailed paragraph, of course it belongs over in #9072 but hopefully adding the link will suffice. :)

Is anyone on the OMC going to move forward with a discussion/vote on this, or should I close it?

I just started the discussion

It's been a month since @t-j-h placed the hold/-1 on this. It seems to have been two weeks since there was discussion on the openssl-project mailing list (https://mta.openssl.org/pipermail/openssl-project/2019-June/001451.html) Does the @openssl/omc have any plan of action?

If there is no other activity on this topic, I will close this PR and delete the branch next week. I'm fine with that, but I'm not fine with doing the exercise of rebasing all the time if nothing is going to happen.

That thread is quite long, and I'd like to catch up with it before commenting here (been away for more than a week, I've forgotten things). This PR is only part of the stuff that needs to happen, if any at all.

I think removing the function id is a positive change.
@levitte could you elucidate on what else is required?

@levitte could you elucidate on what else is required?

Well, not much that affects this PR per se... we did talk about extending the debug information surrounding an error in that thread, but that should probably be subject for another PR.

If the vote passes I'll make additional changes.

Ok. I'll just note that other than the few nits, this PR looks good to me.

I think the defines should stay in the public headers for now, should get deprecated, and can all be changed to the value 0.

@kroeckx is your comment at #9058 (comment) enough to clear your -1/hold? Or do we still need to wait for the vote. If I change mkerr to output zero-valued _F_ define's, should that block be wrapped in #if !OPENSSL_API_3 or something?

It's @t-j-h that put this PR on hold. (I did this for #9082, you didn't reply to my last comment.)

A vote related to this PR has been called, and is still open. I think it's unclear what the vote really is about, since it actually mentions the PR rather than what you want to achieve. But I think the intention of the vote is for removing the function names from the errors, not on how it's implemented. (I wish people would actually discuss what we voted on before calling a vote to prevent such unclear votes.)

So my comment is about the implementation. If you want, you can consider that a -1 on the implementation that goes away when you make that change.

Or do we still need to wait for the vote.

The vote is ongoing, we do need to wait for it either way.

If I change mkerr to output zero-valued _F_ define's, should that block be wrapped in #if !OPENSSL_API_3 or something?

I think that's what @kroeckx suggests. "should get deprecated" looks clear to me.

(I wish people would actually discuss what we voted on before calling a vote to prevent such unclear votes.)

Uhmmmm, but there was a discussion... the vote even links to it. However, mental note made, clearer vote text requested.

Merged.

aac96e2 Remove function name from errors
cbfa5b0 Regenerate mkerr files

So should we do the following from now on?
PROVerr(0, PROV_R_INVALID_QUESTION);

Yes, that's the idea

Defining all these existing macros to zero and also returning zero as the value of the function value means anyone checking function codes will end up getting false matches once this change is in place.

So should we do the following from now on?
PROVerr(0, PROV_R_INVALID_QUESTION);

For ones that are new, is it worth dropping the first argument?
For ones that aren't public, is it worth dropping the first argument?
For ones that are public, would an alternate name without the argument be viable?

Defining all these existing macros to zero and also returning zero as the value of the function value means anyone checking function codes will end up getting false matches once this change is in place.

Considering the function codes set were a moving target anyway, that risk was tangible either way.

For ones that are new, is it worth dropping the first argument?
For ones that aren't public, is it worth dropping the first argument?
For ones that are public, would an alternate name without the argument be viable?

So uhmmm, a new name for all, basically?

Not for all: PROVerr.
I'd not be agin XXXerror as a substitute that made them all internal.

Adding multiple names is a bad idea; the API surface of OpenSSL is already huge.

Wait until the next major release and rewrite the macros to just drop the argument.

I dont see any problem with removing it from Proverr which has never been released

Good point

Pleased to see that some simplifications are in progress that will remove redundancy w.r.t. function names and thus the need to keep function names and function code identifiers consistent.

Shouldn't also crypto/err/openssl.txt be simplified by removing the "function codes" section?

We're keeping the function codes for 3.0 for backward compatibility. Next release they could be removed; see the TODO comment in util/mkerr.pl

I see. So until then, when adding, removing, and renaming functions, one still needs to keep the related function code identifiers in crypto/err/openssl.txt, crypto/*/*err.c, and include/openssl/*err.h in sync.

In particular, when renaming functions that include *err() calls, one still must manually adapt those files (since util/mkerr.pl does not support function name changes and instead wrongly assumes new functions are being added and keeps the outdated names).

I think it would be easy to change mkerr so that it didn't complain about mismatched function names; would that address your comment?

New code can simply avoid the function macro completely.

    FOOerr(0, FOO_R_REASON);

(this has been possible since 1.1.0, I might add)

Why isn't the function argument just removed from the call?

It's a macro in a public header file, someone may be using it, so we kept it for compatibility for this release, just like we kept the _F_ defines

Very glad to learn that for new code one can simply replace the function code identifier argument by 0 in FOOerr(...) calls!
So regarding function codes no need to bother with mkerr.pl's capabilities, either.

So we'll do that for our CMP contribution at least for the current chunk 4 and all further ones.
Yet since also chunks 1..3 are not yet released we can prune the function codes there, too, right?