Move adjust_power_of_2() to integer.h

misc.c it a mess of incoherent functions, and is therefore included by

virtually all our source files. That makes testing harder than it should be. As a first step of cleaning up misc.c, move adjust_power_of_2() to integer.h, which is a more suitable place for a function like this.

This allows us to remove the duplicate implementation from test_argv.c.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/integer.h                | 17 +++++++++++++++++
 src/openvpn/misc.c                   | 18 ------------------
 src/openvpn/misc.h                   |  2 --
 tests/unit_tests/openvpn/test_argv.c | 19 +------------------
 4 files changed, 18 insertions(+), 38 deletions(-)

src/openvpn/integer.h +17 -0

index 240781b..882322a 100644
@@ -118,6 +118,23 @@ modulo_add(int x, int y, int mod)
     return sum;
 }
 
+/*
+ * Return the next largest power of 2
+ * or u if u is a power of 2.
+ */
+static inline size_t adjust_power_of_2(size_t u)
+{
+    size_t ret = 1;
+
+    while (ret < u)
+    {
+        ret <<= 1;
+        ASSERT(ret > 0);
+    }
+
+    return ret;
+}
+
 static inline int
 index_verify(int index, int size, const char *file, int line)
 {

src/openvpn/misc.c +0 -18

index df108b0..561f80f 100644
@@ -1641,24 +1641,6 @@ openvpn_sleep(const int n)
 }
 
 /*
- * Return the next largest power of 2
- * or u if u is a power of 2.
- */
-size_t
-adjust_power_of_2(size_t u)
-{
-    size_t ret = 1;
-
-    while (ret < u)
-    {
-        ret <<= 1;
-        ASSERT(ret > 0);
-    }
-
-    return ret;
-}
-
-/*
  * Remove security-sensitive strings from control message
  * so that they will not be output to log file.
  */

src/openvpn/misc.h +0 -2

index 94573b2..f55186f 100644
@@ -325,8 +325,6 @@ extern const char *iproute_path;
 #define SSEC_PW_ENV    3 /* allow calling of built-in programs and user-defined scripts that may receive a password as an environmental variable */
 extern int script_security; /* GLOBAL */
 
-/* return the next largest power of 2 */
-size_t adjust_power_of_2(size_t u);
 
 #define COMPAT_FLAG_QUERY         0       /** compat_flags operator: Query for a flag */
 #define COMPAT_FLAG_SET           (1<<0)  /** compat_flags operator: Set a compat flag */

tests/unit_tests/openvpn/test_argv.c +0 -9

index 8c90eb9..c5ebc09 100644
@@ -12,24 +12,7 @@
 
 #include "argv.h"
 #include "buffer.h"
