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

[David Woodhouse]

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.

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.)

-- 
dwmw2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5745 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/openconnect-devel/attachments/20140216/4e5b979f/attachment-0001.bin>