This repository has been archived by the owner. It is now read-only.

Implement uv_fs_mkdtemp #1368

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@Hinidu
Copy link
Contributor

Hinidu commented Jul 13, 2014

I implemented uv_fs_mkdtemp for unices and created a test for that. Could someone confirm that I am on the right way or provide some suggestions before I start implementing uv_fs_mkdtemp for Windows? Specifically I'm in doubt whether uv_fs_t->ptr is the right place for storing the name of temp directory which was created by underlying mkdtemp.

For Windows I want to use glibc's implementation which uses __gen_tempname.

Should close #1315

const char* template,
uv_fs_cb cb) {
INIT(MKDTEMP);
req->ptr = strdup(template);

This comment has been minimized.

@saghul

saghul Jul 13, 2014

Contributor

I'd use req->path.

@@ -779,6 +779,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, mkdtemp(req->ptr) ? 0 : -1);

This comment has been minimized.

@saghul

saghul Jul 13, 2014

Contributor

The result is assigned to 'r', which is of type ssize_t, I think you'll need to wrap this into some uv__fs_mkdtemp which returns 0 on success and -1 on error.

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Jul 14, 2014

@saghul thank you for suggestions! Does it seem good now?

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


int uv_fs_mkdtemp(uv_loop_t* loop,
uv_fs_t* req,
const char* path,

This comment has been minimized.

@saghul

saghul Jul 14, 2014

Contributor

Call this 'template'.

const char* path,
uv_fs_cb cb) {
INIT(MKDTEMP);
PATH;

This comment has been minimized.

@saghul

saghul Jul 14, 2014

Contributor

I'd do the strdup manually here, since you are storing the template, it's not a path yet :-) Also, remember to check for errors.

This comment has been minimized.

@Hinidu

Hinidu Jul 14, 2014

Contributor

Ok, I thought about that but decided save some lines :-) I'll change it.

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Jul 14, 2014

@saghul fixed last two issues.

@@ -787,6 +803,12 @@ TEST_IMPL(fs_async_dir) {
uv_run(loop, UV_RUN_DEFAULT);
ASSERT(mkdir_cb_count == 1);

r = uv_fs_mkdtemp(loop, &mkdtemp_req, "test_dir_XXXXXX", mkdtemp_cb);

This comment has been minimized.

@saghul

saghul Jul 14, 2014

Contributor

Better add a new test, even if you need to copy some bolierplate. Also, if you create the file inside a temp directory you can just remove the entire directory, instead of saving the filename and removing that one then.

This comment has been minimized.

@Hinidu

Hinidu Jul 14, 2014

Contributor

Ok, I'll add new test. I didn't create the file inside a temp directory yet, I'll add code for that in test too.
Btw how can I remove entire directory? Does uv_fs_rmdir remove directory recursively?

This comment has been minimized.

@saghul

saghul Jul 14, 2014

Contributor

Btw how can I remove entire directory? Does uv_fs_rmdir remove directory recursively?

I don't recall from the top of my head, but I think some test already does it...

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Jul 18, 2014

@saghul I've pushed the new version of test implementation. Do you (or anyone else) have other suggestions?

ASSERT(req == &mkdtemp_req);
check_mkdtemp_result(req);
mkdtemp_cb_count++;
strcpy(buf, req->path);

This comment has been minimized.

@saghul

saghul Jul 18, 2014

Contributor

Better remove the file here, instead of copying the path to buf.

This comment has been minimized.

@Hinidu

Hinidu Jul 18, 2014

Contributor

I did this to compare results of two uv_fs_mkdtemp calls. But I can also implement it by adding mkdtemp_req2 and by calling uv_fs_req_cleanup in test body instead of mkdtemp_cb. Is this more appropriate way for you?

This comment has been minimized.

@saghul

saghul Jul 18, 2014

Contributor

Sounds good!

ASSERT(memcmp(req->path + 9, "XXXXXX", 6) != 0);
check_permission(req->path, 0700);

/* Create a file to check if req->path is actually a directory */

This comment has been minimized.

@saghul

saghul Jul 18, 2014

Contributor

Better use uv_fs_stat here.

This comment has been minimized.

@Hinidu

Hinidu Jul 18, 2014

Contributor

Could you please explain how to do it in a portable way? I think about mode & S_IFDIR but S_IFDIR seems to be not defined in Windows, though I'm not sure about it.

This comment has been minimized.

@Hinidu

Hinidu Jul 18, 2014

Contributor

S_IFDIR is already used in fs_symlink_dir test, so perhaps I actually can use it too.

This comment has been minimized.

@saghul

saghul Jul 18, 2014

Contributor

Yep.

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);

