Mail Archives: djgpp-workers/2001/02/15/06:18:36
On Wed, 14 Feb 2001, Mark E. wrote:
> Below is a first try at a patch to support O_TEMPORARY currently supported by
> Cygwin and Mingw32. A file opened with O_TEMPORARY will be deleted after the
> last descriptor referring to it has been closed.
Thanks.
Are there any other platforms which support this functionality? How
do these platforms document the behavior of this flag? Where (in what
situations) is this feature useful?
Also, by ``Mingw32'' do you mean to say that this feature is supported
by MSVCRT.DLL (or whatever the C run-time support is called)?
> + void
> + _set_file_delete_on_close_attr(int fd, const char *file)
> + {
> + if (file)
> + {
> + char buffer[PATH_MAX + 1];
I think it's better to use FILENAME_MAX, as it is ANSI.
> + if (__o_temporary_files == NULL)
> + {
> + size_t size = 255 * sizeof(o_temporary_file_rec *);
> + __o_temporary_files = malloc(size);
> + memset(__o_temporary_files, 0, size);
> + }
This should use __bss_count, not __o_temporary_files being NULL, so
that it would DTRT in Emacs after dumping.
Also, the value returned by malloc above should be tested for being
non-NULL before you memset it.
> + _fixpath(file, buffer);
Why _fixpath and not _truename? _fixpath doesn't pay attention to
SUBSTed and JOINed drives. (OTOH, _truename can return a UNC, which
might fail in DOS calls, but that can be fixed with a few lines of
code.)
> + __o_temporary_files[fd] = (o_temporary_file_rec *)malloc(1 + len + 1);
> + __o_temporary_files[fd]->ref_count = 1;
> + strcpy(__o_temporary_files[fd]->filename, buffer);
The value returned by malloc should be tested for NULL.
> + else
> + {
> + if (__o_temporary_files[fd] == NULL)
> + return;
> + if (--(__o_temporary_files[fd]->ref_count) == 0)
> + {
> + remove(__o_temporary_files[fd]->filename);
> + free(__o_temporary_files[fd]);
> + }
> + __o_temporary_files[fd] = NULL;
> + }
This seems to plug NULL into an entry even if its ref_count is
non-zero.
> + void
> + _dup_file_delete_on_close_attr(int from, int to)
> + {
> + if (__o_temporary_files[to])
> + _set_file_delete_on_close_attr(to, NULL);
This does the effect of closing the file whose name happens to be in
__o_temporary_files[to]->filename. This is the Right Thing to do for
a `dup2' call, because DOS closes the other handle behind the scenes.
But I don't think it's right for `dup'. `dup' is supposed to produce
a fresh handle unused by any file, so __o_temporary_files[to] _must_
be NULL. If it is not NULL, you could either panic or silently ignore
it, but removing the file (whose name could be a left-over by some
bug, for example) is IMHO not right.
> + if (__o_temporary_files[from])
> + {
> + __o_temporary_files[to] = __o_temporary_files[from];
> + ++(__o_temporary_files[to]->ref_count);
> + }
> + }
I think this is wrong. After a call to `dup', you have one file open
on two different handles. The above code leaves the ref_count of the
old handle at 1, and sets the ref_count of the new handle to 2. This
means that if I close the old handle, the file will be deleted that
very moment--probably not what you want.
What you need to do instead is to handle this case like the OS does: a
`dup' call simply increments the reference count of the element in the
SFT array where the system keeps track of all open files.
Similar problems will happen with the same file open more than once
with the O_TEMPORARY attribute: when one of these handles is closed,
the file will be removed even though it is still open on the other
handle(s).
Another aspect that I think needs to be considered is the situation
where the same file is opened once with O_TEMPORARY, and then again
without O_TEMPORARY. Do we want the file to be removed when the first
(temporary) handle is closed? (This is one reason why I asked above
about the docs of this feature on other platforms.)
I think these considerations suggest that the data structure you use
to hold the information should be redesigned, to avoid these kinds of
lossage, and the code changed accordingly.
> + typedef struct
> + {
> + unsigned char ref_count __attribute__((packed));
> + char filename[0] __attribute__((packed));
> + } o_temporary_file_rec;
Why did you pack the struct? Wouldn't it be better to reverse the
order of the struct members and leave out the packed attribute?
- Raw text -