I would frankly rather see this as an entirely separate PR. These PRs with stuff from other PRs included do make them hard to review in a sane way...

I appreciate the difficulty, but this seems endemic to multi-part work. At any rate, I promise to rebase once #8886 gets reviewed and merged.

Do we need the same for the legacy provider?

I thought matt was doing some PR's in a similar way to this - which just state 'ignore the first 3 commits in this PR'.

We don't need this for the legacy provider because it links against openssl, or is part of libcrypto, and can call crypto functions directly. If that changes, then we might need this.

There seems to be some cross over between this PR and changes I already made in PR #8728. I'd prefer this PR did not go in until that one has (it's already been delayed multiple times by other PRs and I don't want to drag it out longer).

Yes, deliberately there is cross over, and this can wait for #8728 (hopefully that's not going to wait for the asn1/object issue to be resolved); if that goes in, I will rebase this and "undo" the overlapping code that is in this PR. OF course, this PR needs #8886 merged first as well.

Rebased on latest #8886

Rebased; needs reviews. As a reminder, based on #8886; also addresses the "error codes needed in providers" issue, hopefully good enough for this release.

There is no need to have this based on #8886, it can very well be made independent.

I can do that, but it seems that this one is a bit more controversial and #8886 has no such issues, so I''m assuming that will go in first anyway.

Please do

Okay, rebased against master and pushed.

I changed the names to be consistent, ending with the official OpenSSL name. This is the same convention that I followed in #8886, and that is kinda followed with the two functions already here.

I disagree, your names do not follow the existing pattern.

Well, they follow the pattern used in the PR that this was first based on. I think it's better than what is currently there. Is there a pattern for these lines? IF so, I could not figure it out:

     { OSSL_FUNC_CORE_GET_PARAM_TYPES, (void (*)(void))core_get_param_types },
     { OSSL_FUNC_CORE_GET_PARAMS, (void (*)(void))core_get_params },
    { OSSL_FUNC_CORE_PUT_ERROR, (void (*)(void))ERR_put_error },
    { OSSL_FUNC_CORE_ADD_ERROR_VDATA, (void (*)(void))ERR_add_error_vdata },

IF so, I could not figure it out

Simple, it's functions that the core is offering to the provider. The fact that they are called ERR_blah in libcrypto doesn't matter.

That's broken in my opinion. Feel free to take over this PR and use whatever names you want if you feel that strongly about it.

And further why is the full name for CORE_GET_PARAM_TYPES match the function name, but the short name CORE_PUT_ERROR not match anything like the function name? Presumably remove GET from the first two, or put ERR in the second two.

The patterm I started is that OSSL_FUNC_CORE is a common prefix for the number macros for anything the core offers to the provider. My first thought was actually that all functions offered would be core_something, but if someone wants to shortcut to offer ERR_put_error et al directly, I see no direct harm. get in core_get_param_types and core_get_params is part of the function name proper, not its prefix (core_), that's why it's reproduced in the number macro.

Feel free to take over this PR and use whatever names you want if you feel that strongly about it.

Oh, ok... sure, can do that if that's what you want

I think a much better naming convention is the one I used, not surprisingly. Feel free to take over this and #8886 and close these if you wish. My reasoning is that an upcall should look exactly like a regular call as much as possible; and, in fact, one could replace all the upcalls in this and #8886 with a macro similar to OSSL_CORE_MAKE_FUNC

My reasoning is that an upcall should look exactly like a regular call as much as possible

That doesn't have to imply anything on the names of the dispatch number macros, for example. What if we, somewhere down the line, need to wrap ERR_put_error with core_put_error for providers, should we then change the dispatch number macro name? That's a bit silly.

"What if somewhere down the line...."

As I've said before if that day ever comes, we can consider what to do.

Well, this is definitely where you and I see things differently. You want implementation details encoded in the dispatch number macro names. I don't, I actually think that's a flaw.

Not quite. I want the name of the API being provided encoded, yes.

The libcrypto <-> provider interface is a new API.

Yes, and I think the function pointers and the names to indicate the function that are being passed through that API should match the names of the API's that are being passed. There should not be an extra level of indirection. "What does this function do?" Look at the define, look at the function pointer, either way, and then look at the docs.

We clearly have different ideas of what the dispatch should say. I just had a look at one of the names you propose, OSSL_FUNC_CORE_GET_ERR_PUT_ERROR, and that GET had me realise that you're thinking of the dispatch table as accessors, i.e. something to get diverse parts of the OpenSSL API, while I see it more as a set of functions offered by the core, but that may or may not be implemented as well known OpenSSL functions, and there's really no reason why the provider needs to know that implementation detail.

This discussion, however, is a reminder that the set of dispatch names and functions aren't very well documented. That needs to be started. I have no idea where I'd place that documentation in man-page terms, though.

"my not be implemented a well known OpenSSL functions"