This comment has been minimized.

@saghul

saghul Jul 18, 2014

Contributor

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

This comment has been minimized.

@Hinidu

Hinidu Jul 18, 2014

Contributor

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

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Jul 19, 2014

@saghul thank you again for review! I've implemented your suggestions.

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

This comment has been minimized.

@txdv

txdv Jul 19, 2014

Contributor

Are you freeing this anywhere?

This comment has been minimized.

@saghul

saghul Jul 19, 2014

Contributor

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.

This comment has been minimized.

This comment has been minimized.

@txdv

txdv Jul 19, 2014

Contributor

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.

This comment has been minimized.

@saghul

saghul Jul 19, 2014

Contributor

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.

This comment has been minimized.

@Hinidu

Hinidu Jul 19, 2014

Contributor

@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.

This comment has been minimized.

@txdv

txdv Jul 19, 2014

Contributor

@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

This comment has been minimized.

@saghul

saghul Jul 19, 2014

Contributor

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.

This comment has been minimized.

@txdv

txdv Jul 19, 2014

Contributor

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

This comment has been minimized.

@saghul

saghul Jul 19, 2014

Contributor

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.

@saghul saghul added this to the v0.12 milestone Jul 19, 2014

@saghul

This comment has been minimized.

Copy link
Contributor

saghul commented Jul 22, 2014

@Hinidu ping, do you think you'll have time to complete the Windows part soon?

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Jul 22, 2014

@saghul actually I thought to ask tomorrow whether you have any other suggestions or I can proceed with Windows part :-) I hope tomorrow I will have time to implement uv_fs_mkdtemp for Windows. I think that it would be the most complex part of this PR.
Btw if you don't have objections I will squash last two commits for the unittest into one.

@saghul

This comment has been minimized.

Copy link
Contributor

saghul commented Jul 22, 2014

Fantastic news! :-) Don't worry about squashing, it can be done at the end, once we have the code ready.

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Jul 23, 2014

@saghul I've pushed implementation for Windows. Make it work was simpler than I thought in the beginning :-) Now it passes my test locally.
Actually I ported glibc's implementation which uses __gen_tempname.
I have some questions which I'll post under particular lines shortly.

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


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

This comment has been minimized.

@Hinidu

Hinidu Jul 23, 2014

Contributor

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

This comment has been minimized.

@saghul

saghul Jul 23, 2014

Contributor

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.

static uint64_t value;
unsigned int count;
int fd = -1;
int save_errno = errno;

This comment has been minimized.

@Hinidu

Hinidu Jul 23, 2014

Contributor

How I should handle errno? Should I replace it with _doserrno? I didn't find errno usages in Windows code, but found one _doserrno.
EDIT: MSDN says that _wmkdir sets errno in failure cases.

This comment has been minimized.

@saghul

saghul Jul 23, 2014

Contributor

No need to save the errno here.

static const WCHAR letters[] =
L"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
int len;
WCHAR* XXXXXX;

This comment has been minimized.

@saghul

saghul Jul 23, 2014

Contributor

please use a better variable name here, like 'template'

This comment has been minimized.

@Hinidu

Hinidu Jul 23, 2014

Contributor

