Re: [Openvpn-devel] [PATCH v2] Move adjust_power_of_2() to integer.h

[Emmanuel Deloget]

On Thu, Jun 22, 2017 at 6:08 PM, Antonio Quartulli  wrote:
>
> On Thu, Jun 22, 2017 at 05:33:44PM +0200, Emmanuel Deloget wrote:
> > Hi Antonio, Steffan,
> >
> > On Thu, Jun 22, 2017 at 3:31 PM, Antonio Quartulli  wrote:
> >
> > > Thanks for sending v2 Steffan,
> > >
> > > On Wed, Jun 21, 2017 at 11:10:43PM +0200, Steffan Karger wrote:
> > > > From: Steffan Karger 
> > > >
> > > > 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 
> > >
> > >
> > > This change is quite simple and passes basic tests.
> > > Thanks also for adding explicit #include statements.
> > >
> > > Acked-by: Antonio Quartulli 
> > >
> > >
> > 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.

Fair enough :)

> >
> > 
> >
> > 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.
>

I'll wait until I see Steffan's patch on master . But as you said,
/coda obscura est/ so the commit message will make that very clear.

>
> Cheers,
> --
> Antonio Quartulli

BR,

-- Emmanuel Deloget

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v2] Move adjust_power_of_2() to integer.h

[Antonio Quartulli]

On Thu, Jun 22, 2017 at 05:33:44PM +0200, Emmanuel Deloget wrote:
> Hi Antonio, Steffan,
> 
> On Thu, Jun 22, 2017 at 3:31 PM, Antonio Quartulli  wrote:
> 
> > Thanks for sending v2 Steffan,
> >
> > On Wed, Jun 21, 2017 at 11:10:43PM +0200, Steffan Karger wrote:
> > > From: Steffan Karger 
> > >
> > > 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 
> >
> >
> > This change is quite simple and passes basic tests.
> > Thanks also for adding explicit #include statements.
> >
> > Acked-by: Antonio Quartulli 
> >
> >
> 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.

> 
> 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 :)

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


signature.asc
Description: Digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v2] Move adjust_power_of_2() to integer.h

[Antonio Quartulli]

Thanks for sending v2 Steffan,

On Wed, Jun 21, 2017 at 11:10:43PM +0200, Steffan Karger wrote:
> From: Steffan Karger 
> 
> 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 


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

Acked-by: Antonio Quartulli 

Cheers,

-- 
Antonio Quartulli


signature.asc
Description: Digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH v2] Move adjust_power_of_2() to integer.h

[Steffan Karger]

From: Steffan Karger 

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 
---
v2: fix includes, fix typo in commit msg

 src/openvpn/argv.c   |  1 +
 src/openvpn/integer.h| 18 ++
 src/openvpn/list.c   |  1 +
 src/openvpn/mbuf.c   |  1 +
 src/openvpn/misc.c   | 18 --
 src/openvpn/misc.h   |  2 --
 tests/unit_tests/openvpn/test_argv.c | 18 --
 7 files changed, 21 insertions(+), 38 deletions(-)

diff --git a/src/openvpn/argv.c b/src/openvpn/argv.c
index a71d261c..95bdfeac 100644
--- a/src/openvpn/argv.c
+++ b/src/openvpn/argv.c
@@ -36,6 +36,7 @@
 #include "syshead.h"
 
 #include "argv.h"
+#include "integer.h"
 #include "options.h"
 
 static void
diff --git a/src/openvpn/integer.h b/src/openvpn/integer.h
index 884b849f..600e1384 100644
--- a/src/openvpn/integer.h
+++ b/src/openvpn/integer.h
@@ -114,6 +114,24 @@ 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)
 {
diff --git a/src/openvpn/list.c b/src/openvpn/list.c
index edca6f79..91765d20 100644
--- a/src/openvpn/list.c
+++ b/src/openvpn/list.c
@@ -31,6 +31,7 @@
 
 #if P2MP_SERVER
 
+#include "integer.h"
 #include "list.h"
 #include "misc.h"
 
diff --git a/src/openvpn/mbuf.c b/src/openvpn/mbuf.c
index fafbce01..f969a2b5 100644
--- a/src/openvpn/mbuf.c
+++ b/src/openvpn/mbuf.c
@@ -33,6 +33,7 @@
 
 #include "buffer.h"
 #include "error.h"
+#include "integer.h"
 #include "misc.h"
 #include "mbuf.h"
 
diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 608204ee..0b65e6e2 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -1646,24 +1646,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.
  */
diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h
index d504e68a..c9fa97ab 100644
--- a/src/openvpn/misc.h
+++ b/src/openvpn/misc.h
@@ -328,8 +328,6 @@ extern const char *iproute_path;
 #define SSEC_PW_ENV3 /* 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 */
diff --git a/tests/unit_tests/openvpn/test_argv.c 
b/tests/unit_tests/openvpn/test_argv.c
index 8c90eb9c..4a3ba559 100644
--- a/tests/unit_tests/openvpn/test_argv.c
+++ b/tests/unit_tests/openvpn/test_argv.c
@@ -13,24 +13,6 @@
 #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;
-
-while (ret < u)
-{
-ret <<= 1;
-assert(ret > 0);
-}
-
-return ret;
-}
-
 /* Defines for use in the tests and the mock parse_line() */
 #define PATH1   "/s p a c e"
 #define PATH2   "/foo bar/baz"
-- 
2.11.0


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel