Subject: bug#15425: mktemp - conflicting options affected by ENV

From: Pádraig Brady <[email-masked]>
Date: Sun, 06 Oct 2013 18:10:06 +0100

On 09/26/2013 01:59 PM, Pádraig Brady wrote:
> On 09/26/2013 05:36 AM, Jim Meyering wrote:
>>> I'll push this soon, unless there are objections
>>> to the above "undeprecation" of -p.
>>
>> Go for it. That seems best.  Thank you.
> 
> That -p = --tmpdir aspect is OK.
> 
> But on consideration, erroring on -p,-t is too harsh.
> Really -t is just a mode to operate in,
> i.e. enforce no '/' in template, and give precedence
> to TMPDIR over --tmpdir
> Hopefully the attached documentation only patch
> suffices to clear things up.
> 
> BTW I noticed this variation which could be
> used to generate passwords or something:
> 
>  $ mktemp -u -t -p '' XXXXXXXX
>  L5awccB1
> 
> However without -t, '/tmp/' is inserted.
> I'm inclined to change to not output '/tmp/' here?
> 
>  $ mktemp -u -p '' XXXXXXXX
>  /tmp/L5awccB1

Actually the best way to generate random chars like the above
is to avoid -p entirely. So I've made -t consistent and
output /tmp/... in the above case.

Also I matched the -q option up with the documentation.
The documented functionality made sense, which was to
suppress only I/O errors, but the existing code suppressed
almost all errors, including most usage errors.
So I fixed that up in a second patch.

Both updated patches are attached,
and already pushed.

thanks,
Pádraig.
>From 8e67c2dec1a595619d540bc3e52c2dfcf61a63a3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Sat, 21 Sep 2013 03:33:12 +0100
Subject: [PATCH 1/2] mktemp: synchronize the -p option with docs

* src/mktemp.c (usage): Synchronize the -p option description with
the logic and info docs.  I.E. that -p is just an alias of --tmpdir.
Also for consistency treat --tmpdir='' the same with or without -t.
I.E. always ignore the --tmpdir option if the param is empty.
Fixes http://bugs.gnu.org/15425
---
 src/mktemp.c |   22 ++++++++--------------
 1 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/src/mktemp.c b/src/mktemp.c
index 44845c3..41d5763 100644
--- a/src/mktemp.c
+++ b/src/mktemp.c
@@ -43,7 +43,6 @@ static const char *default_template = "tmp.XXXXXXXXXX";
 enum
 {
   SUFFIX_OPTION = CHAR_MAX + 1,
-  TMPDIR_OPTION
 };
 
 static struct option const longopts[] =
@@ -52,7 +51,7 @@ static struct option const longopts[] =
   {"quiet", no_argument, NULL, 'q'},
   {"dry-run", no_argument, NULL, 'u'},
   {"suffix", required_argument, NULL, SUFFIX_OPTION},
-  {"tmpdir", optional_argument, NULL, TMPDIR_OPTION},
+  {"tmpdir", optional_argument, NULL, 'p'},
   {GETOPT_HELP_OPTION_DECL},
   {GETOPT_VERSION_OPTION_DECL},
   {NULL, 0, NULL, 0}
@@ -85,20 +84,17 @@ Files are created u+rw, and directories u+rwx, minus umask restrictions.\n\
                         This option is implied if TEMPLATE does not end in X\n\
 "), stdout);
       fputs (_("\
-      --tmpdir[=DIR]  interpret TEMPLATE relative to DIR; if DIR is not\n\
+  -p DIR, --tmpdir[=DIR]  interpret TEMPLATE relative to DIR; if DIR is not\n\
                         specified, use $TMPDIR if set, else /tmp.  With\n\
                         this option, TEMPLATE must not be an absolute name;\n\
                         unlike with -t, TEMPLATE may contain slashes, but\n\
                         mktemp creates only the final component\n\
 "), stdout);
-      fputs ("\n", stdout);
       fputs (_("\
-  -p DIR              use DIR as a prefix; implies -t [deprecated]\n\
   -t                  interpret TEMPLATE as a single file name component,\n\
                         relative to a directory: $TMPDIR, if set; else the\n\
                         directory specified via -p; else /tmp [deprecated]\n\
 "), stdout);
-      fputs ("\n", stdout);
       fputs (HELP_OPTION_DESCRIPTION, stdout);
       fputs (VERSION_OPTION_DESCRIPTION, stdout);
       emit_ancillary_info ();
@@ -195,11 +191,6 @@ main (int argc, char **argv)
           dry_run = true;
           break;
 
-        case TMPDIR_OPTION:
-          use_dest_dir = true;
-          dest_dir_arg = optarg;
-          break;
-
         case SUFFIX_OPTION:
           suffix = optarg;
           break;
