[Openvpn-devel] Make --nobind default for --pull

  Currently we default to local binding with udp. But the majority of
configuration files actually uses --nobind in the configuration to
change the default for --client. And client protocols should normally
use a random source port. This changes the default. Local binding with
--client can still be done using --bind.
---
 Changes.rst           |  3 ++-
 src/openvpn/options.c | 11 ++++-------
 2 files changed, 6 insertions(+), 8 deletions(-)
  

> Arne Schwabe <arne@rfc2549.org> hat am 02.12.2021 13:54 geschrieben:
>  
> -    if (ce->proto == PROTO_TCP_CLIENT && !ce->local
> -        && !ce->local_port_defined && !ce->bind_defined)
> -    {
> -        ce->bind_local = false;
> -    }
>  
> -    if (ce->proto == PROTO_UDP && ce->socks_proxy_server && !ce->local
> -        && !ce->local_port_defined && !ce->bind_defined)
> +    bool need_bind = ce->local || ce->local_port_defined || ce->bind_defined;
> +    bool uses_socks = ce->proto == PROTO_UDP && ce->socks_proxy_server;
> +
> +    if (!need_bind && (ce->proto == PROTO_TCP_CLIENT || uses_socks || o->pull))
>      {
>          ce->bind_local = false;
>      }

The refactoring makes it very hard to see what the actual change is. Maybe it would be better to split this in two commits?

The only actual change in behavior is the "|| o->pull", right?

Regards,
  Frank

--
Frank Lichtenheld
  

Am 03.12.21 um 17:18 schrieb Frank Lichtenheld:
>> Arne Schwabe <arne@rfc2549.org> hat am 02.12.2021 13:54 geschrieben:
>>   
>> -    if (ce->proto == PROTO_TCP_CLIENT && !ce->local
>> -        && !ce->local_port_defined && !ce->bind_defined)
>> -    {
>> -        ce->bind_local = false;
>> -    }
>>   
>> -    if (ce->proto == PROTO_UDP && ce->socks_proxy_server && !ce->local
>> -        && !ce->local_port_defined && !ce->bind_defined)
>> +    bool need_bind = ce->local || ce->local_port_defined || ce->bind_defined;
>> +    bool uses_socks = ce->proto == PROTO_UDP && ce->socks_proxy_server;
>> +
>> +    if (!need_bind && (ce->proto == PROTO_TCP_CLIENT || uses_socks || o->pull))
>>       {
>>           ce->bind_local = false;
>>       }
> 
> The refactoring makes it very hard to see what the actual change is. Maybe it would be better to split this in two commits?
> 
> The only actual change in behavior is the "|| o->pull", right?
> 

Yes. But should I really split up this 5 line patch into two patches? 
That feels a bit excessive.

Arne
  

Hi,

On Sun, Dec 05, 2021 at 03:57:55AM +0100, Arne Schwabe wrote:
> > The refactoring makes it very hard to see what the actual change is. Maybe it would be better to split this in two commits?
> > 
> > The only actual change in behavior is the "|| o->pull", right?
> 
> Yes. But should I really split up this 5 line patch into two patches? 
> That feels a bit excessive.

I'm fine with a single patch here.

I need to think a bit more into "what conditions is this checking, exactly,
and why?" - but with refactoring, I'd have to do that thinking twice :-)

gert
  

> Gert Doering <gert@greenie.muc.de> hat am 05.12.2021 11:19 geschrieben:
> 
>  
> Hi,
> 
> On Sun, Dec 05, 2021 at 03:57:55AM +0100, Arne Schwabe wrote:
> > > The refactoring makes it very hard to see what the actual change is. Maybe it would be better to split this in two commits?
> > > 
> > > The only actual change in behavior is the "|| o->pull", right?
> > 
> > Yes. But should I really split up this 5 line patch into two patches? 
> > That feels a bit excessive.
> 
> I'm fine with a single patch here.
> 
> I need to think a bit more into "what conditions is this checking, exactly,
> and why?" - but with refactoring, I'd have to do that thinking twice :-)

I disagree. If you had one patch that does refactoring but does not intend any functional change,
and one patch that just adds "|| o->pull", the general understanding of each patch would require
much less time and thinking. Mostly it would be much easier to match the change with the intent
communicated by the commit message. Which for me was the hardest part, here.

Regards,
  Frank

--
Frank Lichtenheld
  

Hi,

On Sun, Dec 05, 2021 at 12:16:56PM +0100, Frank Lichtenheld wrote:
> > I need to think a bit more into "what conditions is this checking, exactly,
> > and why?" - but with refactoring, I'd have to do that thinking twice :-)
> 
> I disagree. If you had one patch that does refactoring but does not intend any functional change,
> and one patch that just adds "|| o->pull", the general understanding of each patch would require
> much less time and thinking. 

True, but the second patch would not be correct, then :-) - as it
actually adds more conditions than just "o->pull".  The original code
does not check for socks.

I am not exactly happy with the *new* code, as it is still too convoluted
for my coffee level ("if (!bind_needed && conditions) set bind_local=false",
what are these conditions?  why?) but I do not think an intermediate 
patch will make these particular conditional contortions easier...

gert
  

Am 05.12.2021 um 12:40 schrieb Gert Doering:
> Hi,
>
> On Sun, Dec 05, 2021 at 12:16:56PM +0100, Frank Lichtenheld wrote:
>>> I need to think a bit more into "what conditions is this checking, exactly,
>>> and why?" - but with refactoring, I'd have to do that thinking twice :-)
>> I disagree. If you had one patch that does refactoring but does not intend any functional change,
>> and one patch that just adds "|| o->pull", the general understanding of each patch would require
>> much less time and thinking.
> True, but the second patch would not be correct, then :-) - as it
> actually adds more conditions than just "o->pull".  The original code
> does not check for socks.


The original code checks for socks. That is not changed.  The orignal 
code basically has two ifs that checks for the code that is !bind_need 
and the particular condition (socks or tcp-client)

>
> I am not exactly happy with the *new* code, as it is still too convoluted
> for my coffee level ("if (!bind_needed && conditions) set bind_local=false",
> what are these conditions?  why?)

I can add more comments to the code. bind_needed is basically if the 
options have a condition that forces binding anyway.

>   but I do not think an intermediate
> patch will make these particular conditional contortions easier...
>
  

Hi,

On Sun, Dec 05, 2021 at 07:04:01PM +0100, Arne Schwabe wrote:
> The original code checks for socks. That is not changed.  The orignal 
> code basically has two ifs that checks for the code that is !bind_need 
> and the particular condition (socks or tcp-client)

You're right.  As I said, that mass if conditions, half of them
negated, exceeded my coffee level...

> > for my coffee level ("if (!bind_needed && conditions) set bind_local=false",
> > what are these conditions?  why?)
> 
> I can add more comments to the code. bind_needed is basically if the 
> options have a condition that forces binding anyway.

I think that would be good.

thanks :)

gert