Date: Fri, 23 Feb 2001 12:05:49 +0200
From: "Eli Zaretskii" <eliz AT is DOT elta DOT co DOT il>
Sender: halo1 AT zahav DOT net DOT il
To: "Mark E." <snowball3 AT bigfoot DOT com>
Message-Id: <5567-Fri23Feb2001120549+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: <3A95981A.14546.31CA2@localhost> (snowball3@bigfoot.com)
Subject: Re: O_TEMPORARY v2
References:  <3A95981A DOT 14546 DOT 31CA2 AT localhost>
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

> From: "Mark E." <snowball3 AT bigfoot DOT com>
> Date: Thu, 22 Feb 2001 22:52:10 -0500
> 
> Revision two of O_TEMPORARY support is ready for inspection.

Thanks!  Apart of a few minor comments below, I think this can go in.

> I have tested this the following small program:

Perhaps you could add this test program to djtstNNN.zip.

> + typedef struct fd_properties fd_properties;
> + 
> + typedef struct fd_properties
> + {
> +   unsigned char ref_count;
> +   unsigned long flags;
> +   char *filename;
> +   fd_properties *prev;
> +   fd_properties *next;
> + };

Shouldn't the second typedef simply be "struct fd_properties", without
"typedef"?

> + /* Find another properties object using the same filename.  */
> + static
> + fd_properties * find_eq_filename(const fd_properties *fd)
> + {
> +   fd_properties *ptr = active_fds;
> +   
> +   while (ptr)
> +   {
> +     if ((ptr != fd) && (strcmp(fd->filename, ptr->filename) == 0))
> +       return ptr;
> +     ptr = ptr->next;
> +   }
> +   return NULL;
> + }

I wonder whether we should use stricmp here.  Yes, I know: _truename
canonicalizes the letter case, but you know... paranoia...

(If you do change this, don't forget to #include <libc/stubs.h>.)

> + */ static fd_properties * alloc_fd_properties() {
> +   fd_properties *ptr;

This is not according to the braces and indentatioin style.

> + static void free_fd_properties(fd_properties *fd) {
> +   remove_1ist(&active_fds, fd);
> +   insert_1ist(&cached_fds, fd);
> + }

Same here.

> *************** open(const char* filename, int oflag, ..
> *** 152,157 ****
> --- 153,161 ----
>     if(oflag & O_APPEND)
>       lseek(fd, 0, SEEK_END);
> 
> +   if (oflag)
> +     _set_fd_properties(fd, real_name, oflag);
> +     

Isn't it better to put this into _open instead of open?  The opposite
operation is inside _close, not close.

Finally, the docs part is missing.