[Swan-dev] --impair

[Andrew Cagney]

On 5 October 2017 at 15:23, Paul Wouters <paul at nohats.ca> wrote:

> On Thu, 5 Oct 2017, Antony Antony wrote:
>
> --impair ... now works?
>>>
>>
>> what I added is
>>
>> ipsec whack  --debug-all --impair drop-xauth-r0
>>
>> ipsec whack --impair-drop-xauth-r0 do not work.
>> unrecognized option '--impair-drop-xauth-r0'
>>
>>
That's because it was called --impair-drop-no-xauth-r0 when added to whack:

index 74ec5bd03..df9684004 100644
--- a/programs/whack/whack.c
+++ b/programs/whack/whack.c
@@ -786,6 +786,8 @@ static const struct option long_opts[] = {
                IMPAIR_SEND_NO_IKEV2_AUTH_IX + DO },
        { "impair-send-no-xauth-r0", no_argument, NULL,
                IMPAIR_SEND_NO_XAUTH_R0_IX + DO },
+       { "impair-drop-no-xauth-r0", no_argument, NULL,
+               IMPAIR_DROP_XAUTH_R0_IX + DO },
        { "impair-send-no-main-r2", no_argument, NULL,
                IMPAIR_SEND_NO_MAIN_R2_IX + DO },
        /*

the reason I added --impair ... and --debug ... was because I found the
duplication redundant and somewhat error proned


I am confused why we have two methods. When I looked enumcheck-01 had  other
>> unknown changes, so I didn't commit. It seems you are saying enumcheck-01
>> should have noticed me adding  --impair drop-xauth-r0?
>>
>> What need fixing? May we should allow only one method.
>>
>
>
enumcheck validates and dumps the enum tables, including this one so the
changed output needs a review and update

I'm also unsure why there are two methods. Or what was wrong with the
> first one. I understood it was split off from debug, which is good so
> we can presumably run the impair without causing modified debug levels,
> which means we can remove the --debug-all from all impair calls. Whether
> there is a minus or a space doesn't matter to me, as long as it is
> obvious how to specify multiple impairs.
>

 Unfortunately:

     --debug-all --impair ...

is still needed.  Fixing that gets more invasive.  While I suspect we can
get away with sending separate debug and impair lset_t-s over the wire and
then have pluto merge the updates, I'm speculating.


> Since impair should never be used in production, there is no reason to
> keep an old API around. So please kill one of the two.



I figured that for both the existing --impair-... and --debug-* ... options
we should leave leave them (including some broken names), but encourage the
use of the cleaner --impair and --debug ... implementation on new options.

Since --impair is conisidered strictly internal, I'll remove them.

Andrew
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libreswan.org/pipermail/swan-dev/attachments/20171005/e589d671/attachment.html>