Date: Thu, 15 Feb 2001 13:16:03 +0200 (IST) From: Eli Zaretskii X-Sender: eliz AT is To: "Mark E." cc: djgpp-workers AT delorie DOT com Subject: Re: O_TEMPORARY In-Reply-To: <3A8A98EE.30071.A8D978@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 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?