But we're writing in C, which names functions, so there will be a name. And manpages are organized around function names, so a simple mapping of core/dispatch functionality to documented function seems obvious. So obvious to me, that I fail to imagine doing it any other way :)

So revisiting this, there's this comment in core_numbers.h:

/*-
 * Identities
 * ----------
 *
 * All series start with 1, to allow 0 to be an array terminator.
 * For any FUNC identity, we also provide a function signature typedef
 * and a static inline function to extract a function pointer from a
 * OSSL_DISPATCH element in a type safe manner.
 *
 * Names:
 * for any function base name 'foo' (uppercase form 'FOO'), we will have
 * the following:
 * - a macro for the identity with the name OSSL_FUNC_'FOO' or derivates
 *   thereof (to be specified further down)
 * - a function signature typedef with the name OSSL_'foo'_fn
 * - a function pointer extractor function with the name OSSL_'foo'
 */

So logically, if you want the core to offer ERR_put_error and you want to macro name, signature and all that to match, it should be like this:

# define OSSL_FUNC_ERR_PUT_ERROR      3
OSSL_CORE_MAKE_FUNC(void, ERR_put_error,
                (int lib, int func, int reason, const char *file, int line))
# define OSSL_FUNC_ERR_ADD_ERROR_VDATA 4
OSSL_CORE_MAKE_FUNC(int, ERR_add_error_vdata, (int num, va_list args))

Yes...what @levitte said.

@mattcaswell, @levitte : isn't this exactly what this code does?

; git diff master include/openssl/core_numbers.h 
diff --git a/include/openssl/core_numbers.h b/include/openssl/core_numbers.h
index 8d026f4..8970061 100644
--- a/include/openssl/core_numbers.h
+++ b/include/openssl/core_numbers.h
@@ -58,12 +58,11 @@ OSSL_CORE_MAKE_FUNC(const OSSL_ITEM *,
 # define OSSL_FUNC_CORE_GET_PARAMS             2
 OSSL_CORE_MAKE_FUNC(int,core_get_params,(const OSSL_PROVIDER *prov,
                                          const OSSL_PARAM params[]))
-# define OSSL_FUNC_CORE_PUT_ERROR              3
-OSSL_CORE_MAKE_FUNC(void,core_put_error,(int lib, int func, int reason,
-                                         const char *file, int line))
-# define OSSL_FUNC_CORE_ADD_ERROR_VDATA        4
-OSSL_CORE_MAKE_FUNC(void,core_add_error_vdata,(int num, va_list args))
-
+# define OSSL_FUNC_CORE_GET_ERR_PUT_ERROR      3
+OSSL_CORE_MAKE_FUNC(void, ERR_put_error,
+                (int lib, int func, int reason, const char *file, int line))
+# define OSSL_FUNC_CORE_GET_ERR_ADD_ERROR_VDATA 4
+OSSL_CORE_MAKE_FUNC(int, ERR_add_error_vdata, (int num, va_list args))
 
 /* Functions provided by the provider to the Core, reserved numbers 1024-1535 */
 # define OSSL_FUNC_PROVIDER_TEARDOWN         1024
; 

Nope. You're inserting an extra CORE_GET in the middle of the dispatch number macro names.

ah, I was focused on the functions not the define's. Fixed, thanks.

argh, forgot to push the amended commit. oops. fixed both.

If #9072 is accepted, then I will modify this PR to expose an upcall so providers can call the new function.

There are no open issues, rebased against recent master; ping for reviews.

No unresolved issues, rebased, ping.

I think we need to put this on hold for a bit, 'cause I'm not sure that a provider should actually have to call ERR_put_error directly. What should it use for the lib argument, for example? My take on it is that it shouldn't even need to bother with that, that this is a value that should be taken care of in libcrypto... the consequence would be that libcrypto will have to offer other error reporting functions.

I do agree that ERR_add_err_data and ERR_add_err_vdata should append their input to already added data, and I wish that you'd make a separate PR for that alone, not involving the libcrypto<->provider stuff.

I can split off the "vdata append" into a separate PR if you want, but the current code already includes ERR_put_error so putting it on hold seems kinda wrong :)

(Commit 3593266 by @mattcaswell added ERR_put_error). The key part of this PR is that it turns all put-err calls into two calls, the second one appends "in the FIPS module" text.

The key part of this PR is that it turns all put-err calls into two calls, the second one appends "in the FIPS module" text.

I didn't quite see that as the key part, but sure.

However, the main reason I wanted to hold a bit was #9174, which gives providers in general a bit more sanity.

If the key part was appending error data so you could append "(in the FIPS module)", then sure, you can do that as well, in the same PR where you change the semantics of ERR_add_error_data. That in itself should remain unaffected by the whole discussion about exactly what names we have for the libcrypto<->provider interface.

I saw and commented on #9174, so I will make a new PR with just the append and cleanup stuff.

Closing in favor of #9181