Hi, On Thu, Sep 30, 2021 at 02:33:08PM +0300, Petr Mikhalicin via Openvpn-devel wrote: > New pkcs11-helper interface allows to setup pkcs11 provider via > properties: > https://github.com/alonbl/pkcs11-helper/commit/b78d21c7e26041746aa4ae3d08b95469e1714a85 > > Also pkcs11-helper added ability to setup init args for pkcs11 provider: > https://github.com/alonbl/pkcs11-helper/commit/133f893e30856eba1de715ecd6fe176722eb3097
I can't comment on the PKCS#11 feature (not my field), but I have a few
comments about required coding style changes:
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -664,6 +664,11 @@ static const char usage_message[] =
> " 8 : Use Unwrap.\n"
> "--pkcs11-cert-private [0|1] ... : Set if login should be performed
> before\n"
> " certificate can be accessed. Set for
> each provider.\n"
> + "--pkcs11-init-flags hex ... : PKCS#11 init flags.\n"
> + " It's bitwise OR of some PKCS#11
> initialize flags.\n"
> + " Most popular of them is:\n"
> + " 1 :
> CKF_LIBRARY_CANT_CREATE_OS_THREADS\n"
> + " 2 : CKF_OS_LOCKING_OK\n"
The indent here is not right - did you use TABs here? Please don't, they
get usually messed up by mail clients.
> @@ -1838,6 +1843,13 @@ show_settings(const struct options *o)
> SHOW_PARM(pkcs11_cert_private, o->pkcs11_cert_private[i] ?
> "ENABLED" : "DISABLED", "%s");
> }
> }
> + {
> + int i;
> + for (i = 0; i<MAX_PARMS; i++)
> + {
> + SHOW_PARM(pkcs11_init_flags, o->pkcs11_init_flags[i], "%08x");
> + }
> + }
This, we do C99 style nowadays:
> + for (int i=0; i<MAX_PARMS; i++)
> + {
> + SHOW_PARM(pkcs11_init_flags, o->pkcs11_init_flags[i], "%08x");
> + }
(so, no extra brackets, and the "int i" can go right into the for()
clause)
> SHOW_INT(pkcs11_pin_cache_period);
> SHOW_STR(pkcs11_id);
> SHOW_BOOL(pkcs11_id_management);
> @@ -8778,6 +8790,17 @@ add_option(struct options *options,
> options->pkcs11_cert_private[j-1] = atoi(p[j]) != 0 ? 1 : 0;
> }
> }
> + else if (streq(p[0], "pkcs11-init-flags"))
> + {
> + int j;
> +
> + VERIFY_PERMISSION(OPT_P_GENERAL);
> +
> + for (j = 1; j < MAX_PARMS && p[j] != NULL; ++j)
Same here: "int j" goes into the loop.
> diff --git a/src/openvpn/pkcs11.c b/src/openvpn/pkcs11.c
> index 02d0f51f..29db7ea4 100644
> --- a/src/openvpn/pkcs11.c
> +++ b/src/openvpn/pkcs11.c
> @@ -374,12 +374,17 @@ pkcs11_terminate(void)
> + if ((rv = pkcs11h_registerProvider(provider)) != CKR_OK) {
> + msg(M_WARN, "PKCS#11: Cannot register provider '%s' %ld-'%s'",
> provider, rv, pkcs11h_getMessage(rv));
> + success = false;
> + goto exit;
> + }
The "{" always goes to the next line, and indenting is never done with
tabs (the lines above look like a mixture of tabs and spaces, and the
tab being messed up by the mail client).
> + // pkcs11-helper take ownership over this pointer
No C++ comments, please.
> + // pkcs11-helper take ownership over this pointer
> + if ((p_init_args = malloc(sizeof(*p_init_args))) == NULL) {
> + msg(M_FATAL, "PKCS#11: Cannot allocate memory");
> + success = false;
> + goto cleanup;
> + }
> +
> + memset(p_init_args, 0, sizeof(*p_init_args));
Please use calloc() and check_malloc_return() instead.
msg(M_FATAL) never returns, so the "success = false, goto cleanup" bit
is not needed - and all that is done by check_malloc_return() for you :-)
For our coding style guidelines, see also here:
https://community.openvpn.net/openvpn/wiki/CodeStyle
and in the openvpn repo there is a "dev-tools/uncrustify.conf" config
which can be used with the "uncrustify" program to format your code
according to the whitespace rules. Won't do the "for (int i=0; ...)"
C99 changes, though.
gert
--
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany g...@greenie.muc.de
signature.asc
Description: PGP signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
