Attention is currently required from: plaisthos.
Hello plaisthos,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/1151?usp=email
to review the following change.
Change subject: options: Introduce boolean_flag() and review usages of atoi_warn
......................................................................
options: Introduce boolean_flag() and review usages of atoi_warn
This uses atoi_constrained. Note that this makes the tests
stricter since previously any non-zero integer would be
interpreted as "true".
Change-Id: Ic30612971367a4aa54ca89f93452efc172d4f4e2
Signed-off-by: Frank Lichtenheld <fr...@lichtenheld.com>
---
M src/openvpn/options.c
M src/openvpn/options_util.c
M src/openvpn/options_util.h
M tests/unit_tests/openvpn/test_misc.c
4 files changed, 38 insertions(+), 11 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/51/1151/1
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index b62e22c..46f4753 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -8358,7 +8358,7 @@
{
VERIFY_PERMISSION(OPT_P_GENERAL);
options->sc_info.challenge_text = p[1];
- if (atoi_warn(p[2], msglevel))
+ if (boolean_flag(p[2], "static-challenge echo", msglevel))
{
options->sc_info.flags |= SC_ECHO;
}
@@ -8721,7 +8721,7 @@
options->exit_event_name = p[1];
if (p[2])
{
- options->exit_event_initial_state = (atoi_warn(p[2], msglevel) !=
0);
+ options->exit_event_initial_state = boolean_flag(p[2], "service
state", msglevel);
}
}
else if (streq(p[0], "allow-nonadmin") && !p[2])
@@ -9682,7 +9682,8 @@
else if (streq(p[0], "show-pkcs11-ids") && !p[3])
{
char *provider = p[1];
- bool cert_private = (p[2] == NULL ? false : (atoi_warn(p[2], msglevel)
!= 0));
+ bool cert_private = (p[2] == NULL ? false
+ : boolean_flag(p[2],
"show-pkcs11-ids private", msglevel));
#ifdef DEFAULT_PKCS11_MODULE
if (!provider)
@@ -9728,14 +9729,12 @@
}
else if (streq(p[0], "pkcs11-protected-authentication"))
{
- int j;
-
VERIFY_PERMISSION(OPT_P_GENERAL);
- for (j = 1; j < MAX_PARMS && p[j] != NULL; ++j)
+ for (int j = 1; j < MAX_PARMS && p[j] != NULL; ++j)
{
options->pkcs11_protected_authentication[j - 1] =
- atoi_warn(p[j], msglevel) != 0 ? 1 : 0;
+ boolean_flag(p[j], p[0], msglevel);
}
}
else if (streq(p[0], "pkcs11-private-mode") && p[1])
@@ -9751,13 +9750,11 @@
}
else if (streq(p[0], "pkcs11-cert-private"))
{
- int j;
-
VERIFY_PERMISSION(OPT_P_GENERAL);
- for (j = 1; j < MAX_PARMS && p[j] != NULL; ++j)
+ for (int j = 1; j < MAX_PARMS && p[j] != NULL; ++j)
{
- options->pkcs11_cert_private[j - 1] = (bool)(atoi_warn(p[j],
msglevel));
+ options->pkcs11_cert_private[j - 1] = boolean_flag(p[j], p[0],
msglevel);
}
}
else if (streq(p[0], "pkcs11-pin-cache") && p[1] && !p[2])
diff --git a/src/openvpn/options_util.c b/src/openvpn/options_util.c
index c1f7c74..32a9edb 100644
--- a/src/openvpn/options_util.c
+++ b/src/openvpn/options_util.c
@@ -181,6 +181,14 @@
return true;
}
+bool
+boolean_flag(const char *str, const char *name, int msglevel)
+{
+ int number = 0;
+ atoi_constrained(str, &number, name, 0, 1, msglevel);
+ return (bool)number;
+}
+
static const char *updatable_options[] = { "block-ipv6", "block-outside-dns",
"dhcp-option", "dns",
"ifconfig", "ifconfig-ipv6",
diff --git a/src/openvpn/options_util.h b/src/openvpn/options_util.h
index d5ae8b9..b9e1569 100644
--- a/src/openvpn/options_util.h
+++ b/src/openvpn/options_util.h
@@ -59,6 +59,12 @@
int msglevel);
/**
+ * Converts a str to an boolean if the string can be parsed as either 0 or 1.
+ * Otherwise print a warning with \p msglevel and return \c false.
+ */
+bool boolean_flag(const char *str, const char *name, int msglevel);
+
+/**
* Filter an option line by all pull filters.
*
* If a match is found, the line is modified depending on
diff --git a/tests/unit_tests/openvpn/test_misc.c
b/tests/unit_tests/openvpn/test_misc.c
index 80a5dee..2d2cc9e 100644
--- a/tests/unit_tests/openvpn/test_misc.c
+++ b/tests/unit_tests/openvpn/test_misc.c
@@ -431,6 +431,22 @@
assert_string_equal(mock_msg_buf, "test: Must be an integer >= 1, not 0");
assert_int_equal(parameter, -42);
+ /* special tests for boolean_flag */
+ assert_false(boolean_flag("0", "test", msglevel));
+ assert_true(boolean_flag("1", "test", msglevel));
+
+ CLEAR(mock_msg_buf);
+ assert_false(boolean_flag("foo77", "test", msglevel));
+ assert_string_equal(mock_msg_buf, "test: Cannot parse 'foo77' as integer");
+
+ CLEAR(mock_msg_buf);
+ assert_false(boolean_flag("-77", "test", msglevel));
+ assert_string_equal(mock_msg_buf, "test: Must be an integer between 0 and
1, not -77");
+
+ CLEAR(mock_msg_buf);
+ assert_false(boolean_flag("77", "test", msglevel));
+ assert_string_equal(mock_msg_buf, "test: Must be an integer between 0 and
1, not 77");
+
mock_set_debug_level(saved_log_level);
}
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1151?usp=email
To unsubscribe, or for help writing mail filters, visit
http://gerrit.openvpn.net/settings
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ic30612971367a4aa54ca89f93452efc172d4f4e2
Gerrit-Change-Number: 1151
Gerrit-PatchSet: 1
Gerrit-Owner: flichtenheld <fr...@lichtenheld.com>
Gerrit-Reviewer: plaisthos <arne-open...@rfc2549.org>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-open...@rfc2549.org>
Gerrit-MessageType: newchange
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
