Date: Sat, 06 Jan 2001 20:37:00 +0200
From: "Eli Zaretskii" <eliz AT is DOT elta DOT co DOT il>
Sender: halo1 AT zahav DOT net DOT il
To: "Richard Dawe" <rich AT phekda DOT freeserve DOT co DOT uk>
Message-Id: <8632-Sat06Jan2001203659+0200-eliz@is.elta.co.il>
X-Mailer: Emacs 20.6 (via feedmail 8.3.emacs20_6 I) and Blat ver 1.8.6
CC: djgpp-workers AT delorie DOT com
In-reply-to: <E14EtVu-0000G9-00@phekda.freeserve.co.uk>
	(rich AT phekda DOT freeserve DOT co DOT uk)
Subject: Re: /dev/zero & /dev/full FSEXTs
References:  <E14EtVu-0000G9-00 AT phekda DOT freeserve DOT co DOT uk>
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

> Date: Sat, 06 Jan 2001 12:43:52 +0000
> From: "Richard Dawe" <rich AT phekda DOT freeserve DOT co DOT uk>
> 
> Please find below a diff to add the /dev/zero and /dev/full FSEXTs
> to DJGPP.

Thanks.  I have a few comments.

> +     /* Ensure that we only have relevant flags. */
> +     data->flags &= (O_ACCMODE | O_BINARY | O_TEXT | O_NOINHERIT | O_NONBLOCK);

Why do you store O_BINARY and O_TEXT?  These are internal library
flags, and since there's no hook for `setmode', if the program
switches /dev/zero to a different mode, your flags will not be
correct.

> +   case __FSEXT_ready:
> +     /* This must be emulated, since the FSEXT has been called. */
> +     emul = 1;
> + 
> +     *rv  = __FSEXT_ready_read | __FSEXT_ready_write;    
> +     break;

Shouldn't /dev/full be NOT ready for writing?  What does `select'
return on Unix systems for the /dev/full device in this case?

> +   case __FSEXT_close:
[snip]
> +     /* Cope with dup()'d zero devices. */
> +     data->dup_count--;
> + 
> +     if (data->dup_count <= 0) {
> +       /* No longer referenced */
> +       free(data);
> +       _close(fd);
> +     }
> +     break;

This looks like a handle leak: you only close the handle when it is no
longer referenced.  However, internal_dup _always_ calls
__FSEXT_alloc_fd, which always produces a new handle (connected to the
NUL device).  So it looks like you don't close those additional
handles when the application closes them.

Or did I miss something?

> +   case __FSEXT_lseek:
> +     fd     = va_arg(args, int);
> +     offset = va_arg(args, off_t);
> +     whence = va_arg(args, int);    
> + 
> +     /* This must be emulated, since the FSEXT has been called. */
> +     emul = 1;
> + 
> +     *rv = offset;

Even if `offset' and `whence' point before the beginning?

> + #include <sys/xdevices.h>

Is this header used on other platforms?

> + When read, @file{dev/full} always returns @samp{\0} characters.  Writes
> + to @file{/dev/full} will fail with @var{errno} set to @code{ENOSPC}.

`errno' should have the @code markup, not @var, because it is not a
formal parameter but a specific variable name.