I suggest template_part because it is not the entire template but just the XXXXXX part of it. Agree?

This comment has been minimized.

@saghul

saghul Jul 23, 2014

Contributor

Ok.

}

/* We got out of the loop because we ran out of combinations to try. */
errno = EEXIST;

This comment has been minimized.

@saghul

saghul Jul 23, 2014

Contributor

no need to set errno


/* We got out of the loop because we ran out of combinations to try. */
errno = EEXIST;
SET_REQ_UV_ERROR(req, UV_EEXIST, EEXIST);

This comment has been minimized.

@saghul

saghul Jul 23, 2014

Contributor

SET_REQ_RESULT will do the job.

This comment has been minimized.

@Hinidu

Hinidu Jul 23, 2014

Contributor

SET_REQ_RESULT(req, -1)? If I understand correctly it will use _doserrno which already should be equal to EEXIST because it is the only one way to get to that line. Am I right?

This comment has been minimized.

@saghul

saghul Jul 23, 2014

Contributor

SET_REQ_RESULT(req, fd), since fd will be -1 anyway. Also above.

This comment has been minimized.

@Hinidu

Hinidu Jul 23, 2014

Contributor

Wouldn't it be more unclear? In the normal circumstances fd is not the result of the entire function.

This comment has been minimized.

@saghul

saghul Jul 23, 2014

Contributor

Right, let's go with -1 then.

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

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

This comment has been minimized.

@saghul

saghul Jul 23, 2014

Contributor

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

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Jul 23, 2014

@saghul I've pushed your suggestions.

@saghul

This comment has been minimized.

Copy link
Contributor

saghul commented Jul 23, 2014

Nice! I'll take a deeper look tonight, thanks!

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Jul 23, 2014

Thank you, I'll wait final review!

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

/* Magic number 7777 was borrowed from the glibc's implementation */

This comment has been minimized.

@saghul

saghul Jul 23, 2014

Contributor

Please move the comment to outside the function, just say it's borrowed from glibc.

This comment has been minimized.

@Hinidu

Hinidu Jul 24, 2014

Contributor
/* Some parts of the implementation was borrowed from glibc. */

Does this comment look good to you?

@saghul

This comment has been minimized.

Copy link
Contributor

saghul commented Jul 23, 2014

Other than clarifying to me the wide char thing, LGTM. Test pass: http://jenkins.nodejs.org/job/libuv-pullrequest/631/ Can you PTAL, @indutny?

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Jul 24, 2014

@saghul fixed comment, squashed.

WCHAR* template_part;
static uint64_t value;
unsigned int count;
int fd = -1;

This comment has been minimized.

@saghul

saghul Jul 24, 2014

Contributor

No need to initialize this, right?

This comment has been minimized.

@Hinidu

Hinidu Jul 24, 2014

Contributor

👍

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


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

This comment has been minimized.

@Hinidu

Hinidu Jul 24, 2014

Contributor

Note to self: typo was -> were

void fs__mkdtemp(uv_fs_t* req) {
static const WCHAR letters[] =
L"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
int len;

This comment has been minimized.

@Hinidu

Hinidu Jul 24, 2014

Contributor

I think better to change it to size_t. Do you agree @saghul?

This comment has been minimized.

@saghul

saghul Jul 24, 2014

Contributor

yeah

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Jul 24, 2014

Should I do git commit --amend && git push --force for those small changes or should I add new commit for them to squash it after final review?

@saghul

This comment has been minimized.

Copy link
Contributor

saghul commented Jul 24, 2014

Just force push, thanks!
On Jul 24, 2014 10:18 AM, "Pavel Platto" notifications@github.com wrote:

Should I do git commit --amend && git push --force for those small
changes or should I add new commit for them to squash it after final review?


Reply to this email directly or view it on GitHub
#1368 (comment).

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Jul 24, 2014

@saghul Done!

@saghul

This comment has been minimized.

Copy link
Contributor

saghul commented Jul 30, 2014

@indutny Ping.

INIT(MKDTEMP);
req->path = strdup(template);
if (req->path == NULL)
return -ENOMEM;

This comment has been minimized.

@indutny

indutny Jul 30, 2014

Contributor

What about ENOTDIR ?

This comment has been minimized.

@indutny

indutny Jul 30, 2014

Contributor

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

This comment has been minimized.

@saghul

saghul Jul 30, 2014

Contributor

This is just checking the result after strdup.

/* Some parts of the implementation were borrowed from glibc. */
void fs__mkdtemp(uv_fs_t* req) {
static const WCHAR letters[] =
L"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";

This comment has been minimized.

@indutny

indutny Jul 30, 2014

Contributor

hahahaha!

This comment has been minimized.

@Hinidu

Hinidu Jul 30, 2014

Contributor

It was also borrowed from glibc :-)

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

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

This comment has been minimized.

@indutny

indutny Jul 30, 2014

Contributor

Not true randomness! :) You could do better!

