Re: Completely unexplicable (to me ;) behaviour of read (vs fread)

[Nathan Myers <ncm-nospam at cantrip dot org>]

On Sat, Mar 08, 2003 at 09:17:04PM +0100, Paolo Carlini wrote:
> regtesting the fix for 7744, which I'm preparing along the
> lines already discussed in detail, I'm seeing completely
> unexpected failures in filebuf_virtuals.cc.
> 
> Consider this sequence:
> 
>  fb_03.pubseekoff(0, std::ios_base::end);
>  c1 = fb_03.sgetc();
>  c2 = fb_03.sungetc();
> 
> Sungetc() calls pbackfail() which, then, calls underflow here:
> 
>   // At the beginning of the buffer, need to make a
>   // putback position available.
>   // But the seek may fail (f.i., at the beginning of
>   // a file, see libstdc++/9439) and in that case
>   // we return traits_type::eof()
>   if (this->seekoff(-1, ios_base::cur) >= 0)
>     {
>       this->underflow();

This looks deeply strange.  Doing a seek in pbackfail seems absurdly
expensive, by itself.  Doing an underflow afterward is strange too, 
and wasteful; maybe nobody will use those characters it reads.  The 
sensible thing to do would be to stash the character in a holding cell 
and point the get and put pointers at it.  If it gets looked at again,
then eventually underflow() will get called legitimately, and can 
clean up, i.e. pointing back to the real buffer it has filled, etc.
(This would be as I described in my private mail to you.)  

The best way to fix bugs is to delete the code they are in.

> Inside underflow the new _M_file.xsgetn() is called, which, when
> the last parameter is false means read instead of fread:
> 
>   __elen = _M_file.xsgetn(reinterpret_cast<char*>(_M_in_beg),
>                           _M_buf_size, false);

Are we overloading xsgetn now?  It seems unwise to overload a virtual
with a non-virtual function.  

> The new xsgetn is:
> 
>  __basic_file<char>::xsgetn(char* __s, streamsize __n, bool __stdio)
>  {
>    if (__stdio)
>      return fread(__s, 1, __n, _M_cfile);
>    else
>      {
>        streamsize __ret;
>        do
>          {
>            errno = 0;
>            __ret = read(this->fd(), __s, __n);
>          }
>        while (__ret == -1L && errno == EINTR);
>        return __ret;
>      }
>   }

First, there's no reason to set errno before the call.  Either read() 
succeeds and you don't care what's in errno, or it fails and sets 
errno.  This is not an idle remark; errno is a macro that might expand 
to an arbitrarily expensive function call (e.g. to locate thread-local 
storage) itself.   You only want to touch errno when there has been an 
error.

> The big mistery is that read (vs fread) in this situation returns _zero_
> instead of one! No chars are read.
> P.S. I have already tried clearing O_NONBLOCK (as will eventually be the
> case, after the fix for 9533), with no differences.

Hmm, read() _only_ returns 0 at EOF or if that __n argument is zero.

Nathan Myers
ncm-nospam at cantrip dot org