[strongSwan-dev] Patch proposals

[Emeric POUPON]

Hello,

Thanks for your support.
I did not make all of these patches, but I will try to answer your question as best as possible.

>> patch-0000-ha-flush-segment -> Add a command to flush all the SA of a HA segment
> 
> What exactly is the use case here?

We do not use the heartbeat mechanism to detect failures since we have another means for that (corosync).
If the active charon instance crashes and restarts immediately, we use this in order to flush everything on the passive instance.
We have another patch that sends a flush message to the remote charon if we start as an active instance (not sure you're interested by this one)

>> patch-0001-alert-initial-contact -> Add an alert to listen to INITIAL CONTACT events
>
> Same question, what is the use case for this?  And can't you just use

Actually, the alert mechanism is quite useful to get notified from significant events. Therefore we made a custom alert plugin in order to trap
interesting alerts and externally log them.

> the message hook to check for INITIAL_CONTACT notifies?  I'm mainly
> concerned with polluting the alert space with new events that are rarely
> used but increase traffic on the bus.

I am not sure to understand the message hook thing. You would get all the messages thanks to the hook and parse
the decrypted payloads in order to intercept the INITIAL CONTACT notifies?
Would the IKE SA be set in the current thread context? (it contains valuable information to be exported)

>> patch-0004-ha-sync-delay -> Already in a branch on github. The goal is to request a sync only once the configuration has been completely reloaded.
> 
> Can't say I like this.  It is specific to stroke, adds additional
> alerts, won't work if multiple config backends are loaded etc.  Perhaps
> Martin can chime in and comment on why he created the branch.

I do agree this is not an ideal fix. However, we have a real problem here and I did not find any better solution.

>> patch-0005-disable-backtrace-use -> Condition "backtrace" since we do not have backtrace available on our systems
>
>As far as I can tell, if backtrace() isn't available and no alternatives
>are used (like libunwind) backtrace_t.log() is basically a noop that
>will just log "no support for capturing backtraces".  Is that a problem?

Not sure about this one since I did not make it. I will try to see what is wrong with it and give you more information.

>> patch-0006-fix-compile-warnings -> Fix some of the warnings we got on our toolchain
>
>Applied most of them to master.  I don't get the cast in
>openssl_asn1_obj2chunk(), though.  Why is that triple cast needed to
>simply cast the const qualifier away (the `data` member seems to be a
>const u_char*)?

I admit this is quite odd, I tried again:

return chunk_create((u_char*)asn1->data, asn1->length);
->
openssl_util.c:151: warning: passing argument 1 of 'chunk_create' discards qualifiers from pointer target type

u_char* data = (u_char*)asn1->data;
return chunk_create(data, asn1->length);
->
OK

>I haven't applied the cas_ptr() casts, as these are only required when
>no atomic built-ins are available (we could probably define the fallback
>with void* as first argument to avoid the warnings, however, in the
>apidoc void** clarifies that a pointer to a pointer has to be passed).

Indeed we added this in order to build without warning on ARM.


Thanks again for your support,

Emeric