This comment has been minimized.

@saghul

saghul Jul 30, 2014

Contributor

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

@indutny

This comment has been minimized.

Copy link
Contributor

indutny commented Jul 30, 2014

LGTM, if it works

@saghul

This comment has been minimized.

Copy link
Contributor

saghul commented Jul 30, 2014

Tests pass, will land tonight then. Thanks for the review Fedor!

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Jul 30, 2014

LGTM, if it works

Thank you for review!

Tests pass, will land tonight then.

Great! This code will go to 0.11.27? Is there an approximate release date of this version?

@saghul

This comment has been minimized.

Copy link
Contributor

saghul commented Jul 30, 2014

Great! This code will go to 0.11.27? Is there an approximate release date of this version?

Not sure if we'll release any libuv 0.11.x before 0.12. About the release date: it will be ready when it's ready :-)

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Jul 30, 2014

Not sure if we'll release any libuv 0.11.x before 0.12. About the release date: it will be ready when it's ready :-)

Ok, then I'll try to keep an eye on this so I hope I don't miss next release :-)

@saghul

This comment has been minimized.

Copy link
Contributor

saghul commented Jul 31, 2014

Landed in master, thanks for working on this @Hinidu!

@saghul saghul closed this Jul 31, 2014

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Jul 31, 2014

Landed in master, thanks for working on this @Hinidu!

I'm very glad to hear it! Thank you and all other collaborators for your great library!

Hinidu added a commit to Hinidu/neovim that referenced this pull request Aug 5, 2014

cmake: upgrade libuv 0.11.26 -> 0.11.27
Adds new `uv_fs_mkdtemp` function which we need for cross-platform temp
directory creation.

ref:
- neovim#812
- joyent/libuv#1368
- https://github.com/joyent/libuv/releases
@@ -721,6 +721,78 @@ void fs__mkdir(uv_fs_t* req) {
}


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

This comment has been minimized.

@sam-github

sam-github Aug 6, 2014

Is incorporating LGPL source into an MIT licensed project legal?

This comment has been minimized.

@bnoordhuis

bnoordhuis Aug 6, 2014

Contributor

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.

This comment has been minimized.

@saghul

saghul Aug 6, 2014

Contributor

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.

This comment has been minimized.

@Hinidu

Hinidu Aug 7, 2014

Contributor

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.

@saghul

This comment has been minimized.

Copy link
Contributor

saghul commented Aug 7, 2014

I think we can go with the MIT licensed musl libc's implementation: https://github.com/runtimejs/musl-libc/blob/master/src/temp/mkdtemp.c and https://github.com/runtimejs/musl-libc/blob/master/src/temp/__randname.c.

Same license, so there should be no problem.

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Aug 7, 2014

@saghul I've pushed #1402 right now. But if musl's version is more desirable than I can use it.

@saghul

This comment has been minimized.

Copy link
Contributor

saghul commented Aug 7, 2014

Oh, it's ok, will review now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.