--help option and ci: move env variables
#1008
Fixes #1005
Changes:
--help option to bench.c
help() function prints the help to command-linehave_invalid_args() checks if the user has entered an invalid argumentsecp256k1_bench_iters and secp256k1_test_iters environment variables declaration to .cirrus.yml121@@ -97,6 +122,25 @@ int main(int argc, char** argv) {
122 int d = argc == 1;
123 int iters = get_iters(20000);
124
125+ /* Check for invalid user arguments */
126+ char* valid_args[] = {"ecdsa", "verify", "ecdsa_verify", "sign", "ecdsa_sign", "ecdh", "recover",
127+ "ecdsa_recover", "schnorrsig", "schnorrsig_verify", "schnorrsig_sign"};
128+ int valid_args_size = 11;
sizeof(valid_args)
131+ if(argc > 1) {
132+ if(have_flag(argc, argv, "-h")
133+ || have_flag(argc, argv, "--help")
134+ || have_flag(argc, argv, "help")) {
135+ help(argv);
136+ return 1;
ls --help.
ls --help does say:
Exit status: 0 if OK, 1 if minor problems (e.g., cannot access subdirectory), 2 if serious trouble (e.g., cannot access command-line argument).
I used return 1 here since the bench_ecmult.c followed this convention.
Shall I also modify bench_ecmult's help to return 0?
126+ char* valid_args[] = {"ecdsa", "verify", "ecdsa_verify", "sign", "ecdsa_sign", "ecdh", "recover",
127+ "ecdsa_recover", "schnorrsig", "schnorrsig_verify", "schnorrsig_sign"};
128+ int valid_args_size = 11;
129+ int invalid_args = have_invalid_args(argc, argv, valid_args, valid_args_size);
130+
131+ if(argc > 1) {
if and for.
134+
135+ while (argv != NULL && argv != argm) {
136+ check = 0;
137+ for(i=0; i<n; i++) {
138+ if(!strcmp(*argv, valid_arg_list[i])) {
139+ check = 1; /* user entered a valid arg from the list */
break the loop here.
124@@ -125,6 +125,28 @@ int have_flag(int argc, char** argv, char *flag) {
125 return 0;
126 }
127
128+/* check for invalid command line argument */
129+/* valid_arg_list = list of allowed user arguments, n = size of valid_arg_list */
130+int have_invalid_args(int argc, char** argv, char** valid_arg_list, int n) {
valid_args instead of valid_arg_list. (Anyway, it’s an array, not a list. :)) Also size arguments typically use type size_t. (Okay, argc is an int but that’s just C legacy…)
124@@ -125,6 +125,28 @@ int have_flag(int argc, char** argv, char *flag) {
125 return 0;
126 }
127
128+/* check for invalid command line argument */
129+/* valid_arg_list = list of allowed user arguments, n = size of valid_arg_list */
130+int have_invalid_args(int argc, char** argv, char** valid_arg_list, int n) {
131+ int i, check;
check, e.g., found_valid.
133+ argv++;
134+
135+ while (argv != NULL && argv != argm) {
136+ check = 0;
137+ for(i=0; i<n; i++) {
138+ if(!strcmp(*argv, valid_arg_list[i])) {
strcmp(...) == 0 instead of !strcmp because the outputs of strcmp is not just a boolean value.
133+ || have_flag(argc, argv, "--help")
134+ || have_flag(argc, argv, "help")) {
135+ help(argv);
136+ return 1;
137+ } else if(invalid_args) {
138+ fprintf(stderr, "%s: unrecognized argument was entered.\n\n", argv[0]);
124@@ -125,6 +125,28 @@ int have_flag(int argc, char** argv, char *flag) {
125 return 0;
126 }
127
128+/* check for invalid command line argument */
16+ printf(" 1. ECDSA signing/verification\n");
17+ printf(" 2. ECDH key exchange (optional module)\n");
18+ printf(" 3. Public key recovery (optional module)\n");
19+ printf(" 4. Schnorr signatures (optional module)\n");
20+ printf("\n");
21+ printf("The default number of iterations for each benchmark is 20000. This can be customized\n");
bench.h. The other benchmarks (like bench_ecmlult uses 10000) use a different default iteration value so, I declared a local variable in the main function for the default value.
10@@ -11,6 +11,31 @@
11 #include "util.h"
12 #include "bench.h"
13
14+void help(char **argv) {
15+ printf("Benchmarks the following algorithms: \n");
:.
There is one corner case that is not covered. The binary prints the table header if the user tries to benchmark an optional module without building it. Shall I add a check for this? Or is it too much?
The check would look something like this:
0#ifndef ENABLE_MODULE_SCHNORRSIG
1 if (have_flag(argc, argv, "schnorrsig") || have_flag(argc, argv, "schnorrsig_sign") || have_flag(argc, argv, "schnorrsig_verify")) {
2 fprintf(stderr, "%s: schnorr module not enabled.\n\n", argv[0]);
3 return 1;
4 }
5#endif
19+ printf(" 4. Schnorr signatures (optional module)\n");
20+ printf("\n");
21+ printf("The default number of iterations for each benchmark is %d. This can be\n", default_iters);
22+ printf("customized using the SECP256K1_BENCH_ITERS environment variable.\n");
23+ printf("\n");
24+ printf("Usage: %s [args]\n", argv[0]);
What’s a little bit strange is that we typically run this via ./bench but this is a script to some autotools magic. So what I get here on my machine is something like Usage: /<redacted>/secp256k1/.libs/lt-bench [args]. I know using argv[0] is common but in this case, it’s rather cumbersome
Happy to keep my ACK but if you’re motivated, I think in this case a simple: Usage: ./bench [args] is better.
./bench value.
30+ printf(" ecdsa_verify : ecdsa verification algorithm\n");
31+ printf(" ecdsa_recover : ecdsa public key recovery algorithm\n");
32+ printf(" ecdh : ecdh key exchange algorithm\n");
33+ printf(" schnorrsig : all schnorr signature algorithms (sign, verify)\n");
34+ printf(" schnorrsig_sign : schnorr sigining algorithm\n");
35+ printf(" schnorrsig_verify : schnorr verification algorithm\n");
There is one corner case that is not covered. The binary prints the table header if the user tries to benchmark an optional module without building it. Shall I add a check for this? Or is it too much?
It won’t hurt but yeah since this is “only” the benchmarks, it’s not really necessary (like my other two comments). This PR is clearly an improvement even without those issues addressed.
./bench value
10@@ -11,6 +11,31 @@
11 #include "util.h"
12 #include "bench.h"
13
14+void help(int default_iters) {
15+ printf("Benchmarks the following algorithms:\n");
16+ printf(" 1. ECDSA signing/verification\n");
#ifdef ENABLE_MODULE_ECDH etc to make it only print the relevant ones (same below).
19+ printf(" 4. Schnorr signatures (optional module)\n");
20+ printf("\n");
21+ printf("The default number of iterations for each benchmark is %d. This can be\n", default_iters);
22+ printf("customized using the SECP256K1_BENCH_ITERS environment variable.\n");
23+ printf("\n");
24+ printf("Usage: ./bench [args]\n");
argv[0] for printing the binary name (which keeps working if it is renamed etc, or on systems where executable have extensions).
142+ return 1;
143+ }
144+ }
145+
146+ /* Check if the user tries to benchmark optional module without building it */
147+ #ifndef ENABLE_MODULE_ECDH
I find it somewhat ugly that there is this much module-specific information inside main(), but I don’t have a concrete suggestion on how to cleanly do it otherwise.
I’ll try to address this in a follow-up PR if no simple solution can be used here.
int check_ecdh_enabled(argc,argv)) in the bench.h. Not sure if this is a good approach.
Update:
#ifdef ENABLE_MODULE_ECDH etc to void help(), prints only the relevant module-info.
- added
#ifdef ENABLE_MODULE_ECDHetc tovoid help(), prints only the relevant module-info.
I forgot to add these to the arg list, which has been taken care of now.
The following functions were created:
1. bench.c: help()
- prints the help to the command-line
2. bench.h: have_invalid_args()
- takes a list of arguments that the user is allowed to enter on the command-line
- returns 1 if the user entered an invalid argument
- returns 0 if all the user entered arguments are valid
environment var moved:
1. SECP256K1_TEST_ITERS (replaces TEST_ITERS)
2. SECP256K1_BENCH_ITERS (replaces BENCH_ITERS)133+ size_t i;
134+ int found_valid;
135+ char** argm = argv + argc;
136+ argv++;
137+
138+ while (argv != NULL && argv != argm) {
argv will never be NULL, as it is the result of applying argv++; one or more times on the input. Did you mean to write *argv != NULL here?
argv is char**, so it’s a pointer to pointer, not an array.
Update:
argv != NULL check in the while loop traversing the user arguments.The argv cannot have a NULL value so, *argv != NULL might be the proper check here (as suggested in #1008 (review)). Then, I realized we also don’t need the *argv != NULL check in the while loop since, argv != argm condition will take care of this. The value of *argm (or argv[argc]) will be NULL if argm == argv + argc (C89 draft)