Date: Sun, 20 Apr 2003 13:22:08 +0200
From: "Eli Zaretskii" <eliz AT elta DOT co DOT il>
Sender: halo1 AT zahav DOT net DOT il
To: rich AT phekda DOT freeserve DOT co DOT uk
Message-Id: <3791-Sun20Apr2003132207+0300-eliz@elta.co.il>
X-Mailer: emacs 21.3.50 (via feedmail 8 I) and Blat ver 1.8.9
CC: djgpp-workers AT delorie DOT com
In-reply-to: <E196rAk-0007Mp-00@phekda.freeserve.co.uk>
	(rich AT phekda DOT freeserve DOT co DOT uk)
Subject: Re: fstat, fd_props and inventing inodes, revision 3 [PATCH]
References:  <E196rAk-0007Mp-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, 19 Apr 2003 13:14:46 +0100
> From: "Richard Dawe" <rich AT phekda DOT freeserve DOT co DOT uk>
> 
> Here's revision 3 of the patch to fstat, to make it use the filename
> from fd_props to generate an inode number, when it can't obtain the inode
> from the SFT.

Thanks.  I have a few minor comments.

> +/* Find a mapping for the UNC path (\\machine\share) in `src', if one
> + * exists.  If one exists, copy the path into `dest' and convert the UNC
> + * to the mapped drive, then return `dest'.  Otherwise, just return `src'.
> + */
> +
> +static const char *
> +find_mapping_for_unc (const char *src, char *dest)

This sounds like a good function to have in the library, so I'd
suggest to make it a separate module and make the function be extern
(and then its name should begin with 2 underscores).

> +  /* Update the drive mappings, if: we don't have them; a second has
> +   * elapsed; the clock has wrapped.  We update every second as a basic
> +   * caching mechanism, but also to keep up with changes in the mappings.

This caching, like all caches we have in the library, should have the
standard unexec protection using __bss_count.  That is, if the program
is dumped and restarted, the cache should be invalidated.

> +  /* Get the file name from the file descriptor properties (fd_props),
> +   * if possible, and fix it up. */
> +  if (__get_fd_name(fhandle))
> +    filename = __get_fd_name(fhandle);

Is it really necessary to call __get_fd_name twice?  Sounds like a
waste of cycles, even if it is inlined.

> +invented using the file name.  @code{fstat} can probably use the file name
> +that was used to open the file, when generating the inode.  This is done
> +such that the same inode will be generated irrespective of the actual path
> +used to open the file (e.g.: @samp{foo.txt}, @samp{./foo.txt},
> +@samp{../somedir/foo.txt}).  If file names cannot be used, @code{fstat}

File names should be in @file, not @samp.

> +@findex fstat AT r{, and inodes}
> +@code{fstat} will now use the file name used to open the file,
> +when inventing inodes.  This is done so that the same inode is generated
> +irrespective of the actual file path used to open the file.  This also
> +fixes the problem where multiple calls to fstat
> +on the same file descriptor would give different inodes.

I think it's important to mention that this change makes sure `stat'
and `fstat' report the same inode in most cases.

> +    if (drive_mappings[pos] == NULL)
> +      drive_mappings[pos] = malloc(sizeof(drive_mapping));

As you probably already know, I don't like library functions to call
malloc.  Can't we have a static 32-element array for this?  Or would
the required storage be unresonably large?

> +path of the form @file{\\machine\share}.

IIRC, backslashes have problems when you TeX the manual.  I think the
FAQ sources have the solution (search for "\\") which could help you
out.