[Openvpn-devel,2/3] Refactor counting number of element in a : delimited list into function

Message ID 20200401102159.29157-1-arne@rfc2549.org
State Superseded
Headers show
Series
  • [Openvpn-devel] Fix OpenSSL 1.1.1 not using auto ecliptic curve selection
Related show

Commit Message

Arne Schwabe April 1, 2020, 10:21 a.m.
---
 src/openvpn/misc.c        | 18 ++++++++++++++++++
 src/openvpn/misc.h        | 13 +++++++++++++
 src/openvpn/ssl_mbedtls.c | 15 ++-------------
 3 files changed, 33 insertions(+), 13 deletions(-)

Comments

Antonio Quartulli April 15, 2020, 12:05 p.m. | #1
Hi,

On 01/04/2020 12:21, Arne Schwabe wrote:
> ---
>  src/openvpn/misc.c        | 18 ++++++++++++++++++
>  src/openvpn/misc.h        | 13 +++++++++++++
>  src/openvpn/ssl_mbedtls.c | 15 ++-------------
>  3 files changed, 33 insertions(+), 13 deletions(-)
> 
> diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
> index 1931149b..b375451f 100644
> --- a/src/openvpn/misc.c
> +++ b/src/openvpn/misc.c
> @@ -735,4 +735,22 @@ output_peer_info_env(struct env_set *es, const char *peer_info)
>      }
>  }
>  
> +int get_num_elements(const char* string, char delimiter)
> +{
> +  int string_len = strlen(string);
> +
> +  ASSERT(0 != string_len);
> +
> +  int element_count = 1;
> +  /* Get number of ciphers */
> +  for (int i = 0; i < string_len; i++)
> +    {
> +      if (string[i] == delimiter)
> +        {
> +          element_count++;
> +        }
> +    }

while copying this code you are breaking the indentation. note that 2
blanks before the curly brackets. that's nt supposed to be there.


> +
> +  return element_count;
> +}
>  #endif /* P2MP_SERVER */
> diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h
> index 991b7df2..0655b7fe 100644
> --- a/src/openvpn/misc.h
> +++ b/src/openvpn/misc.h
> @@ -175,4 +175,17 @@ void output_peer_info_env(struct env_set *es, const char *peer_info);
>  
>  #endif /* P2MP_SERVER */
>  
> +/**
> + * Counts the number of delimiter in a string and returns
> + * their number +1. 

I'd rephrase this sentence as:

Returns the occurrences of 'delimiter' in 'string' +1.

> This is typically used to find out the
> + * number elements in a cipher string or similar that is separated by : like
> + *
> + *   X25519:secp256r1:X448:secp512r1:secp384r1:brainpoolP384r1
> + *
> + * @param string        the string to work on
> + * @param delimiter     the delimiter to count, typically ':'
> + * @return              number of delimiter found + 1

      I'd change to "occurrences of 'delimiter' +1"

> + */
> +int
> +get_num_elements(const char* string, char delimiter);
>  #endif /* ifndef MISC_H */
> diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
> index 0f0b035b..0e17e734 100644
> --- a/src/openvpn/ssl_mbedtls.c
> +++ b/src/openvpn/ssl_mbedtls.c
> @@ -289,33 +289,22 @@ void
>  tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
>  {
>      char *tmp_ciphers, *tmp_ciphers_orig, *token;
> -    int i, cipher_count;
> -    int ciphers_len;
>  
>      if (NULL == ciphers)
>      {
>          return; /* Nothing to do */
> -
>      }
> -    ciphers_len = strlen(ciphers);
>  
>      ASSERT(NULL != ctx);
> -    ASSERT(0 != ciphers_len);
>  
>      /* Get number of ciphers */
> -    for (i = 0, cipher_count = 1; i < ciphers_len; i++)
> -    {
> -        if (ciphers[i] == ':')
> -        {
> -            cipher_count++;
> -        }
> -    }
> +    int cipher_count = get_num_elements(ciphers, ':');
>  
>      /* Allocate an array for them */
>      ALLOC_ARRAY_CLEAR(ctx->allowed_ciphers, int, cipher_count+1)
>  
>      /* Parse allowed ciphers, getting IDs */
> -    i = 0;
> +    int i = 0;
>      tmp_ciphers_orig = tmp_ciphers = string_alloc(ciphers, NULL);
>  
>      token = strtok(tmp_ciphers, ":");
> 

Other than my little nitpicks above, the patch looks good.
However, I have a question.

Since you are refactoring this code and this is going to master/2.5, why
not reimplementing the get_num_elements() function using strtok() ?


Regards,
Arne Schwabe April 16, 2020, 2:46 p.m. | #2
>>
> 
> Other than my little nitpicks above, the patch looks good.
> However, I have a question.
> 
> Since you are refactoring this code and this is going to master/2.5, why
> not reimplementing the get_num_elements() function using strtok() ?
>

strsep/strok have the disadvantage of modifying the passed string,
requring copying the string. strok in addition breaks on recursive calls.

Also the for loop is very to understand and from playing with godbolt
(https://godbolt.org/z/RVtjVL) it looks that the compilers will
optimise/vectorise this function so using C string function will
probably be even slower. So the simple and easy to understand function
is the best choice here IMHO.

Will send a V2 with the formatting/description fixed.

Arne

Patch

diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 1931149b..b375451f 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -735,4 +735,22 @@  output_peer_info_env(struct env_set *es, const char *peer_info)
     }
 }
 
