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

[Antonio Borneo]

On Wed, Feb 19, 2014 at 5:26 PM, David Woodhouse <dwmw2 at infradead.org> wrote:
> On Tue, 2014-02-18 at 15:39 +0800, Antonio Borneo wrote:
>> Well, my idea was to first patch the scripts in vpnc and your
>> repository, then modify the clients (vpnc and if needed also
>> openconnect).
>>
>> Anyway with my proposal it's easy to make new clients working with old
>> (actually current) script.
>> When the client calls any of the the new methods, the old script prints
>>    unknown reason 'destroy'. Maybe vpnc-script is out of date
>> end exit 1.
>> Would be up to us to handle this case in the client.
>>
>> A new client should do something like:
>>   ret = system("vpnc-script disconnect_only");
>>   if (ret == 1) {
>>     printf("additional warning about old script\n");
>>     system("vpnc-script disconnect");
>>   }
>>   .. whatever other stuff ..
>>   if (ret == 0)
>>     system("vpnc-script destroy");
>
> I don't like this.
>
> Firstly, it's turning the vpnc-script invocation from a
> 'fire-and-forget' into something more complicated, and it's more fragile
> than it used to be.
>
> Secondly, it'll confuse users who see the message about
>      unknown reason 'disconnect_destroy'
> and will lead to more people asking questions about it on random web
> fora where they never get good answers, instead of just sending mail to
> the list like sensible people. Which always makes me sad.
>

Current script will print "unknown reason xyz" whatever new reason we add.
But we can silient it redirecting its stdout to /dev/null, using only
for the new reasons a code equivalent to
    system("/path/to/vpnc-script > /dev/null");

In such case I would add to new scripts a further reason 'script-version'.
On old script it will fallback in the 'unknown reason' case, so exit 1
The new script will exit 0 and return the version. Or even a trivial
exit $version.

> I would much prefer to stick with 'disconnect', with a separate variable
> set to indicate that there will be a subsequent 'destroy' call, and then
> the promised 'destroy' call.
>

Just to be sure.
You mean that in new scripts:
- reason disconnect plus separate variable == alpha, means disconnect only;
- reason disconnect plus separate variable == beta, means destroy only.
Old script will receive twice call to disconnect (since ignores the
new variable) and we have to verify this double call does not messup
the network.
Correct?

Antonio

>> For Dan.
>> This would be transparent to NetworkManager-vpnc.
>> The helper in src/nm-vpnc-service-vpnc-helper.c line 331
>> only handles reason=="connect". Exit 0 in the other cases.
>>
>> Same for NetworkManager-openconnect.
>> Helper src/nm-openconnect-service-openconnect-helper.c line 489
>> only handles reason=="connect". Exit 0 in the other cases.
>
> (Hm, it's a shame those aren't the *same* damn executable.)
>
> --
> dwmw2