-
-/*
- * This is defined here to prevent #include'ing misc.h
- * which makes things difficult beyond any recognition
- */
-size_t
-adjust_power_of_2(size_t u)
-{
-    size_t ret = 1;

tests/unit_tests/openvpn/test_argv.c +0 -0

 /* Defines for use in the tests and the mock parse_line() */
 #define PATH1       "/s p a c e"

Signature · 1 line

2.7.4

However, now that adjust_power_of_2() has been moved to integer.h, shouldn't this file be included by every .c where the function is used? Or do we have some other rule about header files inclusion?

Personally I prefer when every .c file directly includes the .h it relies on, without assuming any indirect inclusion. What's the common habit here?

From: Steffan Karger <[email protected]...>

misc.c is a mess of incoherent functions, and is therefore included by virtually all our source files. That makes testing harder than it should be. As a first step of cleaning up misc.c, move adjust_power_of_2() to integer.h, which is a more suitable place for a function like this.

This allows us to remove the duplicate implementation from test_argv.c.

Signed-off-by Steffan Karger <[email protected]...>

Thanks for sending v2 Steffan,

Quoted 9 lines · Steffan Karger

From: Steffan Karger <[email protected]...>

misc.c is a mess of incoherent functions, and is therefore included by virtually all our source files. That makes testing harder than it should be. As a first step of cleaning up misc.c, move adjust_power_of_2() to integer.h, which is a more suitable place for a function like this.

This allows us to remove the duplicate implementation from test_argv.c.

Quoted 1 line

Signed-off-by Steffan Karger <[email protected]...>

This change is quite simple and passes basic tests. Thanks also for adding explicit #include statements.

Acked-by Antonio Quartulli <[email protected]...>

Cheers,
​​

Hi Antonio, Steffan,

Quoted 18 lines · Antonio Quartulli

Thanks for sending v2 Steffan,

Quoted 10 lines · Steffan Karger

From: Steffan Karger <[email protected]...>

misc.c is a mess of incoherent functions, and is therefore included by virtually all our source files. That makes testing harder than it should be. As a first step of cleaning up misc.c, move adjust_power_of_2() to integer.h, which is a more suitable place for a function like this.

This allows us to remove the duplicate implementation from test_argv.c.

Signed-off-by Steffan Karger <[email protected]...>

This change is quite simple and passes basic tests. Thanks also for adding explicit #include statements.

Quoted 3 lines

Acked-by Antonio Quartulli <[email protected]...>

Since the code is moved, would it be interesting to use a less greedy algorithm to do the same thing? I know the function is not used very often, so it does not look like it's necessary (it's a suggestion, not a commentary on your patch Steffan :))

For example, this code:

size_t
adjust_power_of_2(size_t v)
{
if (!v)
return 1;
v--;

v |= v >> 1;
v |= v >> 2;
v |= v >> 4;
v |= v >> 8;
v |= v >> 16;

if (sizeof(size_t) == 8)
v |= v >> 32;
v++;

return v;
}

does the exact same work on a 64 size_t but will be far faster than the loop-based one on many situation (the sizeof() test should be optimized away if it's never true, so the code will work even with a 32 bit size_t).

Why it works:
* if (v - 1) has 0 bits set, then v = 1 and the function will return 1 ((0 | 0 | ... | 0) + 1)
* if (v - 1) has at least one bit set (let's say it's bit n), then the first iteration will set bit (n-1). The next iteration will set bits (n-2) and (n-3). We goes on for a few steps. Before the last iteration, v has bits (n...(n-32)) set (with sizeof(size_t) == 8), so the very last operation fills the remaining bits and v has bits (n...0) set. Adding one to this value will yield the desired result.

Again, the function is not used extensively, so you can ignore this suggestion as it will not speed up openvpn at all :)

Quoted 22 lines · Emmanuel Deloget

Hi Antonio, Steffan,

Quoted 18 lines · Antonio Quartulli

Thanks for sending v2 Steffan,

Quoted 10 lines · Steffan Karger

From: Steffan Karger <[email protected]...>

misc.c is a mess of incoherent functions, and is therefore included by virtually all our source files. That makes testing harder than it should be. As a first step of cleaning up misc.c, move adjust_power_of_2() to integer.h, which is a more suitable place for a function like this.

This allows us to remove the duplicate implementation from test_argv.c.

Signed-off-by Steffan Karger <[email protected]...>

This change is quite simple and passes basic tests. Thanks also for adding explicit #include statements.

Quoted 7 lines

Quoted 3 lines

Acked-by Antonio Quartulli <[email protected]...>

Since the code is moved, would it be interesting to use a less greedy algorithm to do the same thing? I know the function is not used very often, so it does not look like it's necessary (it's a suggestion, not a commentary on your patch Steffan :))

My personal opinion: *never* mix code refactoring with functional changes (like what you are suggesting).

Therefore we'd still need another patch.

Quoted 5 lines

<snip>

Again, the function is not used extensively, so you can ignore this suggestion as it will not speed up openvpn at all :)

yeah...not sure we need to make some code a bit "obscure" if there is really no need.

Still, I'd suggest to prepare a second patch and propose it over the list.

Cheers,
--
Antonio Quartulli

yeah...not sure we need to make some code a bit "obscure" if there is really no need.