+int get_num_elements(const char* string, char delimiter)
+{
+  int string_len = strlen(string);
+
+  ASSERT(0 != string_len);
+
+  int element_count = 1;
+  /* Get number of ciphers */
+  for (int i = 0; i < string_len; i++)
+    {
+      if (string[i] == delimiter)
+        {
+          element_count++;
+        }
+    }
+
+  return element_count;
+}
 #endif /* P2MP_SERVER */
diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h
index 991b7df2..0655b7fe 100644
--- a/src/openvpn/misc.h
+++ b/src/openvpn/misc.h
@@ -175,4 +175,17 @@  void output_peer_info_env(struct env_set *es, const char *peer_info);
 
 #endif /* P2MP_SERVER */
 
+/**
+ * Counts the number of delimiter in a string and returns
+ * their number +1. This is typically used to find out the
+ * number elements in a cipher string or similar that is separated by : like
+ *
+ *   X25519:secp256r1:X448:secp512r1:secp384r1:brainpoolP384r1
+ *
+ * @param string        the string to work on
+ * @param delimiter     the delimiter to count, typically ':'
+ * @return              number of delimiter found + 1
+ */
+int
+get_num_elements(const char* string, char delimiter);
 #endif /* ifndef MISC_H */
diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
index 0f0b035b..0e17e734 100644
--- a/src/openvpn/ssl_mbedtls.c
+++ b/src/openvpn/ssl_mbedtls.c
@@ -289,33 +289,22 @@  void
 tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
 {
     char *tmp_ciphers, *tmp_ciphers_orig, *token;
-    int i, cipher_count;
-    int ciphers_len;
 
     if (NULL == ciphers)
     {
         return; /* Nothing to do */
-
     }
-    ciphers_len = strlen(ciphers);
 
     ASSERT(NULL != ctx);
-    ASSERT(0 != ciphers_len);
 
     /* Get number of ciphers */
-    for (i = 0, cipher_count = 1; i < ciphers_len; i++)
-    {
-        if (ciphers[i] == ':')
-        {
-            cipher_count++;
-        }
-    }
+    int cipher_count = get_num_elements(ciphers, ':');
 
     /* Allocate an array for them */
     ALLOC_ARRAY_CLEAR(ctx->allowed_ciphers, int, cipher_count+1)
 
     /* Parse allowed ciphers, getting IDs */
-    i = 0;
+    int i = 0;
     tmp_ciphers_orig = tmp_ciphers = string_alloc(ciphers, NULL);
 
     token = strtok(tmp_ciphers, ":");