comments on proposed uthread_write.c changes

[Daniel Eischen]

[ Redirected to -standards ]

Synopsis:

  libc_r wraps write() and changes a blocking file descriptor
  to non-blocking behind the scenes.   When __sys_write()
  returns less than the number of bytes requested, libc_r's
  write() continues looping trying to finish the requested
  write.

Problem:

  A return of 0 for a write() to a tape device means end-of-file.
  Libc_r's write treats a return of 0 from __sys_write() as a
  partial write and continues looping.  The end-of-file is
  passed and a few more blocks may be written before the
  physical end-of-tape is reached.

  [ In this context, end-of-file means a the shiny end-of-tape
    marker on the tape (at least that's what it looks like on
    7 & 9 track tapes -- am I dating myself?).  It doesn't mean
    an EOF marker. ]

I've been reading the POSIX spec in regards to write():

  http://www.opengroup.org/onlinepubs/007904975/toc.htm

particularly the Rationale at the end.  I think it is OK
for libc_r to special case a return of 0 from __sys_write()
and pass it back to the caller.

Any comments?

On Sun, 7 Sep 2003, Dan Langille wrote:

> On 7 Sep 2003 at 12:32, Daniel Eischen wrote:
> 
> > On Sun, 7 Sep 2003, Dan Langille wrote:
> > 
> > > A problem with pthreads and EOT has been identified.  See PR 56274.  It
> > > was suggested the solution was probably just a matter of changing one of
> > > the >0 tests to >=0 in uthread_write.c
> > > 
> > > Any comments on that?
> > 
> > I don't know that a return of 0 isn't valid for other devices.
> > If this is the case, a return of 0 for blocking writes may break
> > other applications.
> > 
> > The patch isn't quite correct (at least looking at -current srcs).
> > Lines 98-99 are:
> > 
> > 			if (blocking && ((n < 0 && (errno == EWOULDBLOCK ||
> > 			    errno == EAGAIN)) || (n >= 0 && num < nbytes))) {
> > 
> > This will get entered first if n == 0, and I don't think your
> > proposed patch would have any effect.  I think you would have
> > to change the "n >= 0" above to be "n > 0" in conjunction with
> > your patch.
> 
> Ahh thank you.  That explains why the test results with the original 
> patch did not differ from -STABLE or 5.1-RELEASE.  After adding your 
> suggestions, we have had success.
> 
> The points to note:
> 
> 1. The status that stopped the writing was 0
> 2. It wrote 17,256 blocks, and read 17,256 blocks.
> 
> Point 1 is key to determining EOT.  Point 2 is what you always want 
> to have...
> 
> [dan at undef:~/tape-test] $ sudo ./tapetest /dev/nsa0
> *rewind
> Rewound /dev/nsa0
> *rawfill
> Begin writing blocks of 64512 bytes.
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++
> weof_dev
> Wrote EOF to /dev/nsa0
> Write failed.  Last block written=17256. stat=0 ERR=Unknown error: 0
> *rewind
> Rewound /dev/nsa0
> *scan
> Starting scan at file 0
> 17256 blocks of 64512 bytes in file 0
> End of File mark.
> End of File mark.
> End of tape
> Total files=1, blocks=17256, bytes = 1113219072
> *
> 
> > This could be solved at the application level by selecting on
> > the tape device and performing non-blocking writes to it.  Since
> > the application knows that a return of 0 is end-of-tape, it
> > must also know the difference between talking to a tape device
> > and talking to a regular file, socket, etc.
> 
> True.  But *if* the code is wrong, it should be fixed.
> 
> FWIW, the patch follows.  As always, opinions and suggestions are 
> welcome.
> 
> --- uthread_write.c.org	Sun Sep  7 10:58:31 2003
> +++ uthread_write.c	Sun Sep  7 15:41:34 2003
> @@ -93,7 +93,7 @@
>  			 * write:
>  			 */
>  			if (blocking && ((n < 0 && (errno == EWOULDBLOCK ||
> -			    errno == EAGAIN)) || (n >= 0 && num < nbytes))) {
> +			    errno == EAGAIN)) || (n > 0 && num < nbytes))) {
>  				curthread->data.fd.fd = fd;
>  				_thread_kern_set_timeout(NULL);
>  
> @@ -131,7 +131,7 @@
>  			 * If there was an error, return partial success
>  			 * (if any bytes were written) or else the error:
>  			 */
> -			} else if (n < 0) {
> +			} else if (n <= 0) {
>  				if (num > 0)
>  					ret = num;
>  				else
> 
> -- 
> Dan Langille : http://www.langille.org/
> 

-- 
Dan Eischen