Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add functions exposing constants for all operation/primitive pairs #42

Merged

Conversation

stouset
Copy link
Contributor

@stouset stouset commented May 16, 2013

Wrappers for other languages can't access constants defined by C
preprocessor macros, so they must be exposed as functions. This was
already done for the "default" implementation of each operation, but
this commit adds functions for (almost) all of the underlying
primitives.

A few are currently excluded due to problems with the automated script
used to generated this commit. They are:

  • crypto_onetimeauth_poly1305
  • crypto_generichash_blake2b
  • crypto_verify16
  • crypto_verify32

@stouset
Copy link
Contributor Author

stouset commented May 16, 2013

In case you're morbidly curious, this was done with:

find src/libsodium/include/sodium -name "crypto_*_*.h" | cut -d\/ -f5 | sed -e 's/\.h//' | tr "_" " " | cut -d\  -f 2,3  | while read OPER PRIM; do IMPL=$(find src/libsodium/crypto_${OPER}/${PRIM} -type d -mindepth 1 -maxdepth 1 | head -1 | cut -d\/ -f5); (echo \#include \"crypto_${OPER}_${PRIM}.h\" && cat src/libsodium/crypto_${OPER}/${PRIM}/${IMPL}/api.h | egrep "#define\s+crypto_[[:alpha:]]+_[A-Z]+" | grep -v IMPLEMENTATION | grep -v PRIMITIVE | sed -E "s/#define crypto_${OPER}_([[:alpha:]]+.*)/\1 size_t/g" | tr " " "\t" | sed -E 's/(\"[[:alnum:]]+\"[[:space:]]+)size_t/\1const char */' | awk -F\t "{ print \"\n\" \$3 \"\ncrypto_${OPER}_${PRIM}_\" tolower(\$1) \"(void) {\n    return \" \$2 \";\n}\" }") > src/libsodium/crypto_${OPER}/${PRIM}/${OPER}_${PRIM}.c; sed -e "/crypto_${OPER}\/${PRIM}\/${IMPL}\/api.h/i \\
\ \ \ \ \ \ \ crypto_${OPER}/${PRIM}/${OPER}_${PRIM}.c \\\\
" -i '' src/libsodium/Makefile.am; sed -e '/#include "export.h"/i \
#include <stddef.h>' -i '' src/libsodium/include/sodium/crypto_${OPER}_${PRIM}.h; DEFS=$(cat src/libsodium/crypto_${OPER}/${PRIM}/${IMPL}/api.h | egrep "#define\s+crypto_[[:alpha:]]+_[A-Z]+" | grep -v IMPLEMENTATION | grep -v PRIMITIVE | sed -E "s/#define crypto_${OPER}_([[:alpha:]]+.*)/\1 size_t/g" | tr " " "\t" | sed -E 's/(\"[[:alnum:]]+\"[[:space:]]+)size_t/\1const char */' | awk -F\t "{ print \"SODIUM_EXPORT\n\" \$3 \" crypto_${OPER}_${PRIM}_\" tolower(\$1) \"(void);\n\" }" | awk '{printf("%s\\n", $0)}'); awk "/SODIUM_EXPORT/ && !x {printf(\"$DEFS\"); x=1} 1" < src/libsodium/include/sodium/crypto_${OPER}_${PRIM}.h | sponge src/libsodium/include/sodium/crypto_${OPER}_${PRIM}.h; done

@stouset
Copy link
Contributor Author

stouset commented May 16, 2013

Whoops, slight problem. Hold off for a moment.

Wrappers for other languages can't access constants defined by C
preprocessor macros, so they must be exposed as functions. This was
already done for the "default" implementation of each operation, but
this commit adds functions for (almost) all of the underlying
primitives.

A few are currently excluded due to problems with the automated script
used to generated this commit. They are:

  * crypto_onetimeauth_poly1305
  * crypto_generichash_blake2b
  * crypto_verify16
  * crypto_verify32
@stouset
Copy link
Contributor Author

stouset commented May 16, 2013

Good to go! 👍

@stouset
Copy link
Contributor Author

stouset commented May 16, 2013

Apparently not. The new source files generate object files that conflict with existing ones. What do you suggest I rename them to? s/$operation_$primitive.c/crypto_$operation/$primitive.c?

@jedisct1
Copy link
Owner

Which ones are conflicting?

@stouset
Copy link
Contributor Author

stouset commented May 16, 2013

Check the Travis build and it'll show you. I just ran out to lunch.

@jedisct1
Copy link
Owner

One way to solve this is probably to rename implementation files to include the implementation name.
Like stream_aes128ctr_ref.c instead of stream_aes128ctr.c.

@stouset
Copy link
Contributor Author

stouset commented May 16, 2013

It's your project, but I feel like it makes sense to the implementation folders themselves untouched from their original state in NaCl.

@jedisct1
Copy link
Owner

Renaming the files doesn't change the content.

NaCl uses the same file names for all implementations of a given primitive. That's fine with the build system it was using, but it doesn't play well with autotools if we want to limit recursion.
As long as files are named in a consistent way, there's nothing wrong with renaming them.

stouset added 2 commits May 16, 2013 15:27
This function definition was created by an automated script that
incorrectly handled a corner case.
The automated script that generated functions for looking up #define'd
constants didn't handle edge cases in these files, so these have been
added by hand. They're thus either more likely or less likely to
contain mistakes (depending on one's particular point of view).
@stouset
Copy link
Contributor Author

stouset commented May 16, 2013

By the time you replied, I'd already named the files to $operation_$primitive_api.c. I hope that's alright.

I've also added functions for the remaining constants that weren't handled correctly by the automated script. Pending confirmation from Travis, this is (hopefully) ready to be merged.

@stouset
Copy link
Contributor Author

stouset commented May 17, 2013

Please let me know if I've missed any stylistic conventions you'd like me to follow.

@stouset
Copy link
Contributor Author

stouset commented May 20, 2013

@jedisct1 What's left on this to do before it's mergeable?

@jedisct1
Copy link
Owner

Hi Stephen,

Sorry, I didn't have time to review it last weekend. But it looks good. I'll review it and merge it as soon as possible.

@jedisct1 jedisct1 merged commit 98c02a2 into jedisct1:master May 22, 2013
@jedisct1
Copy link
Owner

Merged! Thanks a lot for your help!

@stouset stouset deleted the add-lookup-methods-for-all-constants branch May 22, 2013 19:20
@stouset
Copy link
Contributor Author

stouset commented May 22, 2013

👍

@stouset
Copy link
Contributor Author

stouset commented May 31, 2013

Any chance this will be released in a version 0.4.2 sometime soon?

@jedisct1
Copy link
Owner

Sure, I'll try to work on it next weekend.

@stouset
Copy link
Contributor Author

stouset commented May 31, 2013

👍

@stouset
Copy link
Contributor Author

stouset commented Jul 8, 2013

Any word on a new release? This would make my life somewhat easier for continuous integration.

@jedisct1
Copy link
Owner

jedisct1 commented Jul 8, 2013

0.4.2 has been released already :)

@stouset
Copy link
Contributor Author

stouset commented Jul 8, 2013

HA!

herpderp

@jedisct1
Copy link
Owner

jedisct1 commented Jul 8, 2013

Yep, thanks for your help, bro!

pop

Repository owner locked and limited conversation to collaborators Aug 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants