[Tarantool-patches] [PATCH 0/2] fio.tempdir() improvements

[Tarantool-patches] [PATCH 0/2] fio.tempdir() improvements

[Vladislav Shpilevoy]

fio.tempdir() improvements to make it closer to POSIX.

Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4794-tmpdir
Issue: https://github.com/tarantool/tarantool/issues/4794

@ChangeLog
- fio.tempdir() uses TMPDIR as a path to a place where to create
  temporary directories. Also it accepts a template name argument,
  like mkdtemp().

Vladislav Shpilevoy (2):
  fio: respect $TMPDIR in fio.tempdir(), when it is set
  fio: allow to pass a template to fio.tempdir()

 src/lib/core/coio_file.c |  76 ++++++++++++++++++++----
 src/lib/core/coio_file.h |   2 +-
 src/lua/fio.c            |   3 +-
 test/app/fio.result      | 121 +++++++++++++++++++++++++++++++++++++++
 test/app/fio.test.lua    |  44 ++++++++++++++
 5 files changed, 234 insertions(+), 12 deletions(-)

-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH 1/2] fio: respect $TMPDIR in fio.tempdir(), when it is set

[Vladislav Shpilevoy]

TMPDIR is an environment variable used to tell what a directory
should be used to create temporary files. It is described in the
POSIX standard, and should be used by programs creating temporary
files.

Part of #4794
---
 src/lib/core/coio_file.c | 11 ++++---
 test/app/fio.result      | 68 ++++++++++++++++++++++++++++++++++++++++
 test/app/fio.test.lua    | 28 +++++++++++++++++
 3 files changed, 103 insertions(+), 4 deletions(-)

diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c
index 62388344e..e2345567c 100644
--- a/src/lib/core/coio_file.c
+++ b/src/lib/core/coio_file.c
@@ -466,13 +466,16 @@ coio_tempdir(char *path, size_t path_len)
 {
 	INIT_COEIO_FILE(eio);
 
-	if (path_len < sizeof("/tmp/XXXXXX") + 1) {
+	const char *tmpdir = getenv("TMPDIR");
+	if (tmpdir == NULL)
+		tmpdir = "/tmp";
+	int rc = snprintf(path, path_len, "%s/XXXXXX", tmpdir);
+	if (rc < 0)
+		return -1;
+	if ((size_t) rc >= path_len) {
 		errno = ENOMEM;
 		return -1;
 	}
-
-	snprintf(path, path_len, "/tmp/XXXXXX");
-
 	eio.tempdir.tpl = path;
 	eio_req *req =
 		eio_custom(coio_do_tempdir, 0, coio_complete, &eio);
diff --git a/test/app/fio.result b/test/app/fio.result
index f83c43f44..2c9851d9e 100644
--- a/test/app/fio.result
+++ b/test/app/fio.result
@@ -1456,3 +1456,71 @@ fio.mktree('/dev/null/dir')
 - false
 - 'Error creating directory /dev/null: File exists'
 ...
+--
+-- gh-4794: fio.tempdir() should use $TEMPDIR.
+--
+cwd = fio.cwd()
+---
+...
+old_tmpdir = os.getenv('TMPDIR')
+---
+...
+tmpdir = cwd..'/tmp'
+---
+...
+fio.mkdir(tmpdir)
+---
+- true
+...
+os.setenv('TMPDIR', tmpdir)
+---
+...
+dir = fio.tempdir()
+---
+...
+dir:find(tmpdir) ~= nil or {dir, tmpdir}
+---
+- true
+...
+fio.stat(dir) ~= nil or fio.stat(dir)
+---
+- true
+...
+tmpdir = cwd..'/tmp2'
+---
+...
+os.setenv('TMPDIR', tmpdir)
+---
+...
+fio.tempdir()
+---
+- null
+- 'fio: No such file or directory'
+...
+os.setenv('TMPDIR', nil)
+---
+...
+dir = fio.tempdir()
+---
+...
+dir:find('/tmp') ~= nil or dir
+---
+- true
+...
+tmpdir = cwd..'/'..string.rep('t', 5000)
+---
+...
+os.setenv('TMPDIR', tmpdir)
+---
+...
+fio.tempdir()
+---
+- null
+- 'fio: Cannot allocate memory'
+...
+tmpdir = nil
+---
+...
+os.setenv('TMPDIR', old_tmpdir)
+---
+...
diff --git a/test/app/fio.test.lua b/test/app/fio.test.lua
index 56c957d8a..fb6a412b3 100644
--- a/test/app/fio.test.lua
+++ b/test/app/fio.test.lua
@@ -474,3 +474,31 @@ test_run:cmd("clear filter")
 --
 fio.mktree('/dev/null')
 fio.mktree('/dev/null/dir')
+
+--
+-- gh-4794: fio.tempdir() should use $TEMPDIR.
+--
+cwd = fio.cwd()
+old_tmpdir = os.getenv('TMPDIR')
+
+tmpdir = cwd..'/tmp'
+fio.mkdir(tmpdir)
+os.setenv('TMPDIR', tmpdir)
+dir = fio.tempdir()
+dir:find(tmpdir) ~= nil or {dir, tmpdir}
+fio.stat(dir) ~= nil or fio.stat(dir)
+
+tmpdir = cwd..'/tmp2'
+os.setenv('TMPDIR', tmpdir)
+fio.tempdir()
+
+os.setenv('TMPDIR', nil)
+dir = fio.tempdir()
+dir:find('/tmp') ~= nil or dir
+
+tmpdir = cwd..'/'..string.rep('t', 5000)
+os.setenv('TMPDIR', tmpdir)
+fio.tempdir()
+tmpdir = nil
+
+os.setenv('TMPDIR', old_tmpdir)
-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH 2/2] fio: allow to pass a template to fio.tempdir()

[Vladislav Shpilevoy]

Rules are the same as for mkdtemp() POSIX function. Template
should have at least 6 'X' symbols in the end. Last 6 are
replaced with random printable characters: digits or letters.

Closes #4794

@TarantoolBot document
Title: fio.tempdir() $TMPDIR and custom template

fio.tempdir() stores created temporary directory into /tmp by
default. This can be changed by setting TMPDIR environment
variable. Before starting Tarantool, or at runtime by
os.setenv().

fio.tempdir() now accepts a template parameter. It should be a
dirname having 'XXXXXX' at the end. These 'X' are replaced with
random digits and letters. Dirname should not include any other
path parts. Path to the dir to create is set by TMPDIR env var.

Template should have at least 6 'X' in the end. If it has less,
it is an error. If it has more, only 6 last 'X' are replaced. So,
for example, this template 'dirXXXXXXXX' can turn into
'dirXXabcdef'. First 'XX' is not touched.

Default template is 'XXXXXX'.
---
 src/lib/core/coio_file.c | 67 +++++++++++++++++++++++++++++++++++-----
 src/lib/core/coio_file.h |  2 +-
 src/lua/fio.c            |  3 +-
 test/app/fio.result      | 55 ++++++++++++++++++++++++++++++++-
 test/app/fio.test.lua    | 18 ++++++++++-
 5 files changed, 134 insertions(+), 11 deletions(-)

diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c
index e2345567c..adb2fe4b0 100644
--- a/src/lib/core/coio_file.c
+++ b/src/lib/core/coio_file.c
@@ -34,6 +34,7 @@
 #include "say.h"
 #include "fio.h"
 #include "errinj.h"
+#include "random.h"
 #include <stdio.h>
 #include <stdlib.h>
 #include <dirent.h>
@@ -448,28 +449,80 @@ coio_readlink(const char *pathname, char *buf, size_t bufsize)
 	return coio_wait_done(req, &eio);
 }
 
+/**
+ * glibc implementation of mkdtemp(). mkdtemp() is not used
+ * directly, because its behaviour differs on systems. For
+ * example, on Mac mkdtemp() behaves in a non standard way - it
+ * allows a template to have more and less than 6 'X' symbols.
+ * When more than 6, all of them are replaced. On Linux it is only
+ * 6 'X'. Less is treated as an error. More than 6 does not affect
+ * behaviour - only last 6 'X' are replaced.
+ */
 static void
 coio_do_tempdir(eio_req *req)
 {
 	struct coio_file_task *eio = (struct coio_file_task *)req->data;
-	char *res = mkdtemp(eio->tempdir.tpl);
-	req->errorno = errno;
-	if (res == NULL) {
+	char *templ = eio->tempdir.tpl;
+	/*
+	 * A lower bound on the number of temporary files to
+	 * attempt to generate. The maximum total number of
+	 * temporary file names that can exist for a given
+	 * template is 62**6. It should never be necessary to try
+	 * all of these combinations. Instead if a reasonable
+	 * number of names is tried (62**3) fail to give the
+	 * system administrator the chance to remove the problems.
+	 */
+	int attempts = 62 * 62 * 62;
+	if (attempts < TMP_MAX)
+		attempts = TMP_MAX;
+	int len = strlen(templ);
+	if (len < 6 || memcmp(&templ[len - 6], "XXXXXX", 6) != 0) {
+		errno = EINVAL;
 		req->result = -1;
-	} else {
-		req->result = 0;
+		return;
 	}
+	char *xxxxxx = &templ[len - 6];
+	const char *letters = "abcdefghijklmnopqrstuvwxyz"
+			      "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
+	uint64_t value;
+	random_bytes((char *)&value, sizeof(value));
+	for (int count = 0; count < attempts; value += 7777, ++count) {
+		uint64_t v = value;
+		xxxxxx[0] = letters[v % 62];
+		v /= 62;
+		xxxxxx[1] = letters[v % 62];
+		v /= 62;
+		xxxxxx[2] = letters[v % 62];
+		v /= 62;
+		xxxxxx[3] = letters[v % 62];
+		v /= 62;
+		xxxxxx[4] = letters[v % 62];
+		v /= 62;
+		xxxxxx[5] = letters[v % 62];
+		if (mkdir(templ, S_IRUSR | S_IWUSR | S_IXUSR) == 0) {
+			req->result = 0;
+			return;
+		}
+		if (errno != EEXIST) {
+			req->result = -1;
+			return;
+		}
+	}
+	errno = EEXIST;
+	req->result = -1;
 }
 
 int
-coio_tempdir(char *path, size_t path_len)
+coio_tempdir(char *path, size_t path_len, const char *templ)
 {
 	INIT_COEIO_FILE(eio);
 
 	const char *tmpdir = getenv("TMPDIR");
 	if (tmpdir == NULL)
 		tmpdir = "/tmp";
-	int rc = snprintf(path, path_len, "%s/XXXXXX", tmpdir);
+	if (templ == NULL)
+		templ = "XXXXXX";
+	int rc = snprintf(path, path_len, "%s/%s", tmpdir, templ);
 	if (rc < 0)
 		return -1;
 	if ((size_t) rc >= path_len) {
diff --git a/src/lib/core/coio_file.h b/src/lib/core/coio_file.h
index ac7b1aacf..416cc499c 100644
--- a/src/lib/core/coio_file.h
+++ b/src/lib/core/coio_file.h
@@ -80,7 +80,7 @@ int     coio_sync();
 int     coio_fsync(int fd);
 int     coio_fdatasync(int fd);
 
-int	coio_tempdir(char *path, size_t path_len);
+int	coio_tempdir(char *path, size_t path_len, const char *templ);
 
 int	coio_readdir(const char *path, char **buf);
 int	coio_copyfile(const char *source, const char *dest);
diff --git a/src/lua/fio.c b/src/lua/fio.c
index 7437cc0c6..a3b63bbfb 100644
--- a/src/lua/fio.c
+++ b/src/lua/fio.c
@@ -603,13 +603,14 @@ usage:
 static int
 lbox_fio_tempdir(struct lua_State *L)
 {
+	const char *templ = luaL_optstring(L, 1, NULL);
 	char *buf = (char *)lua_newuserdata(L, PATH_MAX);
 	if (!buf) {
 		errno = ENOMEM;
 		return lbox_fio_pushsyserror(L);
 	}
 
-	if (coio_tempdir(buf, PATH_MAX) != 0)
+	if (coio_tempdir(buf, PATH_MAX, templ) != 0)
 		return lbox_fio_pushsyserror(L);
 	lua_pushstring(L, buf);
 	lua_remove(L, -2);
diff --git a/test/app/fio.result b/test/app/fio.result
index 2c9851d9e..2d37ca368 100644
--- a/test/app/fio.result
+++ b/test/app/fio.result
@@ -1457,7 +1457,8 @@ fio.mktree('/dev/null/dir')
 - 'Error creating directory /dev/null: File exists'
 ...
 --
--- gh-4794: fio.tempdir() should use $TEMPDIR.
+-- gh-4794: fio.tempdir() should use $TEMPDIR, and accept a
+-- template.
 --
 cwd = fio.cwd()
 ---
@@ -1524,3 +1525,55 @@ tmpdir = nil
 os.setenv('TMPDIR', old_tmpdir)
 ---
 ...
+dir, err = fio.tempdir('dirXXXXXX')
+---
+...
+err
+---
+- null
+...
+dir:find('XXXXXX') == nil or dir
+---
+- true
+...
+dir, err = fio.tempdir('dirXXXXXXXX')
+---
+...
+err
+---
+- null
+...
+dir:find('XXXXXX') == nil and dir:find('XX') ~= nil or dir
+---
+- true
+...
+fio.tempdir('')
+---
+- null
+- 'fio: Invalid argument'
+...
+fio.tempdir('XXX')
+---
+- null
+- 'fio: Invalid argument'
+...
+fio.tempdir('XXXXXXdir')
+---
+- null
+- 'fio: Invalid argument'
+...
+fio.tempdir(100)
+---
+- null
+- 'fio: Invalid argument'
+...
+fio.tempdir('path/dirXXXXXX')
+---
+- null
+- 'fio: No such file or directory'
+...
+fio.tempdir('/tmp/dirXXXXXX')
+---
+- null
+- 'fio: No such file or directory'
+...
diff --git a/test/app/fio.test.lua b/test/app/fio.test.lua
index fb6a412b3..df8f3a122 100644
--- a/test/app/fio.test.lua
+++ b/test/app/fio.test.lua
@@ -476,7 +476,8 @@ fio.mktree('/dev/null')
 fio.mktree('/dev/null/dir')
 
 --
--- gh-4794: fio.tempdir() should use $TEMPDIR.
+-- gh-4794: fio.tempdir() should use $TEMPDIR, and accept a
+-- template.
 --
 cwd = fio.cwd()
 old_tmpdir = os.getenv('TMPDIR')
@@ -502,3 +503,18 @@ fio.tempdir()
 tmpdir = nil
 
 os.setenv('TMPDIR', old_tmpdir)
+
+dir, err = fio.tempdir('dirXXXXXX')
+err
+dir:find('XXXXXX') == nil or dir
+
+dir, err = fio.tempdir('dirXXXXXXXX')
+err
+dir:find('XXXXXX') == nil and dir:find('XX') ~= nil or dir
+
+fio.tempdir('')
+fio.tempdir('XXX')
+fio.tempdir('XXXXXXdir')
+fio.tempdir(100)
+fio.tempdir('path/dirXXXXXX')
+fio.tempdir('/tmp/dirXXXXXX')
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH 0/2] fio.tempdir() improvements

[Igor Munkin]

Vlad,

Thanks, the patchset LGTM. I dumped below some points we discussed:

1. It worth to discuss whether to use all 'X' wildcards at the template
end, since as you mentioned the behaviour differs on several POSIX
compatible platforms. Yes, you've already documented the current
implementation, but this bullet is more about usability.

2. You also proposed an additional parameter for suffix length (a fixed
length substring that can be appended to 'X' wild cards). Let's discuss
it in public space, since it looks like a great enhancement to the
existing API.

And small nit regarging the second patch: it would be great to mention
that tempname algorithm originates to the glibc one[1].

On 01.03.20, Vladislav Shpilevoy wrote:
> fio.tempdir() improvements to make it closer to POSIX.
> 
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4794-tmpdir
> Issue: https://github.com/tarantool/tarantool/issues/4794
> 
> @ChangeLog
> - fio.tempdir() uses TMPDIR as a path to a place where to create
>   temporary directories. Also it accepts a template name argument,
>   like mkdtemp().
> 
> Vladislav Shpilevoy (2):
>   fio: respect $TMPDIR in fio.tempdir(), when it is set
>   fio: allow to pass a template to fio.tempdir()
> 
>  src/lib/core/coio_file.c |  76 ++++++++++++++++++++----
>  src/lib/core/coio_file.h |   2 +-
>  src/lua/fio.c            |   3 +-
>  test/app/fio.result      | 121 +++++++++++++++++++++++++++++++++++++++
>  test/app/fio.test.lua    |  44 ++++++++++++++
>  5 files changed, 234 insertions(+), 12 deletions(-)
> 
> -- 
> 2.21.1 (Apple Git-122.3)
> 

[1]: https://code.woboq.org/userspace/glibc/sysdeps/posix/tempname.c.html#__gen_tempname

-- 
Best regards,
IM