short read/write and error code

[David Xu]

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.

-------------- next part --------------
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