[strongSwan-dev] Patch proposals

[Tobias Brunner]

Hi Emeric,

> Please find attached some of the patches we have applied.
> The goal is mainly to ease integration with our build environment

Please find my feedback and questions on the patches below.

> patch-0000-ha-flush-segment -> Add a command to flush all the SA of a HA segment

What exactly is the use case here?

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

> patch-0002-libhydra-flush-only-managed-sas -> do not flush everything but only managed SA types (AH, ESP, IPCOMP) -> Leave intact SA inserted by other daemons like bird.

Makes sense, but should probably also be done for the kernel-netlink
plugin.  One problem is that we can't control which policies to flush.
Luckily with the changes I merged to master this morning, already
existing policies are not treated as errors anymore, so we can probably
get rid of the flush_policies() call in starter (if installpolicies=no
is used it wouldn't make sense to flush them anyway - I think the main
reason to flush them was to avoid later conflicts).  I wonder if we
could also remove the flush_sas() call (but not all SAs actually have a
lifetime set, in particular IPComp SAs, but neither have policies for
that matter).  Anyway, if this is required something went wrong anyway.
 I pushed some changes to the kernel-flush branch.

> patch-0003-ha-fifo-sanity-checks -> Add some checks on the HA ctl file

I took some of the ideas and pushed them to the ha-ctl-checks branch.

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

> 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?

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

> patch-0007-kernel_pfroute-virtual-ip-option -> implement the install_virtual_ip option for the kernel_pfroute plugin

Applied to master.

Regards,
Tobias