BoringSSL doesn't have BIO_sock_non_fatal_error() #1438
Conversation
|
FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1516/ for details. |
|
Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1410/ for details. |
|
clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/81/ for details. |
jablko
force-pushed
the
non_fatal_error
branch
from
72ae29f
to
0c9f13a
Compare
|
Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1411/ for details. |
|
FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1517/ for details. |
|
clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/82/ for details. |
| // bio_fd_non_fatal_error() instead.) #1437 | ||
| // | ||
| // The following is copied from | ||
| // https://github.com/openssl/openssl/blob/3befffa39dbaf2688d823fcf2bdfc07d2487be48/crypto/bio/bss_sock.c |
There was a problem hiding this comment.
It looks like we need to add "OpenSSL License" in LICENSE file, because this is just copy.
(Sorry for I should have noticed this before merge)
There was a problem hiding this comment.
@jablko How about just add a function like this? We don't support WINDOWS and don't need those "#ifdef". Just checking error codes looks enough.
static int
non_fetal_error(int err)
{
if (err == EWOULDBLOCK || err == ENOTCONN || err == EINTR || err == EAGAIN || err == EPROTO || err == EINPROGRESS || err == EALREADY) {
return 1;
}
return 0;
}
There was a problem hiding this comment.
That works ... Is the purpose to avoid having to attribute it?
In general, I like to code for OpenSSL, and add fixes to make other environments conform to it -- unless there's a reason not to. So in this case I'd continue calling BIO_sock_non_fatal_error(), and copy it verbatim from OpenSSL if it's missing.
Functionally, your solution is equivalent (and cleaner!), but unless there's a reason not to use OpenSSL's function, that's the style I prefer. I'm happy either way, though :-)
I'll open a PR to add attribution to the NOTICE file.
There was a problem hiding this comment.
I understood. Let's add the attribution to the NOTICE file and use code from OpenSSL.
|
Did we add the necessary attribution to NOTICES files? If not, please do so. |
Fixes #1437