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

[James Yonan]

On 04/03/2016 02:49, Arne Schwabe wrote:


Am 04.03.16 um 08:29 schrieb James Yonan:

On 03/03/2016 16:48, Arne Schwabe wrote:

Am 03.03.16 um 09:18 schrieb 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.

Basically what this patch does is to change the condition to

if (flags)

and if for example PASSTOS_CAPABILITY is not 1, the following path will
always be taken:

process_ip_header (c, PIPV4_PASSTOS|PIP_MSSFIX|PIPV4_CLIENT_NAT,
>c2.buf);

flags mean that possible passtos, mssfix and client_nat should be
applied here.

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

is not compiled in. So flags is at least PIPV4_PASSTOS

So if (flags & 0x) is still true.

So NACK from me butthe code is very confusing...

Arne

I think what makes this patch confusing is that it's really a patch
that facilitates another patch that we've used in the past at OpenVPN
Tech. for some custom NAT algs.  This patch reduces the footprint of
the second patch, making it easier to maintain.


Yes. I think I get the intention of the patch. But if this patch should
work in OpenVPN now we need to replace

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

with

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

to have the correct behaviour if PASSTOS_CAPABILITY is missing.


Actually, I'm fine to withdraw this patch for now -- it's probably more 
trouble than it's worth.


James

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

[Arne Schwabe]

Am 04.03.16 um 08:29 schrieb James Yonan:
> On 03/03/2016 16:48, Arne Schwabe wrote:
>> Am 03.03.16 um 09:18 schrieb 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.
>> Basically what this patch does is to change the condition to
>>
>> if (flags)
>>
>> and if for example PASSTOS_CAPABILITY is not 1, the following path will
>> always be taken:
>>
>>process_ip_header (c, PIPV4_PASSTOS|PIP_MSSFIX|PIPV4_CLIENT_NAT,
>> >c2.buf);
>>
>> flags mean that possible passtos, mssfix and client_nat should be
>> applied here.
>>
>> #if PASSTOS_CAPABILITY
>>if (!c->options.passtos)
>>  flags &= ~PIPV4_PASSTOS;
>> #endif
>>
>> is not compiled in. So flags is at least PIPV4_PASSTOS
>>
>> So if (flags & 0x) is still true.
>>
>> So NACK from me butthe code is very confusing...
>>
>> Arne
>
> I think what makes this patch confusing is that it's really a patch
> that facilitates another patch that we've used in the past at OpenVPN
> Tech. for some custom NAT algs.  This patch reduces the footprint of
> the second patch, making it easier to maintain.
>
Yes. I think I get the intention of the patch. But if this patch should
work in OpenVPN now we need to replace

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

with 

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

to have the correct behaviour if PASSTOS_CAPABILITY is missing.

Arne

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

[James Yonan]

On 03/03/2016 16:48, Arne Schwabe wrote:

Am 03.03.16 um 09:18 schrieb 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.

Basically what this patch does is to change the condition to

if (flags)

and if for example PASSTOS_CAPABILITY is not 1, the following path will
always be taken:

   process_ip_header (c, PIPV4_PASSTOS|PIP_MSSFIX|PIPV4_CLIENT_NAT,
>c2.buf);

flags mean that possible passtos, mssfix and client_nat should be
applied here.

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

is not compiled in. So flags is at least PIPV4_PASSTOS

So if (flags & 0x) is still true.

So NACK from me butthe code is very confusing...

Arne


I think what makes this patch confusing is that it's really a patch that 
facilitates another patch that we've used in the past at OpenVPN Tech. 
for some custom NAT algs.  This patch reduces the footprint of the 
second patch, making it easier to maintain.


James





Merged from OpenVPN 2.1

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

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 4a91f92..ef554fc 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1054,13 +1054,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 (flags & (PIP_MSSFIX
-#if PASSTOS_CAPABILITY
- | PIPV4_PASSTOS
-#endif
- | PIPV4_CLIENT_NAT
- ))
+  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 af3b0a6..7debcb1 100644
--- a/src/openvpn/forward.h
+++ b/src/openvpn/forward.h
@@ -249,9 +249,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);
  

[Openvpn-devel] [PATCH 02/10] 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 | 8 +---
 src/openvpn/forward.h | 4 +++-
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 4a91f92..ef554fc 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1054,13 +1054,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 (flags & (PIP_MSSFIX
-#if PASSTOS_CAPABILITY
- | PIPV4_PASSTOS
-#endif
- | PIPV4_CLIENT_NAT
- ))
+  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 af3b0a6..7debcb1 100644
--- a/src/openvpn/forward.h
+++ b/src/openvpn/forward.h
@@ -249,9 +249,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.9.1