Ambrose Feinstein wrote:
> Thanks for the quick fix!
>
> Aside from it requiring rearranging other code to not close the fd,
> was there a reason not to reuse a single temp file?
Actually that's what I started doing, but the change seemed like it'd
end up being bigger than I wanted for a bug fix. In retrospect, now
that I've done it and see the extent, it would have been ok.
Especially once I considered another factor: we want to avoid use of
functions like xmalloc and file_name_concat that call exit upon failure.
See the log on the second patch below for why.
>From fef2bc68e36c8891780311d8869db23753c093d0 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyer...@redhat.com>
Date: Tue, 18 Oct 2011 11:44:39 +0200
Subject: [PATCH 1/3] tac: use only one temporary file, with multiple
nonseekable inputs
* src/tac.c (temp_stream): New function, factored out of...
(copy_to_temp): ...here.
(tac_nonseekable): Don't free or fclose, now that we reuse the file.
---
src/tac.c | 120 ++++++++++++++++++++++++++++++++++++-------------------------
1 files changed, 71 insertions(+), 49 deletions(-)
diff --git a/src/tac.c b/src/tac.c
index 7d99595..6a7e510 100644
--- a/src/tac.c
+++ b/src/tac.c
@@ -418,53 +418,78 @@ record_or_unlink_tempfile (char const *fn, FILE *fp
ATTRIBUTE_UNUSED)
#endif
-/* Copy from file descriptor INPUT_FD (corresponding to the named FILE) to
- a temporary file, and set *G_TMP and *G_TEMPFILE to the resulting stream
- and file name. Return true if successful. */
-
+/* A wrapper around mkstemp that gives us both an open stream pointer,
+ FP, and the corresponding FILE_NAME. Always return the same FP/name
+ pair, rewinding/truncating it upon each reuse. */
static bool
-copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file)
+temp_stream (FILE **fp, char **file_name)
{
- static char *template = NULL;
- static char const *tempdir;
-
- if (template == NULL)
+ static char *tempfile = NULL;
+ static FILE *tmp_fp;
+ if (tempfile == NULL)
{
- char *t = getenv ("TMPDIR");
- tempdir = t ? t : DEFAULT_TMPDIR;
- template = file_name_concat (tempdir, "tacXXXXXX", NULL);
- }
+ char const *t = getenv ("TMPDIR");
+ char const *tempdir = t ? t : DEFAULT_TMPDIR;
+ tempfile = file_name_concat (tempdir, "tacXXXXXX", NULL);
+
+ /* FIXME: there's a small window between a successful mkstemp call
+ and the unlink that's performed by record_or_unlink_tempfile.
+ If we're interrupted in that interval, this code fails to remove
+ the temporary file. On systems that define DONT_UNLINK_WHILE_OPEN,
+ the window is much larger -- it extends to the atexit-called
+ unlink_tempfile.
+ FIXME: clean up upon fatal signal. Don't block them, in case
+ $TMPFILE is a remote file system. */
+
+ int fd = mkstemp (tempfile);
+ if (fd < 0)
+ {
+ error (0, errno, _("cannot create temporary file in %s"),
+ quote (tempdir));
+ goto Reset;
+ }
- /* FIXME: there's a small window between a successful mkstemp call
- and the unlink that's performed by record_or_unlink_tempfile.
- If we're interrupted in that interval, this code fails to remove
- the temporary file. On systems that define DONT_UNLINK_WHILE_OPEN,
- the window is much larger -- it extends to the atexit-called
- unlink_tempfile.
- FIXME: clean up upon fatal signal. Don't block them, in case
- $TMPFILE is a remote file system. */
-
- char *tempfile = xstrdup (template);
- int fd = mkstemp (tempfile);
- if (fd < 0)
- {
- error (0, errno, _("cannot create temporary file in %s"),
- quote (tempdir));
- free (tempfile);
- return false;
- }
+ tmp_fp = fdopen (fd, (O_BINARY ? "w+b" : "w+"));
+ if (! tmp_fp)
+ {
+ error (0, errno, _("cannot open %s for writing"), quote (tempfile));
+ close (fd);
+ unlink (tempfile);
+ Reset:
+ free (tempfile);
+ tempfile = NULL;
+ return false;
+ }
- FILE *tmp = fdopen (fd, (O_BINARY ? "w+b" : "w+"));
- if (! tmp)
+ record_or_unlink_tempfile (tempfile, tmp_fp);
+ }
+ else
{
- error (0, errno, _("cannot open %s for writing"), quote (tempfile));
- close (fd);
- unlink (tempfile);
- free (tempfile);
- return false;
+ if (fseek (tmp_fp, 0, SEEK_SET) < 0
+ || ftruncate (fileno (tmp_fp), 0) < 0)
+ {
+ error (0, errno, _("failed to rewind stream for %s"),
+ quote (tempfile));
+ return false;
+ }
}
- record_or_unlink_tempfile (tempfile, tmp);
+ *fp = tmp_fp;
+ *file_name = tempfile;
+ return true;
+}
+
+/* Copy from file descriptor INPUT_FD (corresponding to the named FILE) to
+ a temporary file, and set *G_TMP and *G_TEMPFILE to the resulting stream
+ and file name. Return true if successful. */
+
+static bool
+copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file)
+{
+ FILE *fp;
+ char *file_name;
+ if (!temp_stream (&fp, &file_name))
+ return false;
while (1)
{
@@ -477,26 +502,25 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int
input_fd, char const *file)
goto Fail;
}
- if (fwrite (G_buffer, 1, bytes_read, tmp) != bytes_read)
+ if (fwrite (G_buffer, 1, bytes_read, fp) != bytes_read)
{
- error (0, errno, _("%s: write error"), quotearg_colon (tempfile));
+ error (0, errno, _("%s: write error"), quotearg_colon (file_name));
goto Fail;
}
}
- if (fflush (tmp) != 0)
+ if (fflush (fp) != 0)
{
- error (0, errno, _("%s: write error"), quotearg_colon (tempfile));
+ error (0, errno, _("%s: write error"), quotearg_colon (file_name));
goto Fail;
}
- *g_tmp = tmp;
- *g_tempfile = tempfile;
+ *g_tmp = fp;
+ *g_tempfile = file_name;
return true;
Fail:
- fclose (tmp);
- free (tempfile);
+ fclose (fp);
return false;
}
@@ -512,8 +536,6 @@ tac_nonseekable (int input_fd, const char *file)
return false;
bool ok = tac_seekable (fileno (tmp_stream), tmp_file);
- fclose (tmp_stream);
- free (tmp_file);
return ok;
}
--
1.7.6.4
>From 1b9bb7a2d29cdf3977bbe23deca3cebe6c03bfaf Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyer...@redhat.com>
Date: Tue, 18 Oct 2011 12:04:02 +0200
Subject: [PATCH 2/3] tac: do not let failed allocation cause immediate exit
* src/tac.c (temp_stream): Don't exit immediately upon failed heap
allocation, here. That would inhibit processing of any additional
command-line arguments.
---
src/tac.c | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/src/tac.c b/src/tac.c
index 6a7e510..7cc562e 100644
--- a/src/tac.c
+++ b/src/tac.c
@@ -430,7 +430,12 @@ temp_stream (FILE **fp, char **file_name)
{
char const *t = getenv ("TMPDIR");
char const *tempdir = t ? t : DEFAULT_TMPDIR;
- tempfile = file_name_concat (tempdir, "tacXXXXXX", NULL);
+ tempfile = mfile_name_concat (tempdir, "tacXXXXXX", NULL);
+ if (tempdir == NULL)
+ {
+ error (0, 0, _("memory exhausted"));
+ return false;
+ }
/* FIXME: there's a small window between a successful mkstemp call
and the unlink that's performed by record_or_unlink_tempfile.
--
1.7.6.4
>From 424ed2cd9173ef6191bf5cf38fabc1ed7903c6ef Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyer...@redhat.com>
Date: Tue, 18 Oct 2011 14:20:36 +0200
Subject: [PATCH 3/3] maint: tac: prefer "failed to" diagnostic over "cannot"
---
src/tac.c | 6 +++---
tests/misc/tac | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/tac.c b/src/tac.c
index 7cc562e..36122b0 100644
--- a/src/tac.c
+++ b/src/tac.c
@@ -449,7 +449,7 @@ temp_stream (FILE **fp, char **file_name)
int fd = mkstemp (tempfile);
if (fd < 0)
{
- error (0, errno, _("cannot create temporary file in %s"),
+ error (0, errno, _("failed to create temporary file in %s"),
quote (tempdir));
goto Reset;
}
@@ -457,7 +457,7 @@ temp_stream (FILE **fp, char **file_name)
tmp_fp = fdopen (fd, (O_BINARY ? "w+b" : "w+"));
if (! tmp_fp)
{
- error (0, errno, _("cannot open %s for writing"), quote (tempfile));
+ error (0, errno, _("failed to open %s for writing"), quote
(tempfile));
close (fd);
unlink (tempfile);
Reset:
@@ -569,7 +569,7 @@ tac_file (const char *filename)
fd = open (filename, O_RDONLY | O_BINARY);
if (fd < 0)
{
- error (0, errno, _("cannot open %s for reading"), quote (filename));
+ error (0, errno, _("failed to open %s for reading"), quote
(filename));
return false;
}
}
diff --git a/tests/misc/tac b/tests/misc/tac
index 7bd07bd..5602128 100755
--- a/tests/misc/tac
+++ b/tests/misc/tac
@@ -68,7 +68,7 @@ my @Tests =
{ENV => "TMPDIR=$bad_dir"},
{IN_PIPE => "a\n"},
{ERR_SUBST => "s,`$bad_dir': .*,...,"},
- {ERR => "$prog: cannot create temporary file in ...\n"},
+ {ERR => "$prog: failed to create temporary file in ...\n"},
{EXIT => 1}],
# coreutils-8.5's tac would double-free its primary buffer.
--
1.7.6.4
