Date: Wed, 14 Mar 2001 09:42:55 +0200 (IST) From: Eli Zaretskii X-Sender: eliz AT is To: "Mark E." 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: 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?