[vpnc-devel] RFC: [PATCH] vpnc-script: split disconnect and destroy

[Dan Williams]

On Sun, 2014-02-16 at 11:15 +0000, David Woodhouse wrote:
> On Sun, 2014-02-16 at 12:48 +0800, Antonio Borneo wrote:
> > Hi David,
> > 
> > I'm reworking a patch I have received for vpnc
> > http://lists.unix-ag.uni-kl.de/pipermail/vpnc-devel/2013-December/004037.html
> > in a set of new patches that I'm going to apply soon.
> > 
> > The key point is the split, in vpnc-script, of do_disconnect()
> > by creating a new do_destroy().
> > I have isolated this part in the patch in attachment.
> > 
> > For backward compatibility I kept the original behavior for
> > reason=disconnect, but I consider this name as incorrect and
> > obsolete so added synonym reason=disconnect_destroy.
> 
> For me the most interesting case of backward compatibility has been
> using an old script with newer VPN client. At least in the past with
> various systems shipping vpnc-script from the vpnc distribution while
> OpenConnect required various new features, that's the one I've had to
> deal with more.
> 
> So I'm wondering if a better approach would be to continue to use
> 'disconnect', but add a new environment variable which indicates that
> this is a new VPN client that supports a separate 'destroy' invocation.

+1 from me; that's cleaner to deal with for NetworkManager-vpnc too, I
think.

Dan

> So instead of the plethora of
> 'disconnect/disconnect_only/disconnect_destroy' methods, we just have
> 'disconnect' with a modifier in a separate variable. And 'destroy'.
> 
> That way, all forms of compatibility should work. New clients invoking
> an old script will call 'disconnect', which will do the destroy
> operation too, and then call the unsupported 'destroy' method.
> 
> Old clients invoking a new script will calls 'disconnect' but without
> that extra 'disconnect=disconnect_only' environment variable (or
> whatever we choose to call it), and thus the new script will do what
> they expect.
> 
> > Please notice that destroy_tun_device() has code for *BSD only.
> > If we suppose that on these OS the packages are built with proper
> > dependencies, we can be more aggressive with a cleaner patch
> > that introduces only the reason destroy.
> 
> We'd have to look again at that. In the past they've mostly been
> shipping an old version from vpnc which has been problematic for
> openconnect. I think FreeBSD at least did switch to a separate
> vpnc-script package which both their vpnc and openconnect packages
> depend on. And it seems to be up to date as of about two days ago.
> Despite that change being Windows-only :)
> 
> The OpenConnect configure script does check for existence of vpnc-script
> in the default location. It could be augmented to check whether the
> script supports the 'destroy' method, perhaps.
> 
> But in practice I think it's easier and safer to just ensure
> compatibility in both directions, as described above.
> 
> > In this case reason destroy have to be called independently by
> > openconnect and vpnc.
> 
> Right.
> 
> Did I ever give you access to the vpnc-scripts repository, btw? If you
> mail me a SSH public key (and preferred username), I can.
> 
> I have changes to vpnc-script-win.js in that repository, which probably
> don't affect you yet since you don't support IPv6 yet. But there may be
> more changes coming (it doesn't support full-tunnel mode in Legacy IP,
> and should probably *remove* all IP addresses before setting up the
> tunnel and in 'disconnect', etc.).
> 
> (Btw, once vpnc is in git as its primary version control, it should be
> simple enough just to do 'git pull' to pull in vpnc-script changes.)
> 
> _______________________________________________
> vpnc-devel mailing list
> vpnc-devel at unix-ag.uni-kl.de
> https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
> http://www.unix-ag.uni-kl.de/~massar/vpnc/