FW: [PATCH] when we need to transfer data between file and socket we prefer to use sendfile instead of write because we save the copy to a buffer

[Maxim Dounin]

Hello!

(Please keep this on the nginx-devel@ mailinig list.)

On Mon, May 13, 2019 at 06:53:15AM +0000, Ben Ben Ishay wrote:

> On 5/6/2019 5:30 PM, Maxim Dounin wrote:
> > Hello!
> > 
> > On Mon, May 06, 2019 at 06:02:10AM +0000, Ben Ben Ishay wrote:
> > 
> > [...]
> > 
> >>> - The SSL_sendfile() call you are using in this version does not
> >>>     seem to exists in any published version of OpenSSL, including
> >>>     github repo.  This is not going to work.
> >>>
> >> we attach a documentation link about this
> >> function(https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMellanox%2Fopenssl%2Fblob%2Ftls_sendfile%2Fdoc%2Fman3%2FSSL_write.pod&data=02%7C01%7Cbenishay%40mellanox.com%7Cbb2fb3506bbd4953d2fd08d6d22f5f0d%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636927498228272408&sdata=Na%2B%2Be2nibz%2BkFBt%2FSGLYer9oe5D4OpVUp21LNWQsHzU%3D&reserved=0),
> >> we have a pull request at advanced stage for adding this function.
> >>
> >>> - The approach you are using - that is, introducing changes into
> >>>     ngx_linux_sendfile_chain.c - is not portable, and is not going
> >>>     to work on other platforms if/when appropriate kernel level and
> >>>     OpenSSL level support will be available.  As suggested in the
> >>>     previous thread, this should be something handled at the
> >>>     ngx_event_openssl.c level.
> >>>
> >> we think that this is the best solution because there is a unique
> >> function for every OS  and there is a difference between them for
> >> example ngx_linux_sendfile_chain use TCP_CORK option while
> >> ngx_freebsd_sendfile_chain dosent(in addition in solaris function
> >> ngx_solaris_sendfilev_chain there is a call to sendfilev that dosent
> >> exists in linux) , we can create a function that is constructed from the
> >> diffrenet flow for OS's but we think this option include copying code
> >> and thus the best option is to change the sendfile call in every
> >> ngx_sendfile function when the OS and OPENSSL will support SSL_Sendfile.
> > 
> > While handling of the sendfile() syscall is certainly OS-specific,
> > the SSL_sendfile() interface as provided by OpenSSL is certainly
> > not going to follow these OS-specific code paths.  For example, I
> > cannot reasonably assume SSL_sendfile() will support headers and
> > trailers on FreeBSD, or will be similar to sendfilev() interface
> > on Solaris.
> > 
> The SSL_sendfile() is actually OS-specific, we attach the 
> documentation.(https://www.openssl.org/docs/manmaster/man3/SSL_sendfile.html)
> The flag field has a use at least for FreeBSD.

As per the documentation committed, it is actually not 
OS-specific (though may be available only on some OSes, with some 
per-OS nuances).  And that's actually the point of the 
SSL_sendfile() interface.

> > That is, SSL_sendfile() is going to be a separate interface,
> > different from all the OS-specific interfaces, with its own
> > features and limitations.  And most likely it will need different
> > error handling, including handling of SSL-specific errors.
> > 
> we agree with your claim about error handling, and we will be glad to 
> hear your solution.

I've already pointed you to the preliminiry work from Netflix, as 
well as to the SSL_write() usage in nginx.

> > As such, trying to change OS-specific sendfile chains in nginx
> > looks wrong to me.  Not to mention it will require additional
> > application-level work upon introduction of in-kernel SSL/TLS in
> > other OSes.  Rather, it should be an SSL-level changes,
> > introducing an SSL-level send chain, similar to one we already
> > provide for SSL_write().
> > 
> > (Note well that using BIO_get_ktls_send() to find out if
> > SSL_sendfile() can be used or not also looks strange and
> > non-portable.  If the route taken is to provide SSL_sendfile(), an
> > OS-independent interface to use sendfile() when supported by the
> > OpenSSL library, there should be a simplier / more portable way to
> > test if SSL_sendfile() can be used on a particular SSL
> > connection.)
> > 
> The documentation explain this.

While it certainly does, things can be done in a more portable 
way.

-- 
Maxim Dounin
http://mdounin.ru/