On 16/10/18 23:48, Selva Nair wrote: > Hi, > > Going through patchworks noticed this. > > Thankfully this never got committed so here goes a retraction. > > On Sun, Jan 21, 2018 at 1:45 PM Selva Nair <selva.n...@gmail.com > <mailto:selva.n...@gmail.com>> wrote: > > Hi, > > I'm on a reviewing spree (doing my penance), so here goes.. > > Thanks for the patch > > On Tue, Jan 9, 2018 at 2:16 AM, Eyal Birger <eyal.bir...@gmail.com > <mailto:eyal.bir...@gmail.com>> wrote: > > Address prefix length defaults to /64 on Windows. This change allows > using > > Windows clients in setups that use a different prefix length. > > > > Note: the ability to set the prefix length is documented in the netsh > > 'add address' command, but works on the 'set address' command as well. > > Aside: > If interactive service is in use, the ip helper API is used and setting > prefix already works. Ideally I would like to see openvpn on Windows > used only with the interactive service, but we are not there yet -- > instances started by the automatic service does not use it and there > are some users still running the GUI as admin for some inexplicable > reasons. > > So we need to continue supporting these code paths. > > > > > Signed-off-by: Eyal Birger <eyal.bir...@gmail.com > <mailto:eyal.bir...@gmail.com>> > > --- > > src/openvpn/tun.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c > > index 25831ce..b2b4795 100644 > > --- a/src/openvpn/tun.c > > +++ b/src/openvpn/tun.c > > @@ -1561,15 +1561,16 @@ do_ifconfig(struct tuntap *tt, > > } > > else > > { > > - /* example: netsh interface ipv6 set address > interface=42 2001:608:8003::d store=active */ > > + /* example: netsh interface ipv6 set address > interface=42 2001:608:8003::d/64 store=active */ > > char iface[64]; > > openvpn_snprintf(iface, sizeof(iface), "interface=%lu", > tt->adapter_index ); > > argv_printf(&argv, > > - "%s%sc interface ipv6 set address %s %s > store=active", > > + "%s%sc interface ipv6 set address %s %s/%d > store=active", > > get_win_sys_path(), > > NETSH_PATH_SUFFIX, > > iface, > > - ifconfig_ipv6_local ); > > + ifconfig_ipv6_local, > > + tt->netbits_ipv6); > > netsh_command(&argv, 4, M_FATAL); > > /* set ipv6 dns servers if any are specified */ > > netsh_set_dns6_servers(tt->options.dns6, > tt->options.dns6_len, actual); > > Works as expected and the code is good. > > Currently, on setting the address, a default route gets set with > prefix /64 with gateway as OnLink (does not happen when iphelper api > is used). Although our explicit route to fe80::8 may override it, it > looks better to set the correct prefix in the address. So: > > Acked-by: Selva Nair <selva.n...@gmail.com <mailto:selva.n...@gmail.com>> > > > Though this works in my tests I want to retract this ACK. > > Apart from possible issues due to the appearance of the onlink route in some > cases, I think the correct approach going forward is to stop using netsh and > use the IP helper API for such tasks. And do it in the same way as done using > the service. > > Un-Acked-by: Selva Nair <selva.n...@gmail.com <mailto:selva.n...@gmail.com>> > :)
That was close .... I looked at this yesterday, but since the contributor was a new person and changing details in code paths I'm not that deep into, I wanted to have a much closer look before applying it - luckily I didn't have enough brainpower yesterday to dive into this one. And since you had ACKed it, I considered it generally being safe enough for further processing. So thanks for having a closer look again and notify us instantly :) I'll ensure this patch is tagged as rejected in patchwork. -- kind regards, David Sommerseth OpenVPN Inc
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
