Hi Willy, Patrick > Le 30 juin 2017 à 07:00, Willy Tarreau <w...@1wt.eu> a écrit : > > 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. >
ALPN/NPN fetches depend inderectly on ALPN//NPN settings. . used ALPN/NPN fetches could be always valid because without ALPN/NPN setting support ALPN/NPN fetches return 0 is valid. . ALPN//NPN settings return error if lib doesn’t support it ++ Manu