@@ -283,9 +274,12 @@ main (int argc, char **argv)
       if (deprecated_t_option)
         {
           char *env = getenv ("TMPDIR");
-          dest_dir = (env && *env
-                      ? env
-                      : (dest_dir_arg ? dest_dir_arg : "/tmp"));
+          if (env && *env)
+            dest_dir = env;
+          else if (dest_dir_arg && *dest_dir_arg)
+            dest_dir = dest_dir_arg;
+          else
+            dest_dir = "/tmp";
 
           if (last_component (template) != template)
             error (EXIT_FAILURE, 0,
-- 
1.7.7.6

>From 0c1d7917f19b4865544ab9f1b13dfcdfd4e65275 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Sun, 6 Oct 2013 17:26:51 +0100
Subject: [PATCH 2/2] mktemp: with --quiet, only suppress I/O errors

The reason for having a --quiet option is to
suppress only some subset of possible errors.
The most useful separation here is with usage/internal errors,
and errors due to file creation etc. (i.e. I/O errors).

* src/mktemp.c (main): Match the --help and info docs and
only suppress the file/dir creation error messages.
* tests/misc/mktemp.pl: Adjust accordingly.
---
 src/mktemp.c         |   27 ++++++++++-----------------
 tests/misc/mktemp.pl |    8 ++------
 2 files changed, 12 insertions(+), 23 deletions(-)

diff --git a/src/mktemp.c b/src/mktemp.c
index 41d5763..05530a3 100644
--- a/src/mktemp.c
+++ b/src/mktemp.c
@@ -26,7 +26,6 @@
 #include "error.h"
 #include "filenamecat.h"
 #include "quote.h"
-#include "stdio--.h"
 #include "tempname.h"
 
 /* The official name of this program (e.g., no 'g' prefix).  */
@@ -147,7 +146,7 @@ main (int argc, char **argv)
 {
   char const *dest_dir;
   char const *dest_dir_arg = NULL;
-  bool suppress_stderr = false;
+  bool suppress_file_err = false;
   int c;
   unsigned int n_args;
   char *template;
@@ -181,7 +180,7 @@ main (int argc, char **argv)
           use_dest_dir = true;
           break;
         case 'q':
-          suppress_stderr = true;
+          suppress_file_err = true;
           break;
         case 't':
           use_dest_dir = true;
@@ -205,15 +204,6 @@ main (int argc, char **argv)
         }
     }
 
-  if (suppress_stderr)
-    {
-      /* From here on, redirect stderr to /dev/null.
-         A diagnostic from getopt_long, above, would still go to stderr.  */
-      if (!freopen ("/dev/null", "wb", stderr))
-        error (EXIT_FAILURE, errno,
-               _("failed to redirect stderr to /dev/null"));
-    }
-
   n_args = argc - optind;
   if (2 <= n_args)
     {
@@ -317,8 +307,9 @@ main (int argc, char **argv)
       int err = mkdtemp_len (dest_name, suffix_len, x_count, dry_run);
       if (err != 0)
         {
-          error (0, errno, _("failed to create directory via template %s"),
-                 quote (template));
+          if (!suppress_file_err)
+            error (0, errno, _("failed to create directory via template %s"),
+                   quote (template));
           status = EXIT_FAILURE;
         }
     }
@@ -327,8 +318,9 @@ main (int argc, char **argv)
       int fd = mkstemp_len (dest_name, suffix_len, x_count, dry_run);
       if (fd < 0 || (!dry_run && close (fd) != 0))
         {
-          error (0, errno, _("failed to create file via template %s"),
-                 quote (template));
+          if (!suppress_file_err)
+            error (0, errno, _("failed to create file via template %s"),
+                   quote (template));
           status = EXIT_FAILURE;
         }
     }
@@ -342,7 +334,8 @@ main (int argc, char **argv)
         {
           int saved_errno = errno;
           remove (dest_name);
-          error (EXIT_FAILURE, saved_errno, _("write error"));
+          if (!suppress_file_err)
+            error (EXIT_FAILURE, saved_errno, _("write error"));
         }
     }
 
diff --git a/tests/misc/mktemp.pl b/tests/misc/mktemp.pl
index b15b669..d47371c 100755
--- a/tests/misc/mktemp.pl
+++ b/tests/misc/mktemp.pl
@@ -55,14 +55,12 @@ my @Tests =
     (
      # test-name, [option, option, ...] {OUT=>"expected-output"}
      #
-     ['too-many', 'a b',
+     ['too-many', '-q a b',
       {ERR=>"$prog: too many templates\n"
        . "Try '$prog --help' for more information.\n"}, {EXIT => 1} ],
-     ['too-many-q', '-q a b', {EXIT => 1} ],
 
-     ['too-few-x', 'foo.XX', {EXIT => 1},
+     ['too-few-x', '-q foo.XX', {EXIT => 1},
       {ERR=>"$prog: too few X's in template 'foo.XX'\n"}],
-     ['too-few-xq', '-q foo.XX', {EXIT => 1} ],
 
      ['1f', 'bar.XXXX', {OUT => "bar.ZZZZ\n"},
       {OUT_SUBST => 's,\.....$,.ZZZZ,'},
@@ -148,11 +146,9 @@ my @Tests =
 
      ['suffix6f', 'aXXXX/b', {EXIT=>1},
       {ERR=>"$prog: invalid suffix '/b', contains directory separator\n"}],
-     ['suffix6f-q', '-q aXXXX/b', {EXIT=>1}],
 
      ['suffix7f', '--suffix= aXXXXb', {EXIT=>1},
       {ERR=>"$prog: with --suffix, template 'aXXXXb' must end in X\n"}],
-     ['suffix7f-q', '-q --suffix= aXXXXb', {EXIT=>1}],
      ['suffix7d', '-d --suffix=aXXXXb ""', {EXIT=>1},
       {ERR=>"$prog: with --suffix, template '' must end in X\n"}],
 
-- 
1.7.7.6