Re: [bug-gnulib] clean-temp usage question

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Bruno Haible on 10/6/2006 6:14 AM:

Comments just from reading the source:

> 
>> fwriterror is still controversial for the reasons Paul mentioned
> 
> But I need it in GNU gettext.

I think I misunderstood you in my previous mail.  Implementing
fwriteerror_temp is the correct thing to do if you use it in gettext.  And
I just noticed how you made it conditional on the use of the fwriteerror
module.

> +    Also, open file descriptors need to be closed before the temporary files
> +    and the temporary directories can be removed, because only on Unix
> +    (excluding Cygwin) one can remove directories containing open files.

Grammar: s/one can/can one/

> --- gnulib-20060928-modified/lib/clean-temp.c 2006-10-06 03:37:05.000000000 
> +0200
> ***************
> *** 41,46 ****
> --- 41,50 ----
>   
>   #define _(str) gettext (str)
>   
> + #ifndef uintptr_t
> + # define uintptr_t unsigned long
> + #endif
> + 

Can't we just get this by including one of the gnulib headers that
guarantees the correct types?  I'm a bit worried that there might be a
platform with uintptr_t typedef'd to a different size integer, at which
point this #define is broken.  So relying on a header and autoconf test to
check for the presence of uintptr_t would be safer.

> + 
> + /* Open a temporary file in a temporary directory.
> +    Registers the resulting file descriptor to be closed.  */
> + int
> + open_temp (const char *file_name, int flags, mode_t mode)
> + {
> +   int fd;
> +   int saved_errno;
> + 
> +   block_fatal_signals ();
> +   fd = open (file_name, flags, mode);

Could you please use the fcntl-safer module?  Otherwise, this call will
break M4's testsuite which has checks for invocation with any of the std
fds starting life closed; the temp file is created in the slot that should
remain empty, and detectable behavior differences result.  And I can't use
fd_safer on the result of open_temp, because the replacement fd returned
by fd_safer would not be registered, so it has to be done inside
open_temp.  You would then update the comments to state that the resulting
fd is guaranteed to be greater than 2 on success.  Do you want me to
provide this patch?

> + 
> + /* Open a temporary file in a temporary directory.
> +    Registers the resulting file descriptor to be closed.  */
> + FILE *
> + fopen_temp (const char *file_name, const char *mode)
> + {
> +   FILE *fp;
> +   int saved_errno;
> + 
> +   block_fatal_signals ();
> +   fp = fopen (file_name, mode);

Likewise with the fopen-safer module.

> +   saved_errno = errno;
> +   if (fp != NULL)
> +     {
> +       /* It is sufficient to register fileno (fp) instead of the entire fp,
> +      because at cleanup time there is no need to do an fflush (fp); a
> +      close (fileno (fp)) will be enough.  */

At fatal signal time, yes, calling fflush is unnecessary - the program is
going to die from the signal anyways, so losing unflushed data does not
hurt my feelings.  But it does mean that I must manually fflush/fclose all
returned FILE*s before calling cleanup_temp_subdir (although it is good
practice to do that anyways).

> +       int fd = fileno (fp);
> +       if (!(fd >= 0))
> +     abort ();
> +       register_fd (fd);
> +     }
> +   unblock_fatal_signals ();
> +   errno = saved_errno;
> +   return fp;
> + }
> + 
> + /* Close a temporary file in a temporary directory.
> +    Unregisters the previously registered file descriptor.  */
> + int
> + close_temp (int fd)
> + {
> +   if (fd >= 0)
> +     {
> +       /* No blocking of signals is needed here, since a double close of a
> +      file descriptor is harmless.  */
> +       int result = close (fd);
> +       int saved_errno = errno;
> + 
> +       /* No race condition here: we assume a single-threaded program, hence
> +      fd cannot be re-opened here.  */

State that assumption in the comments for create_temp_dir.

Is it worth documenting that given a FILE* from fopen_temp, I can do:
fd=fileno(fp); close_stream(fp); close_temp(fd);
rather than:
fclose_temp(fp);
Once fp is closed, I can't call fclose_temp because the storage underlying
fp is no longer valid, but if you provide a guarantee that I only have to
unregister the fd, then I can choose between close_stream (which M4 is
already using) or converting over to fwriteerror for disposing of
temporary FILE* objects.

- --
Life is short - so eat dessert first!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.1 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFFJlTj84KuGfSFAYARAotCAKCT5z5LrAw/cVm7hRmGyNck67RyQACdGF+2
e/P39RMblaIkZdHdqAR5t+k=
=BEc5
-----END PGP SIGNATURE-----