Skip to content
This repository has been archived by the owner on May 4, 2018. It is now read-only.

Implement uv_fs_mkdtemp #1368

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions include/uv.h
Original file line number Diff line number Diff line change
Expand Up @@ -1849,6 +1849,7 @@ typedef enum {
UV_FS_UNLINK,
UV_FS_RMDIR,
UV_FS_MKDIR,
UV_FS_MKDTEMP,
UV_FS_RENAME,
UV_FS_READDIR,
UV_FS_LINK,
Expand Down Expand Up @@ -1891,6 +1892,9 @@ UV_EXTERN int uv_fs_write(uv_loop_t* loop, uv_fs_t* req, uv_file file,
UV_EXTERN int uv_fs_mkdir(uv_loop_t* loop, uv_fs_t* req, const char* path,
int mode, uv_fs_cb cb);

UV_EXTERN int uv_fs_mkdtemp(uv_loop_t* loop, uv_fs_t* req, const char* template,
uv_fs_cb cb);

UV_EXTERN int uv_fs_rmdir(uv_loop_t* loop, uv_fs_t* req, const char* path,
uv_fs_cb cb);

Expand Down
18 changes: 18 additions & 0 deletions src/unix/fs.c
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,11 @@ static ssize_t uv__fs_futime(uv_fs_t* req) {
}


static ssize_t uv__fs_mkdtemp(uv_fs_t* req) {
return mkdtemp((char*) req->path) ? 0 : -1;
}


static ssize_t uv__fs_read(uv_fs_t* req) {
ssize_t result;

Expand Down Expand Up @@ -789,6 +794,7 @@ static void uv__fs_work(struct uv__work* w) {
X(LSTAT, uv__fs_lstat(req->path, &req->statbuf));
X(LINK, link(req->path, req->new_path));
X(MKDIR, mkdir(req->path, req->mode));
X(MKDTEMP, uv__fs_mkdtemp(req));
X(READ, uv__fs_read(req));
X(READDIR, uv__fs_readdir(req));
X(READLINK, uv__fs_readlink(req));
Expand Down Expand Up @@ -1001,6 +1007,18 @@ int uv_fs_mkdir(uv_loop_t* loop,
}


int uv_fs_mkdtemp(uv_loop_t* loop,
uv_fs_t* req,
const char* template,
uv_fs_cb cb) {
INIT(MKDTEMP);
req->path = strdup(template);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you freeing this anywhere?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's freed on request cleanup internally.
On Jul 19, 2014 9:48 AM, "Andrius Bentkus" notifications@github.com wrote:

In src/unix/fs.c:

@@ -991,6 +997,18 @@ int uv_fs_mkdir(uv_loop_t* loop,
}

+int uv_fs_mkdtemp(uv_loop_t* loop,

  •              uv_fs_t\* req,
    
  •              const char\* template,
    
  •              uv_fs_cb cb) {
    
  • INIT(MKDTEMP);
  • req->path = strdup(template);

Are you freeing this anywhere?


Reply to this email directly or view it on GitHub
https://github.com/joyent/libuv/pull/1368/files#r15142598.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/Hinidu/libuv/blob/mkdtemp/src/unix/fs.c#L1173

everywhere we use free(var), but here we are using free((void *) var);

The user has to actually clean up after the operation is finished by calling uv_fs_req_cleanup.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course the user has to call that function, that's how it works. Not sure
what your concern is.
On Jul 19, 2014 9:58 AM, "Andrius Bentkus" notifications@github.com wrote:

In src/unix/fs.c:

@@ -991,6 +997,18 @@ int uv_fs_mkdir(uv_loop_t* loop,
}

+int uv_fs_mkdtemp(uv_loop_t* loop,

  •              uv_fs_t\* req,
    
  •              const char\* template,
    
  •              uv_fs_cb cb) {
    
  • INIT(MKDTEMP);
  • req->path = strdup(template);

https://github.com/Hinidu/libuv/blob/mkdtemp/src/unix/fs.c#L1173

everywhere we use free(var), but here we are using free((void *) var);

The user has to actually clean up after the operation is finished by
calling uv_fs_req_cleanup.


Reply to this email directly or view it on GitHub
https://github.com/joyent/libuv/pull/1368/files#r15142632.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@txdv sorry, I don't understand what you want to say in your last comment, but I want to point you to https://github.com/Hinidu/libuv/blob/mkdtemp/src/unix/fs.c#L91
This PATH as you can see is used in many functions and it does the same thing as my code here. Actually I written it explicit by @saghul's suggestion because PATH works with path argument but in case of uv_fs_mkdtemp argument is template.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hinidu nevermind, I was wrong

@saghul you said it would be done internally, but it is actually handled by the user. it is a small technical difference

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, what I meant is that you don't explicitly need to free it. It
happens as part of the cleanup process. It's an internal detail the user
doesn't really need to know about.
On Jul 19, 2014 10:27 AM, "Andrius Bentkus" notifications@github.com
wrote:

In src/unix/fs.c:

@@ -991,6 +997,18 @@ int uv_fs_mkdir(uv_loop_t* loop,
}

+int uv_fs_mkdtemp(uv_loop_t* loop,

  •              uv_fs_t\* req,
    
  •              const char\* template,
    
  •              uv_fs_cb cb) {
    
  • INIT(MKDTEMP);
  • req->path = strdup(template);

@Hinidu https://github.com/Hinidu nevermind

@saghul https://github.com/saghul you said it would be done internally,
but it is actually handled by the user. it is a small technical difference


Reply to this email directly or view it on GitHub
https://github.com/joyent/libuv/pull/1368/files#r15142718.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I thought I saw a bug so I checked it out. Sometimes I am wrong.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries! It's great to have an extra pair of eyes on the watch, thanks!
:-)
On Jul 19, 2014 10:37 AM, "Andrius Bentkus" notifications@github.com
wrote:

In src/unix/fs.c:

@@ -991,6 +997,18 @@ int uv_fs_mkdir(uv_loop_t* loop,
}

+int uv_fs_mkdtemp(uv_loop_t* loop,

  •              uv_fs_t\* req,
    
  •              const char\* template,
    
  •              uv_fs_cb cb) {
    
  • INIT(MKDTEMP);
  • req->path = strdup(template);

Yes, I thought I saw a bug so I checked it out. Sometimes I am wrong.


Reply to this email directly or view it on GitHub
https://github.com/joyent/libuv/pull/1368/files#r15142758.

if (req->path == NULL)
return -ENOMEM;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about ENOTDIR ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, it is returned for other functions of this family.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just checking the result after strdup.

POST;
}


int uv_fs_open(uv_loop_t* loop,
uv_fs_t* req,
const char* path,
Expand Down
94 changes: 94 additions & 0 deletions src/win/fs.c
Original file line number Diff line number Diff line change
Expand Up @@ -721,6 +721,78 @@ void fs__mkdir(uv_fs_t* req) {
}


/* Some parts of the implementation were borrowed from glibc. */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is incorporating LGPL source into an MIT licensed project legal?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's clearly recognizable as glibc's __gen_tempname() from sysdeps/posix/tempname.c, parts have been copied verbatim. This seems like murky legal water.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch. My bad since I did the reviewing :-( I'll look for an alternative with appropriate licensing. If anyone gets there beforehand, please open an issue / PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize. It was my mistake. I hadn't much experience with copyright law.
I found slightly different OpenBSD version here http://openbsd.cs.toronto.edu/cgi-bin/cvsweb/src/lib/libc/stdio/mktemp.c It seems like their license permits copying and modification.
I can try change current implementation according to this version.
Sorry again, I hope I didn't create a lot of problems.

void fs__mkdtemp(uv_fs_t* req) {
static const WCHAR letters[] =
L"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to use both upper and lower case? AFAIK most Windows file systems don't distinguish them in file names.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, NTFS can, so yes.
On Jul 23, 2014 9:43 AM, "Pavel Platto" notifications@github.com wrote:

In src/win/fs.c:

@@ -721,6 +721,80 @@ void fs__mkdir(uv_fs_t* req) {
}

+void fs__mkdtemp(uv_fs_t* req) {

  • static const WCHAR letters[] =
  • L"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";

Does it make sense to use both upper and lower case? AFAIK most Windows
file systems don't distinguish them in file names.


Reply to this email directly or view it on GitHub
https://github.com/joyent/libuv/pull/1368/files#r15274807.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hahahaha!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was also borrowed from glibc :-)

size_t len;
WCHAR* template_part;
static uint64_t value;
unsigned int count;
int fd;

/* 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 these combinations. Instead if a reasonable
number of names is tried (we define reasonable as 62**3) fail to
give the system administrator the chance to remove the problems. */
#define ATTEMPTS_MIN (62 * 62 * 62)

/* The number of times to attempt to generate a temporary file. To
conform to POSIX, this must be no smaller than TMP_MAX. */
#if ATTEMPTS_MIN < TMP_MAX
unsigned int attempts = TMP_MAX;
#else
unsigned int attempts = ATTEMPTS_MIN;
#endif

len = wcslen(req->pathw);
if (len < 6 || wcsncmp(&req->pathw[len - 6], L"XXXXXX", 6)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you just use 6 there even if it's a wide-char string?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please check if wcsncmp != 0, it's more explicit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can have a static buffer with 6 "X"s, and its length, instead of having 6 everywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you just use 6 there even if it's a wide-char string?

Yes, it is count of characters to match, not bytes (docs says that and I checked it now).

maybe we can have a static buffer with 6 "X"s, and its length, instead of having 6 everywhere?

Do you have idea how to name these variables? I have only two variants:

  • template_part_pattern and template_part_pattern_len which is perhaps too long.
  • xs and xs_len which also is not ideal but I like it more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, leave them as is, I checked glibc and it also does it like that. Can
you squash all commits into one?
On Jul 24, 2014 8:05 AM, "Pavel Platto" notifications@github.com wrote:

In src/win/fs.c:

  • can exist for a given template is 62**6. It should never be
    
  • necessary to try all these combinations. Instead if a reasonable
    
  • number of names is tried (we define reasonable as 62**3) fail to
    
  • give the system administrator the chance to remove the problems. */
    
    +#define ATTEMPTS_MIN (62 * 62 * 62)
    +
  • /* The number of times to attempt to generate a temporary file. To
  • conform to POSIX, this must be no smaller than TMP_MAX. */
    
    +#if ATTEMPTS_MIN < TMP_MAX
  • unsigned int attempts = TMP_MAX;
    +#else
  • unsigned int attempts = ATTEMPTS_MIN;
    +#endif
  • len = wcslen(req->pathw);
  • if (len < 6 || wcsncmp(&req->pathw[len - 6], L"XXXXXX", 6)) {

can you just use 6 there even if it's a wide-char string?

Yes, it is count of characters to match, not bytes (docs says that and I
checked it now).

maybe we can have a static buffer with 6 "X"s, and its length, instead of
having 6 everywhere?

Do you have idea how to name these variables? I have only two variants:

  • template_part_pattern and template_part_pattern_len which is perhaps
    too long.
  • xs and xs_len which also is not ideal but I like it more.


Reply to this email directly or view it on GitHub
https://github.com/joyent/libuv/pull/1368/files#r15331159.

SET_REQ_UV_ERROR(req, UV_EINVAL, ERROR_INVALID_PARAMETER);
return;
}

/* This is where the Xs start. */
template_part = &req->pathw[len - 6];

/* Get some random data. */
value += uv_hrtime() ^ _getpid();

for (count = 0; count < attempts; value += 7777, ++count) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you borrowed this implementation from glibc, please add a coment so that we know where to look for the magic numbers

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not true randomness! :) You could do better!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is 95$ borrowed from glibc, FWIW. Given the number of attempts we make, I'd say it's Good Enough (TM) :-)

uint64_t v = value;

/* Fill in the random bits. */
template_part[0] = letters[v % 62];
v /= 62;
template_part[1] = letters[v % 62];
v /= 62;
template_part[2] = letters[v % 62];
v /= 62;
template_part[3] = letters[v % 62];
v /= 62;
template_part[4] = letters[v % 62];
v /= 62;
template_part[5] = letters[v % 62];

fd = _wmkdir(req->pathw);

if (fd >= 0) {
len = strlen(req->path);
wcstombs((char*) req->path + len - 6, template_part, 6);
SET_REQ_RESULT(req, 0);
return;
} else if (errno != EEXIST) {
SET_REQ_RESULT(req, -1);
return;
}
}

/* We got out of the loop because we ran out of combinations to try. */
SET_REQ_RESULT(req, -1);
}


void fs__readdir(uv_fs_t* req) {
WCHAR* pathw = req->pathw;
size_t len = wcslen(pathw);
Expand Down Expand Up @@ -1528,6 +1600,7 @@ static void uv__fs_work(struct uv__work* w) {
XX(UNLINK, unlink)
XX(RMDIR, rmdir)
XX(MKDIR, mkdir)
XX(MKDTEMP, mkdtemp)
XX(RENAME, rename)
XX(READDIR, readdir)
XX(LINK, link)
Expand Down Expand Up @@ -1724,6 +1797,27 @@ int uv_fs_mkdir(uv_loop_t* loop, uv_fs_t* req, const char* path, int mode,
}


int uv_fs_mkdtemp(uv_loop_t* loop, uv_fs_t* req, const char* template,
uv_fs_cb cb) {
int err;

uv_fs_req_init(loop, req, UV_FS_MKDTEMP, cb);

err = fs__capture_path(loop, req, template, NULL, TRUE);
if (err) {
return uv_translate_sys_error(err);
}

if (cb) {
QUEUE_FS_TP_JOB(loop, req);
return 0;
} else {
fs__mkdtemp(req);
return req->result;
}
}


int uv_fs_rmdir(uv_loop_t* loop, uv_fs_t* req, const char* path, uv_fs_cb cb) {
int err;

Expand Down
61 changes: 61 additions & 0 deletions test/test-fs.c
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ static int read_cb_count;
static int write_cb_count;
static int unlink_cb_count;
static int mkdir_cb_count;
static int mkdtemp_cb_count;
static int rmdir_cb_count;
static int readdir_cb_count;
static int stat_cb_count;
Expand Down Expand Up @@ -93,6 +94,8 @@ static uv_fs_t write_req;
static uv_fs_t unlink_req;
static uv_fs_t close_req;
static uv_fs_t mkdir_req;
static uv_fs_t mkdtemp_req1;
static uv_fs_t mkdtemp_req2;
static uv_fs_t rmdir_req;
static uv_fs_t readdir_req;
static uv_fs_t stat_req;
Expand Down Expand Up @@ -376,6 +379,32 @@ static void mkdir_cb(uv_fs_t* req) {
}


static void check_mkdtemp_result(uv_fs_t* req) {
int r;

ASSERT(req->fs_type == UV_FS_MKDTEMP);
ASSERT(req->result == 0);
ASSERT(req->path);
ASSERT(strlen(req->path) == 15);
ASSERT(memcmp(req->path, "test_dir_", 9) == 0);
ASSERT(memcmp(req->path + 9, "XXXXXX", 6) != 0);
check_permission(req->path, 0700);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I it possible that default permissions change depending on the system?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These permissions are described in man mkdtemp on my system. I think this is the part of POSIX definition of mkdtemp.


/* Check if req->path is actually a directory */
r = uv_fs_stat(uv_default_loop(), &stat_req, req->path, NULL);
ASSERT(r == 0);
ASSERT(((uv_stat_t*)stat_req.ptr)->st_mode & S_IFDIR);
uv_fs_req_cleanup(&stat_req);
}


static void mkdtemp_cb(uv_fs_t* req) {
ASSERT(req == &mkdtemp_req1);
check_mkdtemp_result(req);
mkdtemp_cb_count++;
}


static void rmdir_cb(uv_fs_t* req) {
ASSERT(req == &rmdir_req);
ASSERT(req->fs_type == UV_FS_RMDIR);
Expand Down Expand Up @@ -927,6 +956,38 @@ TEST_IMPL(fs_async_sendfile) {
}


TEST_IMPL(fs_mkdtemp) {
int r;
const char* path_template = "test_dir_XXXXXX";

loop = uv_default_loop();

/* async mkdtemp, store result value in buf */
r = uv_fs_mkdtemp(loop, &mkdtemp_req1, path_template, mkdtemp_cb);
ASSERT(r == 0);

uv_run(loop, UV_RUN_DEFAULT);
ASSERT(mkdtemp_cb_count == 1);

/* sync mkdtemp */
r = uv_fs_mkdtemp(loop, &mkdtemp_req2, path_template, NULL);
ASSERT(r == 0);
check_mkdtemp_result(&mkdtemp_req2);

/* mkdtemp return different values on subsequent calls */
ASSERT(strcmp(mkdtemp_req1.path, mkdtemp_req2.path) != 0);

/* Cleanup */
rmdir(mkdtemp_req1.path);
rmdir(mkdtemp_req2.path);
uv_fs_req_cleanup(&mkdtemp_req1);
uv_fs_req_cleanup(&mkdtemp_req2);

MAKE_VALGRIND_HAPPY();
return 0;
}


TEST_IMPL(fs_fstat) {
int r;
uv_fs_t req;
Expand Down
2 changes: 2 additions & 0 deletions test/test-list.h
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@ TEST_DECLARE (fs_file_sync)
TEST_DECLARE (fs_file_write_null_buffer)
TEST_DECLARE (fs_async_dir)
TEST_DECLARE (fs_async_sendfile)
TEST_DECLARE (fs_mkdtemp)
TEST_DECLARE (fs_fstat)
TEST_DECLARE (fs_chmod)
TEST_DECLARE (fs_chown)
Expand Down Expand Up @@ -548,6 +549,7 @@ TASK_LIST_START
TEST_ENTRY (fs_file_write_null_buffer)
TEST_ENTRY (fs_async_dir)
TEST_ENTRY (fs_async_sendfile)
TEST_ENTRY (fs_mkdtemp)
TEST_ENTRY (fs_fstat)
TEST_ENTRY (fs_chmod)
TEST_ENTRY (fs_chown)
Expand Down