Date: Thu, 02 Aug 2012 19:53:47 +0800 From: David Xu <listlog2011@gmail.com> To: davidxu@freebsd.org Cc: Konstantin Belousov <kostikbel@gmail.com>, arch@freebsd.org Subject: Re: short read/write and error code Message-ID: <501A6A4B.8060209@gmail.com> In-Reply-To: <501A69EB.9000701@gmail.com> References: <5018992C.8000207@freebsd.org> <20120801071934.GJ2676@deviant.kiev.zoral.com.ua> <20120801183240.K1291@besplex.bde.org> <20120801162836.GO2676@deviant.kiev.zoral.com.ua> <20120802040542.G2978@besplex.bde.org> <20120802100240.GV2676@deviant.kiev.zoral.com.ua> <501A69EB.9000701@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
--------------020705050204070000090402
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
On 2012/8/2 19:52, David Xu wrote:
> On 2012/8/2 18:02, Konstantin Belousov wrote:
>> diff --git a/sys/kern/sys_pipe.c b/sys/kern/sys_pipe.c
>> index 338256c..1a61b93 100644
>> --- a/sys/kern/sys_pipe.c
>> +++ b/sys/kern/sys_pipe.c
>> @@ -1286,13 +1286,13 @@ pipe_write(fp, uio, active_cred, flags, td)
>> }
>> /*
>> - * Don't return EPIPE if I/O was successful
>> + * Don't return EPIPE if any byte was written.
>> + * EINTR and other interrupts are handled by generic I/O layer.
>> + * Do not pretend that I/O succeeded for obvious user error
>> + * like EFAULT.
>> */
>> - if ((wpipe->pipe_buffer.cnt == 0) &&
>> - (uio->uio_resid == 0) &&
>> - (error == EPIPE)) {
>> + if (uio->uio_resid != orig_resid && error == EPIPE)
>> error = 0;
>> - }
>> if (error == 0)
>> vfs_timestamp(&wpipe->pipe_mtime);
>
> I dislike the patch, if I thought it is right, I would have already
> submit such
> a patch. Now let us see why your patch is wore than my version
> (attached):
> -current:
> short write is done, some bytes were sent
> dofilewrite returns -1, errno is EPIPE
> dofilewrite sends SIGPIPE
> application is killed by SIGPIPE
> -my attached version:
> short write is done, some bytes were sent
> dofilewrite return number of bytes sent, errno is zero
> dofilewrite sends SIGPIPE.
> application is killed by SIGPIPE
> -you version:
> short write is done, some bytes were sent.
> dofilewrite returns number of bytes sent, errno is zero.
> dofilewrite does not send SIGPIPE signal
> application is not killed
> application does not check return value from write()
> application thinks it is successful, application does other things,
> application might begin a bank account transaction.
> ...
> application never comes back...
>
> my patch is more compatible with -current. if application does not
> setup a signal handler for SIGPIPE, when short write happens, it is
> killed,
> it is same as -current. if the application set up a signal handler for
> the signal,
> it always should check the return value from write(), this is how
> traditional
> code works.
>
> in your patch, short write does not kill application, you can not assume
> that the application will request a next write() on the pipe, and you
> hope
> the second write to kill the application, but there is always exception,
> the next write does not happen, application works on other things.
> This is too incompatible with -current.
>
>
Oops, I forgot to attach it.
--------------020705050204070000090402
Content-Type: text/plain; charset=gb18030;
name="sys_generic_short_pipe_write.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment; filename="sys_generic_short_pipe_write.diff"
Index: sys_generic.c
===================================================================
--- sys_generic.c (revision 238927)
+++ sys_generic.c (working copy)
@@ -539,15 +539,21 @@
if (fp->f_type == DTYPE_VNODE)
bwillwrite();
if ((error = fo_write(fp, auio, td->td_ucred, flags, td))) {
- if (auio->uio_resid != cnt && (error == ERESTART ||
- error == EINTR || error == EWOULDBLOCK))
- error = 0;
/* Socket layer is responsible for issuing SIGPIPE. */
if (fp->f_type != DTYPE_SOCKET && error == EPIPE) {
PROC_LOCK(td->td_proc);
tdsignal(td, SIGPIPE);
PROC_UNLOCK(td->td_proc);
}
+ if (auio->uio_resid != cnt) {
+ if (error == ERESTART || error == EINTR ||
+ error == EWOULDBLOCK)
+ error = 0;
+ else if (error == EPIPE &&
+ (fp->f_type == DTYPE_PIPE ||
+ fp->f_type == DTYPE_FIFO))
+ error = 0;
+ }
}
cnt -= auio->uio_resid;
#ifdef KTRACE
--------------020705050204070000090402--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?501A6A4B.8060209>