Re: temp file suffixes: mktemp DWIM
Eric Blake wrote:
> Now that glibc 2.11 has mkostemps, and I'm working on adding that to gnulib,
> it
Good to hear it.
> would be nice to expose the idea of an explicit suffix to temporary file
> names. But rather than require the user to count how long their suffix is, I
> imagine it would make more sense to give mktemp(1) some do-what-I-mean smarts.
> That is, I envision:
>
> mktemp foo-XX.txt
>
> calling mk[o]stemps("foo-XX.txt",4,0), automatically figuring out that the
> final run of XX in the template implies a suffix of length 4, to create a
> file such as "foo-abcdef.txt".
>
> Maybe it's even worth an option in case a user wants an explicit X in the
> suffix, as in:
>
> mktemp --suffix-len=1 foo-XXX
Here's another way to do it:
mktemp --suffix=X foo-XX
> to create a file named foo-abcdefX, although that starts to be a bit of
> overkill. Meanwhile, if the user supplies less than 6 X, I think it would
> also
> be nice to supply the missing X rather than facing an EINVAL from too few X,
> as
> in:
>
> mktemp foo-
>
> calling mkstemp("foo-XX").
I disagree. IMHO, "mktemp foo-" is an error, and requires a diagnostic
and nonzero exit.
> Then again, this would be a slight change from
> the current codebase, since 'mktemp foo-XXX' currently creates a file such
> as "foo-abc" rather than giving EINVAL or creating "foo-abcdef".
>
> Finally, is there any reason that we guarantee that more than six X will be
> munged, even though glibc munges only the last six X? In other words, since
GNU mktemp does that because the original mktemp did that.
Who knows, someone may actually require more than 6 random bytes.
I for one appreciate not having to have 6 all the time.
> the default template is tmp.XX, do we want to continue to guarantee
> that we generate a file such as tmp.abcdef1234,
Why change?
> or are we okay with generating
> tmp.abcdef? Or do we even shorten the default template to just six X,
> since that seems to be enough to guarantee success.
>
> Thoughts?
Re: temp file suffixes: mktemp DWIM
Jim Meyering meyering.net> writes:
> > the default template is tmp.XX, do we want to continue to guarantee
> > that we generate a file such as tmp.abcdef1234,
>
> Why change?
Good point. As part of my gnulib work, I guess that means I'll have to
regenerate the coreutils patch to gen_tempname that lets us continue replacing
an arbitrary number of X (as long as it is at least 3), rather than the
gnulib/glibc approach of exactly 6.
> > I imagine it would make more sense to give mktemp(1) some do-what-I-mean
smarts.
> > That is, I envision:
> >
> > mktemp foo-XX.txt
> >
> > calling mk[o]stemps("foo-XX.txt",4,0), automatically figuring out that
the
> > final run of XX in the template implies a suffix of length 4, to create
a
> > file such as "foo-abcdef.txt".
> >
> > Maybe it's even worth an option in case a user wants an explicit X in the
> > suffix, as in:
> >
> > mktemp --suffix-len=1 foo-XXX
>
> Here's another way to do it:
>
> mktemp --suffix=X foo-XX
Nice idea. Putting it all together, how about the following semantics:
mktemp a => error, no run of X
mktemp aXX => error, run of X is too short
mktemp XXX => generates 3-character name (if possible)
mktemp aXXX.b => generates 6-character name (if possible)
mktemp --suffix=.b aXXX => longer spelling of the above line
mktemp aXXX --suffix=.b => likewise, if not POSIXLY_CORRECT
mktemp aXXXbXXX => generates aXXXb123 (only the last run changed)
mktemp --suffix=X aXXX => only way to generate a123X instead of a1234
mktemp --suffix=.txt file => error, no run of X
mktemp --suffix=.txt aXXXb => error, explicit --suffix requires trailing X
The changes from the existing behavior:
Addition of a --suffix option (does it warrant a short option? Probably not
until it has had more field testing). Explicit use of this argument requires
trailing X in the template.
Addition of DWIM magic if --suffix is not present. Currently, mktemp aXXX.b
errors out, stating that there are too few X's in the template. The new
behavior recognizes any run of X, not just at the tail, and this goes a long
way to meet user expectations:
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=548316
--
Eric Blake
Re: temp file suffixes: mktemp DWIM
Eric Blake wrote: ... >> Here's another way to do it: >> >> mktemp --suffix=X foo-XX > > Nice idea. Putting it all together, how about the following semantics: > > mktemp a => error, no run of X > mktemp aXX => error, run of X is too short > mktemp XXX => generates 3-character name (if possible) > mktemp aXXX.b => generates 6-character name (if possible) > mktemp --suffix=.b aXXX => longer spelling of the above line > mktemp aXXX --suffix=.b => likewise, if not POSIXLY_CORRECT > mktemp aXXXbXXX => generates aXXXb123 (only the last run changed) > mktemp --suffix=X aXXX => only way to generate a123X instead of a1234 > mktemp --suffix=.txt file => error, no run of X > mktemp --suffix=.txt aXXXb => error, explicit --suffix requires trailing X Nice, comprehensive set of examples. > The changes from the existing behavior: > > Addition of a --suffix option (does it warrant a short option? Probably not > until it has had more field testing). Explicit use of this argument requires > trailing X in the template. I agree. > Addition of DWIM magic if --suffix is not present. Currently, mktemp aXXX.b > errors out, stating that there are too few X's in the template. The new > behavior recognizes any run of X, not just at the tail, and this goes a long > way to meet user expectations: > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=548316 All sounds good. Thanks!
Re: temp file suffixes: mktemp DWIM
Eric Blake byu.net> writes: > Now that glibc 2.11 has mkostemps, and I'm working on adding that to gnulib, it > would be nice to expose the idea of an explicit suffix to temporary file > names. But rather than require the user to count how long their suffix is, I > imagine it would make more sense to give mktemp(1) some do-what-I-mean smarts. Before I do that, how about documenting what mktemp currently does. The proposed patch documents the current behavior, although it probably needs some tweaks (and corresponding tweaks in mktemp.c) based this email. mktemp --help is not very obvious that --tmpdir is implied if no TEMPLATE is given. Right now, 'mktemp --help' claims that --tmpdir and -p are distinct, but the code says they are almost the same (they both set use_dest_dir=true and dest_dir_arg=optarg). The only difference is that --tmpdir has an optional argument, while -p has a mandatory argument, so dest_dir_arg can be NULL if you use --tmpdir but not if you use -p. By the way, the optional argument to --subdir has some interesting effects, because we are not consistent on whether we check optarg for being the empty string: mkdir --subdir="$dir" => if $dir is empty, try $TMPDIR; if that is empty, use /tmp mkdir --subdir="$dir" -t => if $TMPDIR is empty, use $dir; if that is empty, use . In other words, the only thing that I can see -t doing is whether we first check $TMPDIR or the argument to -p/--tmpdir, and restricts us from using subdirectories in the template (is this restriction really necessary)? It doesn't even print a deprecation warning or have a FIXME date with proposed removal. So, what if we just undeprecate -t (by adding a long option), and document/implement things as follows: -p DIR, --tmpdir[=DIR] interpret TEMPLATE relative to DIR. If DIR is empty or not supplied, use $TMPDIR if set, else /tmp -t, --favor-tmpdir with -p, favor $TMPDIR over the DIR argument; otherwise use $TMPDIR or /tmp instead of current directory TEMPLATE may contain slashes, but intermediate directories must exist. If -p or -t is specified, TEMPLATE must not be absolute. Also, is there a cleanup hole? For most code paths, an exit status of 1 means that the temporary file could not be created. But consider: mktemp >/dev/full This currently gives exit status 1, because the atexit() handler recognizes failure to print the file name to stdout, but leaves the temporary file around. Should we go ahead and manually flush/close stdout, rather than relying on close_stdout, so that we can then remove the just-created file if we detect write failure? That way, if we fail to inform the user what just got created, we are at least avoiding littering their file system with a random file. Fortunately, 'mktemp >&-' does not make the mistake of printing the just- created file as the contents of that file, since we close the fd returned by mkstemp before printing anything. At any rate, here's the proposed documentation patch: From: Eric Blake Date: Tue, 3 Nov 2009 10:47:42 -0700 Subject: [PATCH] doc: document mktemp * doc/coreutils.texi (mktemp invocation): New node. --- doc/coreutils.texi | 144 +++- 1 files changed, 142 insertions(+), 2 deletions(-) diff --git a/doc/coreutils.texi b/doc/coreutils.texi index ec5bcfb..1fd1bec 100644 --- a/doc/coreutils.texi +++ b/doc/coreutils.texi @@ -31,7 +31,6 @@ @c FIXME: the following need documentation @c * [: (coreutils)[ invocation. File/string tests. @c * pinky: (coreutils)pinky invocation. FIXME. -...@c * mktemp: (coreutils)mktemp invocation. FIXME. @dircategory Individual utilities @direntry @@ -80,6 +79,7 @@ * mkdir: (coreutils)mkdir invocation. Create directories. * mkfifo: (coreutils)mkfifo invocation. Create FIFOs (named pipes). * mknod: (coreutils)mknod invocation. Create special files. +* mktemp: (coreutils)mktemp invocation. Create temporary files. * mv: (coreutils)mv invocation. Rename files. * nice: (coreutils)nice invocation. Modify niceness. * nl: (coreutils)nl invocation. Number lines and write files. @@ -193,7 +193,7 @@ Top * Printing text::echo printf yes * Conditions:: false true test expr * Redirection:: tee -* File name manipulation:: dirname basename pathchk +* File name manipulation:: dirname basename pathchk mktemp * Working context:: pwd stty printenv tty * User information:: id logname whoami groups users who * System context:: date arch uname hostname hostid uptime @@ -378,6 +378,7 @@ Top * basename invocation:: Strip directory and suffix from a file name * dirname invocation:: Strip non-directory suffix from a file name *
Re: temp file suffixes: mktemp DWIM
Eric Blake wrote: > diff --git a/doc/coreutils.texi b/doc/coreutils.texi > index ec5bcfb..1fd1bec 100644 > --- a/doc/coreutils.texi > +++ b/doc/coreutils.texi > @@ -31,7 +31,6 @@ > @c FIXME: the following need documentation > @c * [: (coreutils)[ invocation. File/string tests. > @c * pinky: (coreutils)pinky invocation. FIXME. > -...@c * mktemp: (coreutils)mktemp invocation. FIXME. There is also a mention in TODO to remove. The doc looks good from a very quick review, the examples are nice. cheers, Pádraig.
Re: temp file suffixes: mktemp DWIM
Eric Blake wrote: > Eric Blake byu.net> writes: > >> Now that glibc 2.11 has mkostemps, and I'm working on adding that to gnulib, > it >> would be nice to expose the idea of an explicit suffix to temporary file >> names. But rather than require the user to count how long their suffix is, I >> imagine it would make more sense to give mktemp(1) some do-what-I-mean > smarts. Thanks a lot for writing the documentation (that's been a thorn in my side for far too long) and the careful once-over. I haven't had time yet to study much of this message, but bear in mind that I tried to implement a program that was faithful to this documentation: http://mktemp.org/manual.html I hope to get to this tomorrow. > Before I do that, how about documenting what mktemp currently does. The > proposed patch documents the current behavior, although it probably needs some > tweaks (and corresponding tweaks in mktemp.c) based this email. > > mktemp --help is not very obvious that --tmpdir is implied if no TEMPLATE is > given.
Re: temp file suffixes: mktemp DWIM
Jim Meyering wrote: > Eric Blake wrote: >> Eric Blake byu.net> writes: >> >>> Now that glibc 2.11 has mkostemps, and I'm working on adding that to gnulib, >> it >>> would be nice to expose the idea of an explicit suffix to temporary file >>> names. But rather than require the user to count how long their suffix is, >>> I >>> imagine it would make more sense to give mktemp(1) some do-what-I-mean >> smarts. > > Thanks a lot for writing the documentation (that's been a thorn in my > side for far too long) and the careful once-over. > I haven't had time yet to study much of this message, but bear > in mind that I tried to implement a program that was faithful > to this documentation: > > http://mktemp.org/manual.html Here's more perspective on -p vs -t: http://thread.gmane.org/gmane.comp.gnu.coreutils.bugs/11527
Re: temp file suffixes: mktemp DWIM
Jim Meyering meyering.net> writes: > > mktemp a => error, no run of X > > mktemp aXX => error, run of X is too short > > mktemp XXX => generates 3-character name (if possible) > > mktemp aXXX.b => generates 6-character name (if possible) > > mktemp --suffix=.b aXXX => longer spelling of the above line > > mktemp aXXX --suffix=.b => likewise, if not POSIXLY_CORRECT > > mktemp aXXXbXXX => generates aXXXb123 (only the last run changed) > > mktemp --suffix=X aXXX => only way to generate a123X instead of a1234 > > mktemp --suffix=.txt file => error, no run of X > > mktemp --suffix=.txt aXXXb => error, explicit --suffix requires trailing X > > Nice, comprehensive set of examples. Here's my first cut at implementing this. Eric Blake (4): [1/4] build: update gnulib submodule to latest, for tempname changes Real gnulib commit id TBD, once I push my pending mkostemps series to gnulib (http://thread.gmane.org/gmane.comp.lib.gnulib.bugs/19282). Note that either patch 1 or patch 2 in isolation will cause bootstrap failure; so even though you normally like to do gnulib submodule updates in isolation, I'm wondering if this is a case where the two patches should be squashed together. [2/4] build: reflect gnulib changes to tempname Since gnulib added mkostemps support to gen_tempname, we need to reflect the extra parameter in gen_tempname_len. I think the .diff format makes it easier to see how we intentionally differ from glibc, rather than writing a flat-out replacement file (it's also more robust at catching subsequent gnulib changes to the same file, to let us know to resynchronize). [3/4] tests: enhance mktemp test Add more tests of existing behavior, to ensure I didn't break it, and to make it obvious what I'm changing. I could rebase the series to put this first, alongside my pending mktemp doc patch, especially if your review of my doc patch and the accompanying email turn up any changes we want to existing behavior. [4/4] mktemp: add suffix handling The real fun. Hopefully I added enough tests to cover everything new that results from the new code. Also, is the rearranged output in usage() okay, and should I split that into a separate patch? In rereading this before hitting the send button, I see an out-of-bounds reference in patch 4. I'll know if you reviewed this if you spot the same bug ;) Also, I guess I should also amend my series to mention this bug report in the commit message: > > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=548316 >From 7a9f7441d27053c6842f1dc24087cd6bcca5e8b7 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 3 Nov 2009 08:47:24 -0700 Subject: [PATCH 1/4] build: update gnulib submodule to latest, for tempname changes --- gnulib |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/gnulib b/gnulib index 5ff8115..a820a49 16 --- a/gnulib +++ b/gnulib @@ -1 +1 @@ -Subproject commit 5ff811558adf7013f9fd9109fa794dd4e5ee8c91 +Subproject commit a820a49944558a7f956f1c6ed476fb1e90fff313 -- 1.6.4.2 >From 4a45ad587212bc80315362fcb7aa19a6db714333 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 3 Nov 2009 08:51:31 -0700 Subject: [PATCH 2/4] build: reflect gnulib changes to tempname * gl/lib/tempname.h: Change... * gl/lib/tempname.h.diff: ...to diff. Accommodate new suffixlen parameter. * gl/lib/tempname.c: Change... * gl/lib/tempname.c.diff: ...to diff. (check_x_suffix): Allow for X in suffix beyond x_suffix_len. (gen_tempname_len): Add suffixlen parameter. x_suffix_len is appropriately typed, but suffixlen inherits from the original botched type that glibc copied from BSD. (__gen_tempname): Update caller. * src/mktemp.c (mkstemp_len, mkdtemp_len): Update callers. --- gl/lib/tempname.c | 294 gl/lib/tempname.c.diff | 196 gl/lib/tempname.h | 42 --- gl/lib/tempname.h.diff | 20 src/mktemp.c |6 +- 5 files changed, 220 insertions(+), 338 deletions(-) delete mode 100644 gl/lib/tempname.c create mode 100644 gl/lib/tempname.c.diff delete mode 100644 gl/lib/tempname.h create mode 100644 gl/lib/tempname.h.diff diff --git a/gl/lib/tempname.c b/gl/lib/tempname.c deleted file mode 100644 index 84679bc..000 --- a/gl/lib/tempname.c +++ /dev/null @@ -1,294 +0,0 @@ -/* tempname.c - generate the name of a temporary file. - - Copyright (C) 1991, 1992, 1993, 1994, 1995, 1996, 1997, 1998, 1999, - 2000, 2001, 2002, 2003, 2005, 2006, 2007, 2009 Free Software Foundation, - Inc. - - This program is free software: you can redistribute it and/or modify - it under the terms of the GNU General Public License as published by - the Free Software Foundation; either version 3 of the License, or - (at your option) any later version. - - This program is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILIT
Re: temp file suffixes: mktemp DWIM
Eric Blake byu.net> writes:
> This currently gives exit status 1, because the atexit() handler recognizes
> failure to print the file name to stdout, but leaves the temporary file
> around. Should we go ahead and manually flush/close stdout, rather than
> relying on close_stdout, so that we can then remove the just-created file if
we
> detect write failure? That way, if we fail to inform the user what just got
> created, we are at least avoiding littering their file system with a random
> file.
>
> Fortunately, 'mktemp >&-' does not make the mistake of printing the just-
> created file as the contents of that file, since we close the fd returned by
> mkstemp before printing anything.
How about the following?
From: Eric Blake
Date: Wed, 4 Nov 2009 14:02:20 -0700
Subject: [PATCH] mktemp: don't leave file behind on write failure
* src/mktemp.c (main): Remove just-created file if stdout had
problems.
* bootstrap.conf (gnulib_modules): Add remove.
* tests/misc/close-stdout: Test it.
* NEWS: Document it.
---
NEWS|4
bootstrap.conf |1 +
src/mktemp.c| 12 +++-
tests/misc/close-stdout |6 ++
4 files changed, 22 insertions(+), 1 deletions(-)
diff --git a/NEWS b/NEWS
index 3f8baf0..4d1b4f5 100644
--- a/NEWS
+++ b/NEWS
@@ -17,6 +17,10 @@ GNU coreutils NEWS-*-
outline -*-
This also affected sum, sha1sum, sha224sum, sha384sum and sha512sum.
[the bug dates back to the initial implementation]
+ mktemp no longer leaves a temporary file behind if it was unable to
+ output the name of the file to stdout.
+ [the bug dates back to the initial implementation]
+
nice -n -1 PROGRAM now runs PROGRAM even when its internal setpriority
call fails with errno == EACCES.
[the bug dates back to the initial implementation]
diff --git a/bootstrap.conf b/bootstrap.conf
index 7960546..b3a82e0 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -180,6 +180,7 @@ gnulib_modules="
readutmp
realloc
regex
+ remove
rename
rmdir
root-dev-ino
diff --git a/src/mktemp.c b/src/mktemp.c
index e645362..12acad8 100644
--- a/src/mktemp.c
+++ b/src/mktemp.c
@@ -23,6 +23,7 @@
#include "system.h"
+#include "close-stream.h"
#include "error.h"
#include "filenamecat.h"
#include "quote.h"
@@ -322,7 +323,16 @@ main (int argc, char **argv)
}
if (status == EXIT_SUCCESS)
-puts (dest_name);
+{
+ puts (dest_name);
+ /* If we created a file, but then failed to output the file
+ name, we should clean up the mess before failing. */
+ if (!dry_run && close_stream (stdout))
+{
+ remove (dest_name);
+ error (EXIT_FAILURE, errno, _("write error"));
+}
+}
#ifdef lint
free (dest_name);
diff --git a/tests/misc/close-stdout b/tests/misc/close-stdout
index fec1add..852c3c8 100755
--- a/tests/misc/close-stdout
+++ b/tests/misc/close-stdout
@@ -50,12 +50,18 @@ if "$p/src/test" -w /dev/stdout >/dev/null &&
"$p/src/test" ! -w /dev/stdout >&-; then
"$p/src/printf" 'foo' >&- 2>/dev/null && fail=1
cp --verbose a b >&- 2>/dev/null && fail=1
+ rm -Rf tmpfile-?? || fail=1
+ mktemp tmpfile-XX >&- 2>/dev/null && fail=1
+ test -e tmpfile-?? && fail=1
fi
# Likewise for /dev/full, if /dev/full works.
if test -w /dev/full && test -c /dev/full; then
"$p/src/printf" 'foo' >/dev/full 2>/dev/null && fail=1
cp --verbose a b >/dev/full 2>/dev/null && fail=1
+ rm -Rf tmpdir-?? || fail=1
+ mktemp -d tmpdir-XX >/dev/full 2>/dev/null && fail=1
+ test -e tmpdir-?? && fail=1
fi
Exit $fail
--
1.6.4.2
Re: temp file suffixes: mktemp DWIM
Eric Blake byu.net> writes:
>if (status == EXIT_SUCCESS)
> -puts (dest_name);
> +{
> + puts (dest_name);
> + /* If we created a file, but then failed to output the file
> + name, we should clean up the mess before failing. */
> + if (!dry_run && close_stream (stdout))
> +{
> + remove (dest_name);
> + error (EXIT_FAILURE, errno, _("write error"));
The remove() can corrupt errno, so I'm squashing this on top:
diff --git i/src/mktemp.c w/src/mktemp.c
index 12acad8..049401f 100644
--- i/src/mktemp.c
+++ w/src/mktemp.c
@@ -329,8 +329,9 @@ main (int argc, char **argv)
name, we should clean up the mess before failing. */
if (!dry_run && close_stream (stdout))
{
+ int saved_errno = errno;
remove (dest_name);
- error (EXIT_FAILURE, errno, _("write error"));
+ error (EXIT_FAILURE, saved_errno, _("write error"));
}
}
--
Eric Blake
Re: temp file suffixes: mktemp DWIM
Eric Blake wrote: > Eric Blake byu.net> writes: > >> This currently gives exit status 1, because the atexit() handler recognizes >> failure to print the file name to stdout, but leaves the temporary file >> around. Should we go ahead and manually flush/close stdout, rather than >> relying on close_stdout, so that we can then remove the just-created file if > we >> detect write failure? That way, if we fail to inform the user what just got >> created, we are at least avoiding littering their file system with a random >> file. >> >> Fortunately, 'mktemp >&-' does not make the mistake of printing the just- >> created file as the contents of that file, since we close the fd returned by >> mkstemp before printing anything. > > How about the following? > > From: Eric Blake > Date: Wed, 4 Nov 2009 14:02:20 -0700 > Subject: [PATCH] mktemp: don't leave file behind on write failure > > * src/mktemp.c (main): Remove just-created file if stdout had > problems. > * bootstrap.conf (gnulib_modules): Add remove. > * tests/misc/close-stdout: Test it. > * NEWS: Document it. With your fix-up, that's a fine fix. Thanks! I should catch up on your doc- and primary change tomorrow.
Re: temp file suffixes: mktemp DWIM
Eric Blake wrote: > Jim Meyering meyering.net> writes: > >> > mktemp a => error, no run of X >> > mktemp aXX => error, run of X is too short >> > mktemp XXX => generates 3-character name (if possible) >> > mktemp aXXX.b => generates 6-character name (if possible) >> > mktemp --suffix=.b aXXX => longer spelling of the above line >> > mktemp aXXX --suffix=.b => likewise, if not POSIXLY_CORRECT >> > mktemp aXXXbXXX => generates aXXXb123 (only the last run changed) >> > mktemp --suffix=X aXXX => only way to generate a123X instead of a1234 >> > mktemp --suffix=.txt file => error, no run of X >> > mktemp --suffix=.txt aXXXb => error, explicit --suffix requires trailing X >> >> Nice, comprehensive set of examples. > > Here's my first cut at implementing this. > > Eric Blake (4): > [1/4] build: update gnulib submodule to latest, for tempname changes > Real gnulib commit id TBD, once I push my pending mkostemps series to gnulib > (http://thread.gmane.org/gmane.comp.lib.gnulib.bugs/19282). Note that either > patch 1 or patch 2 in isolation will cause bootstrap failure; so even though > you normally like to do gnulib submodule updates in isolation, I'm wondering > if > this is a case where the two patches should be squashed together. This sounds like a good reason indeed to include the gnulib update with the dependent change. FWIW, I'm less averse to including a gnulib submodule update with other changes, now that I've become more used to dealing with submodules. > [2/4] build: reflect gnulib changes to tempname > Since gnulib added mkostemps support to gen_tempname, we need to reflect the > extra parameter in gen_tempname_len. I think the .diff format makes it easier > to see how we intentionally differ from glibc, rather than writing a flat-out > replacement file (it's also more robust at catching subsequent gnulib changes > to the same file, to let us know to resynchronize). Yes, I was thinking the same thing. This would be easier to review if there were two deltas: - the no-semantic-change, convert-to-.diff one - the add-argument semantics-changing one but it's not that big a deal. Yeah, it's a shame about glibc using the incorrect type for the new parameter. > [3/4] tests: enhance mktemp test > Add more tests of existing behavior, to ensure I didn't break it, and to make > it obvious what I'm changing. I could rebase the series to put this first, > alongside my pending mktemp doc patch, especially if your review of my doc > patch and the accompanying email turn up any changes we want to existing > behavior. Those new tests look fine. > [4/4] mktemp: add suffix handling > The real fun. Hopefully I added enough tests to cover everything new that > results from the new code. Also, is the rearranged output in usage() okay, > and > should I split that into a separate patch? I'll probably run out of time here, but will start commenting. AUTHORS: Glad you added your name. > In rereading this before hitting the send button, I see an out-of-bounds > reference in patch 4. I'll know if you reviewed this if you spot the same > bug ;) Also, I guess I should also amend my series to mention this bug report > in the commit message: >> > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=548316 Better to stop now. Will resume here in the morning and then revisit your doc-change message.
Re: temp file suffixes: mktemp DWIM
Eric Blake wrote:
+ if (suffix)
+{
+ size_t len = strlen (template);
+ if (template[len - 1] != 'X')
If you pass '' the above is invalid
+{
+ error (EXIT_FAILURE, 0,
+ _("with --suffix, template %s must end in X"),
+ quote (template));
+}
+ suffix_len = strlen (suffix);
+ dest_name = xcharalloc (len + suffix_len + 1);
That's the first use of that function.
Seems of marginal benefit.
+ memcpy (dest_name, template, len);
+ memcpy (dest_name + len, suffix, suffix_len + 1);
+ template = dest_name;
+ suffix = dest_name + len;
+}
Other than that it's a really nice patch.
cheers,
Pádraig.
Re: temp file suffixes: mktemp DWIM
Eric Blake byu.net> writes:
> Before I do that, how about documenting what mktemp currently does. The
> proposed patch documents the current behavior, although it probably needs
some
> tweaks (and corresponding tweaks in mktemp.c) based this email.
I noticed this when rereading my patch. I'll be pushing this soon...
From: Eric Blake
Date: Mon, 9 Nov 2009 15:49:44 -0700
Subject: [PATCH] doc: fix typo
* doc/coreutils.texi (mktemp invocation): Quote shell variable.
---
doc/coreutils.texi |9 +
1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 3fc3a52..ad729b1 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -12127,7 +12127,7 @@ mktemp invocation
@example
$ dir=$(mktemp -p "$...@{tmpdir:-...@}" -d dir-) || exit 1
$ fifo=$dir/fifo
-$ mkfifo "$fifo" || @{ rmdir $dir; exit 1; @}
+$ mkfifo "$fifo" || @{ rmdir "$dir"; exit 1; @}
@end example
@item
@@ -12136,9 +12136,10 @@ mktemp invocation
or else in @file{/tmp}.
@example
$ file=$(mktemp -q) && @{
-> # Safe to use $file only within this block
-> echo ... > $file
-> rm $file
+> # Safe to use $file only within this block. Use quotes,
+> # since $TMPDIR, and thus $file, may contain whitespace.
+> echo ... > "$file"
+> rm "$file"
> @}
@end example
--
1.6.4.2
Re: temp file suffixes: mktemp DWIM
Eric Blake byu.net> writes:
>
> Eric Blake byu.net> writes:
>
> > Before I do that, how about documenting what mktemp currently does. The
> > proposed patch documents the current behavior, although it probably needs
> some
> > tweaks (and corresponding tweaks in mktemp.c) based this email.
>
> I noticed this when rereading my patch. I'll be pushing this soon...
after squashing this in, too:
diff --git i/doc/coreutils.texi w/doc/coreutils.texi
index d24a92c..3721bee 100644
--- i/doc/coreutils.texi
+++ w/doc/coreutils.texi
@@ -12187,12 +12187,12 @@ mktemp invocation
can create an object by the same name.
@item -p @var{dir}
-...@itemx --tempdi...@var{dir}]
+...@itemx --tmpdi...@var{dir}]
@opindex -p
-...@opindex --tempdir
+...@opindex --tmpdir
Treat @var{template} relative to the directory @var{dir}. If
@var{dir} is not specified (only possible with the long option
-...@option{--tempdir}) or is the empty string, use the value of
+...@option{--tmpdir}) or is the empty string, use the value of
@env{TMPDIR} if available, otherwise use @samp{/tmp}. If this is
specified, @var{template} must not be absolute. However,
@var{template} can still contain slashes, although intermediate
