Hi, so, I finally found time to review this - apologies.
We need a rebase (because of the isatty() change, sorry for that) - and
a v3, though, as I found a few minor things...
On Tue, Aug 11, 2015 at 06:41:49PM +0200, David Sommerseth wrote:
> index d66d408..212a0a8 100644
> --- a/src/openvpn/console.c
> +++ b/src/openvpn/console.c
> @@ -5,7 +5,8 @@
[..]
> -static bool
> -get_console_input_win32 (const char *prompt, const bool echo, char *input,
> const int capacity)
> +void query_user_init(struct gc_arena *gc, struct query_user ** setup)
> {
> - HANDLE in = INVALID_HANDLE_VALUE;
> - HANDLE err = INVALID_HANDLE_VALUE;
> - DWORD len = 0;
> + ASSERT (&gc != NULL);
This needs to be "ASSERT (gc != NULL)" or just "ASSERT (gc)"... &gc will
always be != NULL.
(Theoretically it could just go, as gc_malloc() is safe to be used with
gc being NULL... so it would allocate global memory, but since this is
just called once, even *that* would not be actually harmful)
> + *setup = (struct query_user *) gc_malloc (sizeof(**setup)+2, true, gc);
Why "+2"? This is a structure, it has a well defined size, no
"extra 0 bytes" to be expected.
> + if( unlikely(*setup == NULL) )
> {
gc_malloc() guarantees that the resulting pointer is not NULL, as it
will always call check_malloc_return() and that will exit(1) on NULL
(out_of_memory() in error.c, which is safer than calling msg() in an
out of memory situation anyway)
[..]
> +bool query_user_add(struct query_user * setup,
> + char *prompt, size_t prompt_len,
> + char *resp, size_t resp_len,
> + bool query, bool echo)
> {
[..]
> + /* Save the information needed for the user interaction */
> + p->_gc = setup->_gc;
> + p->prompt = prompt;
> + p->prompt_len = prompt_len;
> + p->response = resp;
> + p->response_len = resp_len;
> + p->query = query;
> + p->echo = echo;
> + query_user_init(p->_gc, &p->next);
[..]
> + return (p->next != NULL) ? true : false;
> }
I don't think this final check makes sense. First of all, no caller
of query_user_add() ever *cares*, and second, it just cannot happen
given that query_user_init() exit()s anyway if malloc fails.
So just make this "void query_user_add(...)" :-)
> diff --git a/src/openvpn/console.h b/src/openvpn/console.h
> index 268f3fe..4d0a57f 100644
> --- a/src/openvpn/console.h
> +++ b/src/openvpn/console.h
> @@ -5,7 +5,8 @@
[..]
> + * Configuration setup for declaring what kind of information to ask a user
> for
> + */
> +struct query_user {
> + struct gc_arena *_gc; /**< Local pointer to the gc_arena maintaining
> this chained list */
> + char *prompt; /**< Prompt to present to the user */
> + size_t prompt_len; /**< Lenght of the prompt string */
> + char *response; /**< The user's response */
> + size_t response_len; /**< Lenght the of the user reposone */
> + bool query; /**< True: Expect user to provide a response,
> False: only print */
> + bool echo; /**< True: The user should see what is being
> typed, otherwise mask it */
> + struct query_user *next; /**< Next information to query for, NULL
> indicates end of list */
> +};
Is there anything in OpenVPN that would print out a message to console
but not prompt for a response? The "query" field adds a bit of carry-on
to structure and code, but all current uses of query_user_add() set it
to "true"...
[..]
> diff --git a/src/openvpn/console_builtin.c b/src/openvpn/console_builtin.c
> new file mode 100644
> index 0000000..b8d6430
> --- /dev/null
> +++ b/src/openvpn/console_builtin.c
> @@ -0,0 +1,246 @@
[..]
> +static bool get_console_input_win32 (const char *prompt, const bool echo,
> char *input, const int capacity)
> +{
[..]
> + if (echo)
> + flags |= ENABLE_ECHO_INPUT;
> + SetConsoleMode (in, flags);
> + } else
> + is_console = 0;
When moving code, please do not change coding style in-progress - this
used to have an "else" on its own line, which makes comparing "has this
just be moved or what was changed" more complicated...
> + free (winput);
> + } else
> + status = ReadFile (in, input, capacity, &len, NULL);
Same here.
[..]
> + * If no alternative implementation is declared, a wrapper in console.h will
> ensure
> + * query_user_exec() will call this function instead.
> + *
> + */
> +bool query_user_exec_builtin(struct query_user * setup)
> +{
> + struct query_user *p;
> + bool ret = true;
> +
> + ASSERT( setup != NULL );
> +
> + /* Loop through the complete query setup and when needed, collect the
> information */
> + for (p = setup; p->next != NULL; p = p->next)
> + {
I wonder why you coose to implement the linked list this way, instead of
the more common "for (p=first; p != NULL; p = p->next)" approach?
Yours will work fine, but will always have fully-empty structure dangling
at the end of the list, serving only to have an "p->next" of NULL... or
am I misreading this?
(Of course the query_user_add() function would need to be changed
accordingly - right now it's symmetric and should work fine, it just
makes me curious what the underlying trade-off is)
> + if( p->query )
> + {
> + /* If the user is expected to provide some data */
> + ret &= get_console_input(p->prompt, p->echo, p->response,
> p->response_len);
> + } else {
> + /* If the prompt should only be presented to the user with no
> further action */
> + msg(M_INFO|M_NOPREFIX, "%s", p->prompt);
> + }
> + }
See above, I'm wondering about the use case of having a mechanism for
"print stuff only" which we then never *use* (... there's way too much
"we might eventually do that!" code in OpenVPN today).
> diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
> index 5627cb9..cc1dc7b 100644
> --- a/src/openvpn/misc.c
> +++ b/src/openvpn/misc.c
> @@ -5,7 +5,8 @@
> * packet encryption, packet authentication, and
> * packet compression.
> *
> - * Copyright (C) 2002-2010 OpenVPN Technologies, Inc. <sa...@openvpn.net>
> + * Copyright (C) 2002-2015 OpenVPN Technologies, Inc. <sa...@openvpn.net>
> + * Copyright (C) 2014-2015 David Sommerseth <dav...@redhat.com>
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License version 2
> @@ -1049,11 +1050,15 @@ get_user_pass_cr (struct user_pass *up,
> */
> if (flags & GET_USER_PASS_NEED_OK)
> {
> + struct query_user *qucfg = NULL;
> struct buffer user_prompt = alloc_buf_gc (128, &gc);
>
> + query_user_init (&gc, &qucfg);
After the change from "return false to M_FATAL", we could keep this more
in line with the rest of the code - which avoids double pointers for
return values - and make it
> + struct query_user *qucfg = query_user_init (&gc);
(adjusting query_user_add() appropriately, of course).
But the way it is works, of course.
> buf_printf (&user_prompt, "NEED-OK|%s|%s:", prefix, up->username);
> + query_user_add (qucfg, BSTR(&user_prompt), BLEN(&user_prompt),
> up->password, USER_PASS_LEN,
> + true, true);
>
> - if (!get_console_input (BSTR (&user_prompt), true, up->password,
> USER_PASS_LEN))
> + if (!query_user_exec (qucfg))
> msg (M_FATAL, "ERROR: could not read %s ok-confirmation from
> stdin", prefix);
... also, all but one of these invocations use the same sequence
query_user_init()
query_user_add() (only a single time)
query_user_exec()
but use quite a bit of "screen real estate" of the three independent steps
- while I value the flexibility (and it's used when querying for usename
*and* password), for single-shot calls, wouldn't a wrapper that just does
all in one go make the code more readable?
So you'd call something like
> + query_user_single_exec (BSTR(&user_prompt), BLEN(&user_prompt),
> up->password, USER_PASS_LEN,
> + true, true);
which would expand to init()/add()/exec() calls inside...
(I'm sure I suggested this already for v1)
> --- a/src/openvpn/pkcs11.c
> +++ b/src/openvpn/pkcs11.c
> @@ -5,7 +5,8 @@
> * packet encryption, packet authentication, and
> * packet compression.
> *
> - * Copyright (C) 2002-2010 OpenVPN Technologies, Inc. <sa...@openvpn.net>
> + * Copyright (C) 2002-2015 OpenVPN Technologies, Inc. <sa...@openvpn.net>
> + * Copyright (C) 2014-2015 David Sommerseth <dav...@redhat.com>
I'm not sure if this is really "our style" - I can see adding a copyright
notice if a module is basically rewritten (console.c, console.h), but for
a 3-line change I find this surprising, as it is not done elsewhere - most
of our modules only have the generic OpenVPN Technologies, just a very
small set carry other copyright notices (and these usually stem from
"we wrote this from scratch" or "this was taken from elsewhere").
Just thinking out loud, though. Not considering this a showstopper.
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License version 2
> @@ -740,6 +741,7 @@ _pkcs11_openvpn_show_pkcs11_ids_pin_prompt (
> const size_t pin_max
> ) {
> struct gc_arena gc = gc_new ();
> + struct query_user *qucfg = NULL;
> struct buffer pass_prompt = alloc_buf_gc (128, &gc);
>
> (void)global_data;
> @@ -748,10 +750,14 @@ _pkcs11_openvpn_show_pkcs11_ids_pin_prompt (
>
> ASSERT (token!=NULL);
>
> + query_user_init (&gc, &qucfg);
> buf_printf (&pass_prompt, "Please enter '%s' token PIN or 'cancel': ",
> token->display);
> + query_user_add(qucfg, BSTR(&pass_prompt), BLEN(&pass_prompt),
> + pin, pin_max, true, false);
>
> - if (!get_console_input (BSTR (&pass_prompt), false, pin, pin_max)) {
> - msg (M_FATAL, "Cannot read password from stdin");
> + if (!query_user_exec (qucfg))
> + {
> + msg (M_FATAL, "Could not retrieve the PIN");
> }
...this would be another good candidate for "query_user_single_exec()"... :-)
So - in principle feature-ACK, but I think the code can be improved
(and it needs a rebase due to the changes in console.c and misc.c anyway
- please do not forget to move the isatty() bits to console_builtin.c,
git might not be smart enough to get *that* right. But maybe it is...)
gert
--
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025 g...@net.informatik.tu-muenchen.de
signature.asc
Description: PGP signature
