Date: Tue, 10 Oct 2017 06:23:24 -0700 From: Jason Eggleston <eggnet@gmail.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Gleb Smirnoff <glebius@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org, Sean Bruno <sbruno@freebsd.org>, src-committers@freebsd.org Subject: Re: svn commit: r324405 - head/sys/kern Message-ID: <CAC27FbWSX1R_6jHo5Wjg9rbPY_tmiJGdJdic8EsxcvpS9T1TAQ@mail.gmail.com> In-Reply-To: <20171010071703.GR95911@kib.kiev.ua> References: <201710072330.v97NUv7E040636@repo.freebsd.org> <20171009232052.GU1055@FreeBSD.org> <20171010071703.GR95911@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
Sendfile does return EPIPE today. All that has to happen is receiving a RST after sendfile starts looping. There is a race condition, which still exists after this suggested patch, where ECONNRESET could be returned. But mostly it is EPIPE. On Oct 10, 2017 12:17 AM, "Konstantin Belousov" <kostikbel@gmail.com> wrote: > On Mon, Oct 09, 2017 at 04:20:52PM -0700, Gleb Smirnoff wrote: > > Sean & Jason, > > > > On Sat, Oct 07, 2017 at 11:30:57PM +0000, Sean Bruno wrote: > > S> Author: sbruno > > S> Date: Sat Oct 7 23:30:57 2017 > > S> New Revision: 324405 > > S> URL: https://svnweb.freebsd.org/changeset/base/324405 > > S> > > S> Log: > > S> Check so_error early in sendfile() call. Prior to this patch, if a > > S> connection was reset by the remote end, sendfile() would just report > > S> ENOTCONN instead of ECONNRESET. > > S> > > S> Submitted by: Jason Eggleston <jason@eggnet.com> > > S> Reviewed by: glebius > > S> Sponsored by: Limelight Networks > > S> Differential Revision: https://reviews.freebsd.org/D12575 > > S> > > S> Modified: > > S> head/sys/kern/kern_sendfile.c > > S> > > S> Modified: head/sys/kern/kern_sendfile.c > > S> ============================================================ > ================== > > S> --- head/sys/kern/kern_sendfile.c Sat Oct 7 23:10:16 2017 > (r324404) > > S> +++ head/sys/kern/kern_sendfile.c Sat Oct 7 23:30:57 2017 > (r324405) > > S> @@ -514,6 +514,11 @@ sendfile_getsock(struct thread *td, int s, > struct file > > S> *so = (*sock_fp)->f_data; > > S> if ((*so)->so_type != SOCK_STREAM) > > S> return (EINVAL); > > S> + if ((*so)->so_error) { > > S> + error = (*so)->so_error; > > S> + (*so)->so_error = 0; > > S> + return (error); > > S> + } > > S> if (((*so)->so_state & SS_ISCONNECTED) == 0) > > S> return (ENOTCONN); > > S> return (0); > > > > Despite my initial positive review, now I am quite unsure on that. > > > > Problem is that sendfile(2) isn't defined by SUS, so there is no > > distinctive final answer on that. Should we match other OSes? > > Should we match our historic behaviour? Or should we match > > write(2)/send(2) to socket, which are closest analogy. I probably > > believe in the latter: sendfile(2) belongs to write(2)/send(2) > > family. > > > > SUS specifies that write may return ECONNRESET. It also documents > > EPIPE. However, our manual page documents only EPIPE for both > > send(2) and write(2). For write we have: > > > > SOCKBUF_LOCK(&so->so_snd); > > if (so->so_snd.sb_state & SBS_CANTSENDMORE) { > > SOCKBUF_UNLOCK(&so->so_snd); > > error = EPIPE; > > goto out; > > } > > if (so->so_error) { > > error = so->so_error; > > so->so_error = 0; > > SOCKBUF_UNLOCK(&so->so_snd); > > goto out; > > } > > > > Indeed, EPIPE will be returned prior to return/clear of so_error, > > which supposedly is ECONNRESET. > > > > In the sendfile(2) implementation we see exactly same code: > > > > if (so->so_snd.sb_state & SBS_CANTSENDMORE) { > > error = EPIPE; > > SOCKBUF_UNLOCK(&so->so_snd); > > goto done; > > } else if (so->so_error) { > > error = so->so_error; > > so->so_error = 0; > > SOCKBUF_UNLOCK(&so->so_snd); > > goto done; > > } > > > > But it isn't reached. Before due to SS_ISCONNECTED check, now > > due to your change. Now we got two spots where so_error is > > returned/cleared. > Do you mean that EPIPE could be returned from sendfile(2) after some > round of changes ? It should be not. > > EPIPE is a workaround for naive programs that use stdio and cannot detect > the broken pipe when used as an element in the shell plumbing. EPIPE > results in SIGPIPE terminating such programs (instead of looping endlessly > when write(2) returns error). > > Any application using sendfile(2) must understand the API to properly > handle > disconnects, so SIGPIPE workaround would be avoided. Often such > applications > are already sofisticated enough to block SIGPIPE, but if blocking becomes > a new requirement because EPIPE was not returned in earlier version of > the system, they might not block it still. > > > > > For the receive family (read(2) and recv(2)), SUS specifies > > ECONNRESET and our code does that. It also clears so_error. > > > > So, after your change at least one standard case is broken: an > > application that polls/selects for both readability and > > writeability of a socket. It discovers that socket is available > > for writing, does sendfile(2), receives ECONNRESET. Then it does > > read on the socket, and blocks(?) or returns a bogus error(?), > > not sure. But definitely not ECONNRESET, since it is now cleared. > > > > Now, where does this ENOTCONN come from? These two lines: > > > > if (((*so)->so_state & SS_ISCONNECTED) == 0) > > return (ENOTCONN); > > > > can be traced back all the way to original 1998 commit of sendfile. > > > > Here comes difference between sendfile(2) and send(2). In send(2), > > this check is done _after_ so_error check, so your change is correct > > in placing of so_error check wrt so_state check, but is incorrect > > in placing wrt socket buffer state check. > > > > After all this considerations, I would suggest to revert your change, > > and apply this patch. It would make behavior of sendfile(2) to match > > send(2). I would appreciate more reviews around it. > > > > -- > > Gleb Smirnoff > > > Index: kern_sendfile.c > > =================================================================== > > --- kern_sendfile.c (revision 324457) > > +++ kern_sendfile.c (working copy) > > @@ -507,13 +507,6 @@ sendfile_getsock(struct thread *td, int s, struct > > *so = (*sock_fp)->f_data; > > if ((*so)->so_type != SOCK_STREAM) > > return (EINVAL); > > - if ((*so)->so_error) { > > - error = (*so)->so_error; > > - (*so)->so_error = 0; > > - return (error); > > - } > > - if (((*so)->so_state & SS_ISCONNECTED) == 0) > > - return (ENOTCONN); > > return (0); > > } > > > > @@ -622,6 +615,12 @@ retry_space: > > SOCKBUF_UNLOCK(&so->so_snd); > > goto done; > > } > > + if ((so->so_state & SS_ISCONNECTED) == 0) { > > + SOCKBUF_UNLOCK(&so->so_snd); > > + error = ENOTCONN; > > + goto done; > > + } > > + > > space = sbspace(&so->so_snd); > > if (space < rem && > > (space <= 0 || > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAC27FbWSX1R_6jHo5Wjg9rbPY_tmiJGdJdic8EsxcvpS9T1TAQ>