Date: Wed, 14 Mar 2001 09:42:55 +0200 (IST)
From: Eli Zaretskii <eliz AT is DOT elta DOT co DOT il>
X-Sender: eliz AT is
To: "Mark E." <snowball3 AT bigfoot DOT com>
cc: djgpp-workers AT delorie DOT com
Subject: Re: zero fill the eof gap (complete patch)
In-Reply-To: <3AAE27DD.8570.57C673@localhost>
Message-ID: <Pine.SUN.3.91.1010314093834.22222E-100000@is>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Reply-To: djgpp-workers AT delorie DOT com
Errors-To: nobody AT delorie DOT com
X-Mailing-List: djgpp-workers AT delorie DOT com
X-Unsubscribes-To: listserv AT delorie DOT com
Precedence: bulk


On Tue, 13 Mar 2001, Mark E. wrote:

> I assume a blurb for wc204.txi 
> is appropriate since this solves a problem that keeps coming up on c.o.m.d.

Yes.  I think the docs of `write' and `_write' should also mention
this side effect.

> +   if (__has_fd_properties(handle)
> +       && (__fd_properties[handle]->flags & FILE_DESC_FILL_TEST))
> +   {
> +     if (_write_fill_seek_gap(handle) < 0)
> +       return -1;
> +   }

I wonder whether we should really fail the operation if we cannot
zero-fill the gap.  This seems like a subtly incompatible change: what
if the file/device we are writing to doesn't support some of the
trickery you do inside `_write_fill_seek_gap'?  This might break
programs which worked until now and which don't care about leaving
garbage in the gaps.

> + int
> + _write_fill_seek_gap(int fd)
> + {
> +   offset_t eof_off, cur_off, fill_count;
> +   unsigned long tbsize, buf_size;
> +   unsigned long i, write_size, out_count;
> +   __dpmi_regs r;
> + 
> +   __clear_fd_flags(fd, FILE_DESC_FILL_TEST);
> +   
> +   if (isatty(fd))
> +     return -1;

This is really harsh, IMHO: unless I'm mistaken, it will make all
writes to a terminal device fail if they dare to lseek it first.

I think you should simply return zero here: it doesn't make sense to
zero-fill gaps in writes to terminal devices.

> +   /* Quit when unable to get the file length.  */    
> +   eof_off = lfilelength (fd);
> +   if (eof_off < 0)
> +     return -1;
> +   
> +   /* Quit when unable to get the current file offset.  */
> +   cur_off = llseek (fd, 0, SEEK_CUR);
> +   if (cur_off < 0)
> +     return -1;
> + 
> +   /* Quit (but return success) if the current offset is not past EOF.  */
> +   if (cur_off <= eof_off)
> +     return 0;
> +     
> +   /* Quit when unable to seek to EOF.  */
> +   if (llseek (fd, eof_off, SEEK_SET) == -1)
> +     return -1;

These failures are also dangerous with handles that don't support
seeking.  However, if you don't fail `write' and `_write' when
`_write_fill_seek_gap' fails, perhaps it's okay to return -1 here.

> +   /* Fill the transfer buffer with zeros.  */
> +   tbsize = _go32_info_block.size_of_transfer_buffer;
> +   fill_count = cur_off - eof_off;
> + 
> +   buf_size = (fill_count > tbsize) ? tbsize : fill_count;
> + 
> +   i = 0;
> +   _farsetsel(_dos_ds);
> +   while (i < buf_size)
> +   {
> +     _farnspokel(__tb + i, 0);
> +     i += 4;
> +   }
> + 
> +   /* Write out the zeros.  */
> +   out_count = 0;
> +   do
> +   {
> +     r.x.ax = 0x4000;
> +     r.x.bx = fd;
> +     write_size = (fill_count > buf_size) ? buf_size : fill_count;
> +     r.x.cx = write_size;
> +     r.x.dx = __tb & 15;
> +     r.x.ds = __tb / 16;
> +     __dpmi_int (0x21, &r);
> +     if (r.x.flags & 1)
> +     {
> +       errno =__doserr_to_errno (r.x.ax);
> +       return -1;
> +     }
> +     fill_count -= r.x.ax;
> +     out_count += r.x.ax;
> +   } while (fill_count && (write_size == r.x.ax));
> + 
> +   if (fill_count && out_count == 0)
> +   {
> +     errno = ENOSPC;
> +     return -1;
> +   }
> + 
> +   return 0;
> + }

This is a duplicate of the code in `_write'.  I wonder whether it
would be better to make this a separate function which both `_write'
and `_write_fill_seek_gap' could call.  That would avoid having two
almost identical code fragments, which adds to maintenance costs.

One other aspect of this change bothers me: it looks like FSEXT
handles will not be handled consistently wrt the flags in
__fd_properties.  For example, you call lseek, which might be hooked
by an FSEXT, but write the zeroes with direct DOS calls.  It would
make sense to do that if we make sure that no FSEXT handle will ever
have any bits set in __fd_properties.  Is it true that no function
which sets bits in __fd_properties will ever do that for an FSEXT
handle?

Finally, this code in lseek:

> +   if (__has_fd_properties(handle))
> +   {
> +     /* Set the fill test flag when the file pointer may move past EOF.
> +        Clear the fill test flag when the file pointer can't be past EOF. */
> +     if (offset > 0)
> +       __set_fd_flags(handle, FILE_DESC_FILL_TEST);
> +     else if (whence == SEEK_SET || whence == SEEK_END)
> +       __clear_fd_flags(handle, FILE_DESC_FILL_TEST);
> +   }

Seems to suggest that __fd_properties will have to be set for a handle
before it can use the gap filling feature.  But where is the code
which sets up __fd_properties for any given handle?  Does this imply
that an application which wants this feature needs to do something
special with each handle that needs zero-filling?  Or am I missing
something?