Can you help me understand why making the functions themselves visible is necessary for replacing them via callbacks? I understand that the set callbacks function would need to be visible, but not the functions themselves.

Can you help me understand why making the functions themselves visible is necessary for replacing them via callbacks?

They are not necessary, though I would say having them in the public header doesn't harm much, as they are not exposed from the library ABI.

I understand that the set callbacks function would need to be visible, but not the functions themselves.

I can split them into a separate header, say <oqs/sha3_callbacks.h>, if preferred.

Can you help me understand why making the functions themselves visible is necessary for replacing them via callbacks?

They are not necessary, though I would say having them in the public header doesn't harm much, as they are not exposed from the library ABI.

Fair, although then it could be confusing to someone that they're exposed one way (in the headers) but not another way (in the binary).

I understand that the set callbacks function would need to be visible, but not the functions themselves.

I can split them into a separate header, say <oqs/sha3_callbacks.h>, if preferred.

I think this would be my preference.

@dstebila I split the header into {aes,sha2,sha3,sha3x4}_ops.h with only the callback API. Could you take another look?

@dstebila I split the header into {aes,sha2,sha3,sha3x4}_ops.h with only the callback API. Could you take another look?

Thanks @ueno, I think this makes sense. @praveksharma or @SWilson4, any thoughts?

I agree the logic of the PR makes sense and could merge as-is. However, @dstebila @hartm fyi, this is another example where an agreed-upon answer to open-quantum-safe/tsc#2 would be good. If my understanding of liboqs as an early stage, not-for-external-people-to-rely on project would hold, this PR could merge as-is as users will not expect API (visibility) stability in such project. If your take of it as a "production grade" library were to rule, things like API documentation and stability as in my comment should be considered, no? In that sense, my review feedback should rather be yours, so I discard it as a blocker for merge and leave this to you.

Sorry for being the joykill -- but am I the only one missing some documentation explaining this split (what is public, what is "private"? How/by whom to use which APIs? Sample code?)? I'm not sure everyone will just (want to) read CMakeLists.txt files to understand this (what I effectively see as an externally visible liboqs API) change.

I guess I viewed this as a "fixup" to #1603, moving things (the *_set_callbacks functions) into public headers that always should have been in public headers. All of the functionality was added in that previous PR, except that these (OQS_API) functions were (unintentionally) declared in non-public headers and hence couldn't be used as part of the API. So, I didn't deem documentation necessary beyond what was already added in that PR.

That said... taking a second look with an eye to documentation, I think the *_ops.h files need to be added to docs/.Doxyfile so that Doxygen picks up the API documentation.

As an aside, this also slipped by when reviewing #1603---maybe we need a CI check to ensure that all API functions are included in the generated docs.

@baentsch Given that we haven't even had the final NIST standards provided yet, I'm not too worried about breaking APIs at this point.

Your point, overall, is a very good one though. It's important for users to know whether or not APIs will be changed in the future. Typically projects indicate that they are likely to support APIs for a long time by issuing an LTS (Long Term Support) release. While APIs are obviously never completely set in stone, people expect that they won't have to constantly update if they use LTS code.

Does all of this make sense?

Given that we haven't even had the final NIST standards provided yet, I'm not too worried about breaking APIs at this point.

What's the connection between NIST providing algorithm standards and a library striving for API stability? Do you expect NIST to standardize APIs, @hartm? Can you please point to such intention? If so, the purpose of this project indeed remains "experimental" until NIST finalizes this. But then OQS should clearly state that its APIs shall not be considered reliable until the NIST standards are announced. I personally doubt NIST will mandate APIs, though, as that'd be a can of worms: Which languages would it support, say?

I'm not too worried about breaking APIs at this point.

I can understand you don't worry as you don't contribute or use liboqs afaik @hartm, but please consider the impact of API changes to a library others are/should be using --particularly if it has no formal LTS release done but strives for uptake as OQS does: This is not very friendly to all downstream users: I for one had been on the receiving end of API changes in many projects integrating this code and had to work long hours coping with such changes... This was one reason for me now wanting others to begin handling "OQS-internal" liboqs integrations, too, such as for them to experience this and give me support (getting stability and/or a hand in following the changes). And again, there may be more people using liboqs APIs, regardless or not of the project having declared an "LTS" version.

What's the connection between NIST providing algorithm standards and a library striving for API stability? Do you expect NIST to standardize APIs, @hartm? Can you please point to such intention? If so, the purpose of this project indeed remains "experimental" until NIST finalizes this. But then OQS should clearly state that its APIs shall not be considered reliable until the NIST standards are announced. I personally doubt NIST will mandate APIs, though, as that'd be a can of worms: Which languages would it support, say?

The vast majority of people with which I've spoken on PQC have said that they aren't planning on fully putting PQC into production until after the NIST standards are finalized.

NIST is not going to standardize APIs, of course, but they could change (or recommend changes) to minor things in protocols.

You are correct that OQS should state a policy on APIs for the sake of users.

I can understand you don't worry as you don't contribute or use liboqs afaik @hartm, but please consider the impact of API changes to a library others are/should be using --particularly if it has no formal LTS release done but strives for uptake as OQS does: This is not very friendly to all downstream users: I for one had been on the receiving end of API changes in many projects integrating this code and had to work long hours coping with such changes... This was one reason for me now wanting others to begin handling "OQS-internal" liboqs integrations, too, such as for them to experience this and give me support (getting stability and/or a hand in following the changes). And again, there may be more people using liboqs APIs, regardless or not of the project having declared an "LTS" version.

Who is using OQS in production deployments? And would they be upset at API changes?

Ultimately, as you allude, whether or not API changes go smoothly or not are mostly dependent on appropriate communication between maintainers/contributors and users. The decision belongs to the maintainers/OQS TSC (certainly not me, and not the people who use and don't contribute either). Some of these decisions can be complicated and involve weighing whether or not improving the project by improving the APIs is worth making life more difficult for a subset of the users.

I do generally worry about API changes, but 1) imagine that there aren't a lot of production deployments where these API changes would cause major grief (happy to be corrected here--please let me know if I'm wrong) and 2) am confident that API changes are better the earlier they are in a project's lifecycle. As the project grows, it becomes harder and harder to make these kinds of changes. So if it's something that's needed, it's better to do now rather than later.

I hope all of this makes sense!

Apologies @ueno for your PR being caught up in this discussion. I'm trying to move it to TSC issues that have been left open for (too?) long.

My ask to you only was to consider adding documentation for this public/internal API split -- and I'm grateful for the comment by @SWilson4 picking/following that up: We could surely also split this into a separate issue regarding API documentation in general: If that approach would be OK for you, @SWilson4 (?) let's merge this when CI turns green.

Apologies @ueno for your PR being caught up in this discussion. I'm trying to move it to TSC issues that have been left open for (too?) long.

My ask to you only was to consider adding documentation for this public/internal API split -- and I'm grateful for the comment by @SWilson4 picking/following that up: We could surely also split this into a separate issue regarding API documentation in general: If that approach would be OK for you, @SWilson4 (?) let's merge this when CI turns green.

That works for me. I was the one who created the external/internal API split with #1648, so I will bear the burden of documenting it. 🙂 I just created #1841 to track.

@ueno has updated the Doxyfile to pick up the OQS_*_set_callbacks functions, so I think the in-scope documentation for this PR has been dealt with.