Stdarg based OSSL_PARAM array building.#9246
Stdarg based OSSL_PARAM array building.#9246paulidale wants to merge 6 commits intoopenssl:masterfrom
Conversation
Add a function that allows an array of OSSL_PARAMs to be allocated and populated in one call.
|
see #9139 (comment) |
crypto/params.c
Outdated
|
|
||
| for (; name != NULL; name = va_arg(ap, const char *)) { | ||
| type = va_arg(ap, int); | ||
| #define T(type, type_name, param_type) \ |
There was a problem hiding this comment.
maybe make this be CASE instead of T
There was a problem hiding this comment.
Looks kind of ugly, too many leading capitals after argument reordering.
There was a problem hiding this comment.
yeah, but then it looks kinda like C code.
crypto/params.c
Outdated
| break; \ | ||
| } | ||
| switch (type) { | ||
| T(int, int, INTEGER); |
There was a problem hiding this comment.
put the all-caps, the type_name first.
There was a problem hiding this comment.
Made this change.
| int setup_tests(void) | ||
| { | ||
| ADD_ALL_TESTS(test_case, OSSL_NELEM(test_cases)); | ||
| ADD_TEST(test_build); |
There was a problem hiding this comment.
Can't it be made into a set of test cases alongside the other test cases?
There was a problem hiding this comment.
The storage for each item is allocated dynamically which means the global variables aren't being updated. This goes against the premise of the other test cases. (I tried).
| params = OSSL_PARAM_build( | ||
| OSSL_PARAM_utf8_string("foo", NULL, 32), | ||
| OSSL_PARAM_int("bar", 42), | ||
| NULL |
There was a problem hiding this comment.
This doesn't correspond to what's described in this manual.
There was a problem hiding this comment.
The comment at line 150 says OSSL_PARAM_END structure the text of the docs say NULL.
There was a problem hiding this comment.
It also says automatically :)
I'll change.
doc/man3/OSSL_PARAM_build.pod
Outdated
|
|
||
| =head1 HISTORY | ||
|
|
||
| These APIs were introduced in OpenSSL 3.0.0. |
There was a problem hiding this comment.
"APIs" should be "functions"
(using "API" as synonym for "functions" is an abomination IMNSHO)
There was a problem hiding this comment.
It's not just functions though: it's also macros and various constants: together that makes an API.
There was a problem hiding this comment.
Yes, exactly. An API, not a set of APIs. You made my point
There was a problem hiding this comment.
Fair enough, fixed.
| OSSL_PARAM *OSSL_PARAM_build(const char *name, ...); | ||
| void OSSL_PARAM_build_free(OSSL_PARAM *params); | ||
|
|
||
| #define OSSL_PARAM_BUILD_int(name, value) \ |
There was a problem hiding this comment.
Hmmm, the manual says "address", not "value". Please decide.
There was a problem hiding this comment.
Name for scalar types, address for strings and BN's.
|
#9139 (comment) made me wonder about use cases. |
|
(maybe I'm not seeing it all clearly... How often do we pass immediate values via ctrl functions today?) |
richsalz
left a comment
There was a problem hiding this comment.
I remain convinced that this is a bad trade-off in complexity/usefulness.
| A collection of utility functions that simplify building arrays of | ||
| OSSL_PARAM structures. The following B<TYPE> names are supported: | ||
|
|
||
| =over 1 |
There was a problem hiding this comment.
if you indent this and just make a block of text it takes less space and looks like C code.
|
|
||
| =item * | ||
|
|
||
| long int (long) |
There was a problem hiding this comment.
why "long int" and not just "long" ?
because "long int" isn't really a supported value for the TYPE is it. :)
And flip the lines so the TYPE appears first, then the native C type in parens.
| Using these functions introduces additional overhead when compared to raw | ||
| OSSL_PARAM operations. | ||
|
|
||
| =over 1 |
There was a problem hiding this comment.
doesn't doc-nits complain on anything other than 4 here?
| OSSL_PARAM_BUILD_utf8_string(), OSSL_PARAM_BUILD_octet_string(), | ||
| OSSL_PARAM_BUILD_BN() are macros that provide support | ||
| for defining UTF8 strings, OCTET strings and big numbers. | ||
| A parameter with B<name> is defined, storage of length B<len> is allocated and |
There was a problem hiding this comment.
Looking at the code, I don't see where any storage is allocated, also because OSSL_PARAM_build_free only free's an array structure.
OH, now I get it, you allocate a single excess space to avoid memory fragmentation. Bleaugh.
|
Closing an unrequited PR. This won't fly and I think I favour @mattcaswell's approach in #9266 (& fleshed out in #9305). |
Add a function that allows an array of OSSL_PARAMs to be allocated and populated in one call.