Hi,

On Tue, Jul 19, 2022 at 04:16:39PM +0200, Antonio Quartulli wrote:
> open_tun_generic already contains the logic required to find a device
> name when not specified b the user. For this reason the DCO case can
> easily leverage on function and avoid code duplication.
> 
> Signed-off-by: Antonio Quartulli <a...@unstable.cc>

*sigh*

First of all, my apologies for letting you jump through all these hoops -
I'm NOT doing this to annoy you or drag my feet, but really to keep the
code readable and maintainable.

That said, I've spent two days now pondering the latest version of this
patch, and I can not ACK it - it's ugly beyond openvpn standards.

My main gripe is that we have this open_tun_generic() function which
is used for "all but linux (and Windows)".  So you add a call to it
now for the DCO call (this is okay-ish) but then it's not actually
*using* the code paths in open_tun_generic(), but adding new and
linux-exclusive code paths #ifdef TARGET_LINUX.

So, on Linux with DCO, very little of that function is actually used
(half of it is #ifdef TARGET_LINUX, and the non-#ifdef parts are 
"else" branches for "not DCO" - but "for not DCO", we do not call this
in the first place) - all that remains is a for() loop to iterate
device names, and a string_alloc() call...


That said, I've cooked up a v11 of 05, which introduces a much
leaner open_tun_dco_generic() function - to be shared by Linux
and FreeBSD - which does "only DCO things", leaving open_tun_generic()
alone.  I've tested this with the usual test cases

 --dev tap    (works, because options.c will already disable DCO)
 --dev tun    (iterates to "tun7" on my DCO test system)
 --mktun --dev tun471 ; use --dev tun471  (works, falls back to tun)
 --dev tun472 (works as DCO device))


Right now this is sitting on top of my clone of your "dco" tree - 
I'll see that I can apply this to the right parts in the middle and
send a 05 v11 out later, then we can haggle on the least ugly way
forward.

gert

PS1: I'm fairly sure *I* did the existing #ifdef TARGET_NETBSD in 
     open_tun_generic() and I should not have done that.  Bad precedence.

PS2: 05 v10 works, and does not break anything, But It Is Too Ugly.

-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             g...@greenie.muc.de

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Reply via email to