Hi Patrick, sorry for the delay :-/
On Mon, Jun 19, 2017 at 01:54:36PM -0400, Patrick Hemmer wrote:
> Well my argument for keeping the name starting with `ssl_fc_session_` is
> that there is also `ssl_fc_session_id`. These 2 fetches pull their
> attribute from the same "session" structure. They are also closely
> related as using `ssl_fc_session_key` almost requires the
> `ssl_fc_session_id` value (you could technically get by without it, but
> it'll make using the key rather difficult unless you have some other way
> of correlating a key with a specific SSL session).
OK, that totally makes sense then.
> >> static int
> >> +smp_fetch_ssl_fc_session_key(const struct arg *args, struct sample *smp,
> >> const char *kw, void *private)
> >> +{
> >> +#if OPENSSL_VERSION_NUMBER >= 0x10100000L || defined(OPENSSL_IS_BORINGSSL)
> > Here I'd put the ifdef around the whole function, like we have for
> > ALPN for example, so that there's no risk it could be used in a non-working
> > config.
> My objection to this is that most of the other sample fetches don't do
> this, and so it makes the user experience inconsistent. For example the
> `ssl_fc_session_id` fetch, which `ssl_fc_session_key` is strongly
> related to, behaves the same way.
But this has already been causing problems, because people build with their
lib, then configure their logs, and suddenly realize that the field is empty
and report the bug here. The issue I'm having is that there's no notification
that this will not work. Using #ifdef ensures that what is not supported will
report an error. Then the user looks at the keyword in the doc and reads
"requires openssl 1.1 or above" and understands why there's this problem.
> The ALPN/NPN fetches are the only ones
> that make using the fetch a config error if the underlying support is
> missing.
For SSL indeed, but if you look at other places like TCP, bind keywords
or server keywords, it's exactly the opposite. There are other places
which are cleaner, it's only the arg resolver which has the ifdef so
that instead of not parsing, it's parsed and reported as "not supported"
or "requires version blah". I just can't find such an example but I'm
pretty sure we do have some.
Cheers,
Willy
