On 08/03/2016 11:57 PM, Amos Jeffries wrote: > +Security::SetSessionResumeData(const Security::SessionPointer &s, const > Security::SessionStatePointer &data) > +{ > + if (data) { > +#if USE_OPENSSL > + (void)SSL_set_session(s.get(), data.get()); > +#elif USE_GNUTLS > + (void)gnutls_session_set_data(s.get(), data->data, data->size); > +#endif > + } > +}
Why cast to (void) here? The current code does not do that AFAICT. Casting the result to (void) usually means * I know this may fail, but I do not care. It should be accompanied by a comment that explains why we do not care, unless the code makes it obvious (the patched code does not). Not casting may mean many things, including: * It returns void. * I did not even think about it. * I know it may fail, but I either do not know how to handle its failure or just do not have the time to handle it. AFAICT, in this particular case, the current code probably matches one of the last two bullets. If I am right, then there are two correct ways to deal with it IMO: 1. Return a boolean success/failure result. The caller will still ignore the result. 2. Remove "(void)". Ideally, add a "// TODO: throw on failures" comment. My weak recommendation is for #2. > +Security::SessionIsResumed(const Security::SessionPointer &s) > +{ > + return > +#if USE_OPENSSL > + SSL_session_reused(s.get()) == 1; > +#elif USE_GNUTLS > + gnutls_session_is_resumed(s.get()) != 0; > +#else > + false; > +#endif > +} > + > +void > +Security::GetSessionResumeData(const Security::SessionPointer &s, > Security::SessionStatePointer &data) > +{ > + if (!SessionIsResumed(s)) { > +#if USE_OPENSSL > + data.reset(SSL_get1_session(s.get())); > +#elif USE_GNUTLS > + gnutls_datum_t *tmp = nullptr; > + (void)gnutls_session_get_data2(s.get(), tmp); > + data.reset(tmp); > +#endif > + } > +} Can we establish some rules for when to use "#else" in OpenSSL/GnuTLS/Security contexts? The inconsistency illustrated above is a red flag. I do not think "use #else if it is needed to compile" is the rule used for the above code because Security::GetSessionResumeData() may produce an "unused parameter 'data'" warning, killing the build. > +/// Retrieve the data needed to resume this session on a later connection > +void GetSessionResumeData(const Security::SessionPointer &, > Security::SessionStatePointer &); > +void > +Security::GetSessionResumeData(const Security::SessionPointer &s, > Security::SessionStatePointer &data) > +{ > + if (!SessionIsResumed(s)) { The implementation does not seem to match the description. The implementation matches the current code, but the description says nothing about !SessionIsResumed() precondition. Please either change description and rename OR test the precondition in the callers, whichever you think is better long-term. Thank you, Alex. _______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev