[PATCH] tmpdir: Add function to create a unique temp directory.

[PATCH] tmpdir: Add function to create a unique temp directory.

[John Darrington]

* lib/tmpdir.c (create_tmp_dir): New function definition.
* lib/tmpdir.h (create_tmp_dir): New function declaration.
---
 lib/tmpdir.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/tmpdir.h |  20 +++++++++
 2 files changed, 139 insertions(+)

diff --git a/lib/tmpdir.c b/lib/tmpdir.c
index 28ff99f58..b6cd3e526 100644
--- a/lib/tmpdir.c
+++ b/lib/tmpdir.c
@@ -163,3 +163,122 @@ path_search (char *tmpl, size_t tmpl_len, const char *dir, const char *pfx,
   sprintf (tmpl + dlen, &"/%.*sXXXXXX"[!add_slash], (int) plen, pfx);
   return 0;
 }
+
+
+/* Create a unique temporary directory and return its name.
+
+   If BASEDIR is non-null the directory will be a sub-directory of BASEDIR.
+   Otherwise, if TRY_TMPDIR is non-null the directory will be created in the
+   first of $TMPDIR,  P_tmpdir, /tmp that exists.  If none of those directories
+   exist,  the function will fail, and ENOENT will be set in errno.
+
+   TRY_TMPDIR is ignored if BASEDIR is non-null.
+
+   If PFX is non-null,  the first 5 bytes of it will be used as a prefix for
+   the directory name.    On failure, NULL will be returned and errno set
+   accordingly.
+
+   On success, a temporary directory will be created (using mkdtemp from
+   stdlib.h), and its  name returned.
+
+   The caller is responsible for freeing the return value (and removing the
+   directory if appropriate). */
+char *
+create_tmp_dir (const char *basedir, const char *pfx, bool try_tmpdir)
+{
+  char *dir = NULL;
+  bool add_slash;
+  size_t dlen = 0;
+  size_t plen = 0;
+
+  if (!pfx || !pfx[0])
+    {
+      pfx = "file";
+      plen = 4;
+    }
+  else
+    {
+      plen = strlen (pfx);
+      if (plen > 5)
+        plen = 5;
+    }
+
+  if (basedir != NULL)
+    {
+      dir = strdup (basedir);
+      if (dir == NULL)
+        {
+          return NULL;
+        }
+    }
+
+  if (dir == NULL)
+    {
+      if (try_tmpdir)
+        {
+          char *d = __libc_secure_getenv ("TMPDIR");
+          if (d != NULL && direxists (d))
+            dir = strdup (d);
+          else if (dir != NULL && direxists (dir))
+            /* nothing */ ;
+          else
+            dir = NULL;
+        }
+    }
+
+  if (dir == NULL)
+    {
+#if defined _WIN32 && ! defined __CYGWIN__
+      char dirbuf[PATH_MAX];
+      DWORD retval;
+
+      /* Find Windows temporary file directory.
+         We try this before P_tmpdir because Windows defines P_tmpdir to "\\"
+         and will therefore try to put all temporary files in the root
+         directory (unless $TMPDIR is set).  */
+      retval = GetTempPath (PATH_MAX, dirbuf);
+      if (retval > 0 && retval < PATH_MAX && direxists (dirbuf))
+        dir = strdup (dirbuf);
+      else
+#endif
+      if (direxists (P_tmpdir))
+        dir = strdup (P_tmpdir);
+      else if (strcmp (P_tmpdir, "/tmp") != 0 && direxists ("/tmp"))
+        dir = strdup ("/tmp");
+      else
+        {
+          __set_errno (ENOENT);
+          return NULL;
+        }
+    }
+
+  dlen = strlen (dir);
+#ifdef __VMS
+  add_slash = 0;
+#else
+  add_slash = dlen != 0 && !ISSLASH (dir[dlen - 1]);
+#endif
+
+  {
+    char *d = dir;
+    /* Make room for "${dir}/${pfx}XXXXXX\0" */
+    dir = realloc (d, dlen + add_slash + plen + 6 + 1);
+    if (dir == NULL)
+      {
+        int e = errno;
+        free (d);
+        __set_errno (e);
+        return NULL;
+      }
+  }
+
+  if (0 > sprintf (dir + dlen, &"/%.*sXXXXXX"[!add_slash], (int) plen, pfx))
+    {
+        int e = errno;
+        free (dir);
+        __set_errno (e);
+        return NULL;
+    }
+
+  return mkdtemp (dir);
+}
diff --git a/lib/tmpdir.h b/lib/tmpdir.h
index 4d694a3d9..ea71ef635 100644
--- a/lib/tmpdir.h
+++ b/lib/tmpdir.h
@@ -24,3 +24,23 @@
    doesn't exist, none of the searched dirs exists, or there's not
    enough space in TMPL. */
 extern int path_search (char *tmpl, size_t tmpl_len, const char *dir, const char *pfx, bool try_tmpdir);
+
+/* Create a unique temporary directory and return its name.
+
+   If BASEDIR is non-null the directory will be a sub-directory of BASEDIR.
+   Otherwise, if TRY_TMPDIR is non-null the directory will be created in the
+   first of $TMPDIR,  P_tmpdir, /tmp that exists.  If none of those directories
+   exist,  the function will fail, and ENOENT will be set in errno.
+
+   TRY_TMPDIR is ignored if BASEDIR is non-null.
+
+   If PFX is non-null,  the first 5 bytes of it will be used as a prefix for
+   the directory name.    On failure, NULL will be returned and errno set
+   accordingly.
+
+   On success, a temporary directory will be created (using mkdtemp from
+   stdlib.h), and its  name returned.
+
+   The caller is responsible for freeing the return value (and removing the
+   directory if appropriate). */
+char *create_tmp_dir (const char *basedir, const char *pfx, bool try_tmpdir);
-- 
2.20.1

Re: [PATCH] tmpdir: Add function to create a unique temp directory.

[Bruno Haible]

Hi John,

First of all, thanks for having signed a copyright assignment for Gnulib!

Then, let me see what Gnulib modules we already have, to see how your proposed
patch fits in.

1) We have modules that provide auxiliary functions.
    step 1 : tmpdir - find an appropriate parent dir
    step 2 : tempname - use random bits to fill in the XXXXXX

2) Modules that create a file:

  Create a file, return an fd, includes step 2, but step 1 left to the caller:
    mkstemp (char *template)
    mkstemps (char *template, int suffixlen)
    mkostemp (char *template, int flags)
    mkostemps (char *template, int suffixlen, int suffixlen)

  Create a file, return a FILE *, includes step 1 and step 2:
    tmpfile (void)
    tmpfile_safer (void)

3) Modules that create a directory:

  Create a directory, includes step 2, but step 1 left to the caller:
    mkdtemp (char *template)

  Create a directory, includes step 1 and step 2, with automatic cleanup:
    clean-temp : create_temp_dir (prefix, parentdir, cleanup_verbose)

Your proposed function falls into the category 3:
  Create a directory, includes step 1 and step 2.

So, it differs from 'create_temp_dir' only in the aspect that it does NOT
do automatic cleanup.

Since you are already aware of the 'clean-temp' module — you contributed to
it 8 years ago :) — what could we write in the documentation, what is the
advantage of your proposed function?

Further remarks:

* You have better comments regarding PFX and TRY_TMPDIR that those found
  for 'path_search' in lib/tmpdir.h and lib/tmpdir.c. It would make sense
  to improve these comments first.

* Given that 'tmpdir', so far, is an auxiliary module (used by other modules),
  I don't like to add higher-level function to it. I would prefer if the
  higher-level function were in a separate module.
  We can rename the existing module 'tmpdir' to, say, 'find-tmpdir', if that
  helps.

* The comment "TRY_TMPDIR is ignored if BASEDIR is non-null." suggests that
  it would be better to have two different functions, instead of trying to
  force it into one.

* Much of the code looks like a copy of the 'path_search' function. Why not
  use 'path_search'? What is missing?

Bruno

Re: [PATCH] tmpdir: Add function to create a unique temp directory.

[John Darrington]

On Thu, Dec 17, 2020 at 12:26:20AM +0100, Bruno Haible wrote:
     
     So, it differs from 'create_temp_dir' only in the aspect that it does NOT
     do automatic cleanup.
     
     Since you are already aware of the 'clean-temp' module — you contributed to
     it 8 years ago :) — what could we write in the documentation, what is the
     advantage of your proposed function?

I'd forgotten about that.   I guess there is little advantage except in the
implementation.  See below

     * Given that 'tmpdir', so far, is an auxiliary module (used by other modules),
       I don't like to add higher-level function to it. I would prefer if the
       higher-level function were in a separate module.
       We can rename the existing module 'tmpdir' to, say, 'find-tmpdir', if that
       helps.


I think that would make sense.  But so far as I can tell, path_search is used
only by the module clean-temp so why bother with it at all?  Why not move the
function into the clean-temp module and obsolete the tmpdir module?

     
     * The comment "TRY_TMPDIR is ignored if BASEDIR is non-null." suggests that
       it would be better to have two different functions, instead of trying to
       force it into one.
     
     * Much of the code looks like a copy of the 'path_search' function. Why not
       use 'path_search'? What is missing?
     
This was the original motivation for the change.  The path_search function in
its existing form is a pain to use:

One must pass it a buffer containing enough bytes to contain the result.  But
until you've called it, one doesn't know how many bytes are "enough".  One
solution is to dynamically allocate a buffer of 2  bytes, and call path_search
in a loop, increasing the buffer size each iteration, until it succeeds.  But
that of course is hardly efficient.

Another solution is to declare a buffer of length PATH_MAX but PATH_MAX does not
exist on the Hurd.   I see that what the clean-temp module does is to define
PATH_MAX as 1024 if it is not already defined.   This can be problematic for
two reasons:

1.  How do you know that 1024 is enough.

2.  What happens if the operating system *does* define PATH_MAX, but defines it
    to something very large: eg:  0xFFFFFFFF  ?
    In this case, clean-temp will fail when it gets to the line

    xtemplate = (char *) xmalloca (PATH_MAX);


So I think what really needs to happen is, that path_search needs to be
rewritten (and renamed) so that it dynamically allocates its output, and
clean-temp needs to be updated to use the rewritten version.

J'