Re: [Openvpn-devel] [PATCH 2/4] Added PIP_OPT_MASK for process_ip_header fast exit path.

[James Yonan]

On 23/04/2014 04:17, Arne Schwabe wrote:

Am 21.04.14 21:26, schrieb James Yonan:

On 21/04/2014 05:27, Arne Schwabe wrote:

On 21.04.2014 09:10, James Yonan wrote:

Define PIP_OPT_MASK to represent all flags of interest to
process_ip_header, so that it can have a fast exit path
if no flags are set.


I haven't look at the code but if remember  correctly, this method does
not get passed the actual flags but the flags should *potientially* be
checked.


Yes, the point is to aggregate the potential flags of interest to
process_ip_header under a single mask, i.e. PIP_OPT_MASK.

So instead of having "if (flags & (PIPV4_PASSTOS|PIP_MSSFIX|...|...))"
you just have "if (flags & PIP_OPT_MASK)".

The main issue for us is that we have an internal patch for one of our
services that adds an additional flag, and the previous usage is a
merge conflict attractor because all the flags of interest are
explicitly given on a single line.


I only wanted to point out my patch also exits. I looked at the code again.

I think to really get rid of the #ifdef here we also need to
unconditionally enable the other &= ~ statements:

Change

#if PASSTOS_CAPABILITY
   if (!c->options.passtos)
 flags &= ~PIPV4_PASSTOS;
#endif

to

#if PASSTOS_CAPABILITY
   if (!c->options.passtos)
#endif
 flags &= ~PIPV4_PASSTOS;


Otherwise if the function is called with PIPV4_PASSTOS but not compiled
with PASSTOS_CAPABILITY the

flags & PIP_OPT_MASK will be always true.

Arne


Sure, that makes sense.

James

Re: [Openvpn-devel] [PATCH 2/4] Added PIP_OPT_MASK for process_ip_header fast exit path.

[Arne Schwabe]

Am 21.04.14 21:26, schrieb James Yonan:
> On 21/04/2014 05:27, Arne Schwabe wrote:
>> On 21.04.2014 09:10, James Yonan wrote:
>>> Define PIP_OPT_MASK to represent all flags of interest to
>>> process_ip_header, so that it can have a fast exit path
>>> if no flags are set.
>>
>> I haven't look at the code but if remember  correctly, this method does
>> not get passed the actual flags but the flags should *potientially* be
>> checked.
>
> Yes, the point is to aggregate the potential flags of interest to
> process_ip_header under a single mask, i.e. PIP_OPT_MASK.
>
> So instead of having "if (flags & (PIPV4_PASSTOS|PIP_MSSFIX|...|...))"
> you just have "if (flags & PIP_OPT_MASK)".
>
> The main issue for us is that we have an internal patch for one of our
> services that adds an additional flag, and the previous usage is a
> merge conflict attractor because all the flags of interest are
> explicitly given on a single line.
>
I only wanted to point out my patch also exits. I looked at the code again.

I think to really get rid of the #ifdef here we also need to
unconditionally enable the other &= ~ statements:

Change

#if PASSTOS_CAPABILITY
  if (!c->options.passtos)
flags &= ~PIPV4_PASSTOS;
#endif

to

#if PASSTOS_CAPABILITY
  if (!c->options.passtos)
#endif
flags &= ~PIPV4_PASSTOS;


Otherwise if the function is called with PIPV4_PASSTOS but not compiled
with PASSTOS_CAPABILITY the

flags & PIP_OPT_MASK will be always true.

Arne







signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] [PATCH 2/4] Added PIP_OPT_MASK for process_ip_header fast exit path.

[James Yonan]

On 21/04/2014 05:27, Arne Schwabe wrote:

On 21.04.2014 09:10, James Yonan wrote:

Define PIP_OPT_MASK to represent all flags of interest to
process_ip_header, so that it can have a fast exit path
if no flags are set.


I haven't look at the code but if remember  correctly, this method does
not get passed the actual flags but the flags should *potientially* be
checked.


Yes, the point is to aggregate the potential flags of interest to 
process_ip_header under a single mask, i.e. PIP_OPT_MASK.


So instead of having "if (flags & (PIPV4_PASSTOS|PIP_MSSFIX|...|...))" 
you just have "if (flags & PIP_OPT_MASK)".


The main issue for us is that we have an internal patch for one of our 
services that adds an additional flag, and the previous usage is a merge 
conflict attractor because all the flags of interest are explicitly 
given on a single line.



See also my never reviewed patch:
http://thread.gmane.org/gmane.network.openvpn.devel/7532


Your patch would be simplified with this approach because you don't need 
to add the test for PIPV4_CLIENT_NAT to "if (flags & ...)".


James

Re: [Openvpn-devel] [PATCH 2/4] Added PIP_OPT_MASK for process_ip_header fast exit path.

[Arne Schwabe]

On 21.04.2014 09:10, James Yonan wrote:

Define PIP_OPT_MASK to represent all flags of interest to
process_ip_header, so that it can have a fast exit path
if no flags are set.


I haven't look at the code but if remember  correctly, this method does 
not get passed the actual flags but the flags should *potientially* be 
checked.


See also my never reviewed patch: 
http://thread.gmane.org/gmane.network.openvpn.devel/7532


Arne

[Openvpn-devel] [PATCH 2/4] Added PIP_OPT_MASK for process_ip_header fast exit path.

[James Yonan]

Define PIP_OPT_MASK to represent all flags of interest to
process_ip_header, so that it can have a fast exit path
if no flags are set.

Merged from OpenVPN 2.1

Signed-off-by: James Yonan 
---
 src/openvpn/forward.c | 6 +-
 src/openvpn/forward.h | 4 +++-
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 0ec00f3..fbc58c7 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1026,11 +1026,7 @@ process_ip_header (struct context *c, unsigned int 
flags, struct buffer *buf)
* The --passtos and --mssfix options require
* us to examine the IPv4 header.
*/
-#if PASSTOS_CAPABILITY
-  if (flags & (PIPV4_PASSTOS|PIP_MSSFIX))
-#else
-  if (flags & PIP_MSSFIX)
-#endif
+  if (flags & PIP_OPT_MASK)
{
  struct buffer ipbuf = *buf;
  if (is_ipv4 (TUNNEL_TYPE (c->c1.tuntap), ))
diff --git a/src/openvpn/forward.h b/src/openvpn/forward.h
index 1830a00..0ece01e 100644
--- a/src/openvpn/forward.h
+++ b/src/openvpn/forward.h
@@ -229,9 +229,11 @@ bool send_control_channel_string (struct context *c, const 
char *str, int msglev

 #define PIPV4_PASSTOS (1<<0)
 #define PIP_MSSFIX(1<<1) /* v4 and v6 */
-#define PIPV4_OUTGOING(1<<2)
 #define PIPV4_EXTRACT_DHCP_ROUTER (1<<3)
 #define PIPV4_CLIENT_NAT  (1<<4)
+#define PIP_OPT_MASK  0x /* all possible options for */
+ /* process_ip_header() */
+#define PIPV4_OUTGOING(1<<16)

 void process_ip_header (struct context *c, unsigned int flags, struct buffer 
*buf);

-- 
1.8.5.5