Skip to content
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
15 changes: 15 additions & 0 deletions changelog/std-file-createTempFile.dd
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
`createTempFile` added to std.file

$(REF std, file, createTempFile) creates a file in $(REF tempDir, std, file)
with a random name (with an optionally provided prefix and suffix) and returns
Copy link
Member

Choose a reason for hiding this comment

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

repeated "with", use e.g. s/with an/including an/

the file name. It also optionally takes data to write to the file when creating
it (if no data is provided, then the newly created file will be empty).
Copy link
Member

Choose a reason for hiding this comment

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

It's awkward that the file is created, closed, to presumably be opened by the user again right after. Shouldn't this function return e.g. an opened File? Then people can call name to get its name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Returning an open File would fit in std.stdio, not std.file, because std.file specifically only ever deals with entire files at at time. Also, in most cases, I'd personally find it very annoying to have a File to deal with. The main purpose is to write data to a file for testing or to pass to another program and then pass the file name to something else to open it. Having an open File rather than just a filename just makes that use case more annoying. We did have a version of this in std.stdio briefly that did that, but it got removed from std.stdio on the grounds that it increased the size of "hello world" due to issues with the compiler.

The way I would expect to use this normally would be to just call this function like write and write the whole thing at once, and in the rarer case where they want a File, they can just not pass any data to it and then reopen it with File (in which case, since the file was already created, there is no security risk like there is the first time).


This is different from $(REF File.tmpfile, std, stdio), which creates a
temporary file with no name, returns a $(REF File, std, stdio) to that file,
and then deletes the file when the $(REF File, std, stdio) is closed. The file
created by $(REF std, file, createTempFile) is a normal file which can be
opened and closed as many times as desired and will only be deleted by the
program if the program explicitly does so (though it is sitting in a temp
Copy link
Member

Choose a reason for hiding this comment

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

s/is sitting/is created/

directory and is subject to the normal things tha happen to such files - e.g.
Copy link
Member

Choose a reason for hiding this comment

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

"normal things" is too colloquial, please rephrase

Copy link
Member

Choose a reason for hiding this comment

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

typo: "tha"

some OSes delete those files on boot).
217 changes: 190 additions & 27 deletions std/file.d
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ $(TR $(TD Files) $(TD
$(LREF remove)
$(LREF slurp)
$(LREF write)
$(LREF createTempFile)
))
$(TR $(TD Symlinks) $(TD
$(LREF symlink)
Expand Down Expand Up @@ -809,8 +810,6 @@ if (isConvertibleToString!R)
static assert(__traits(compiles, append(TestAliasedString("foo"), [0, 1, 2, 3])));
}

// Posix implementation helper for write and append

Copy link
Member

Choose a reason for hiding this comment

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

Why?

version(Posix) private void writeImpl(const(char)[] name, const(FSChar)* namez,
in void[] buffer, bool append) @trusted
{
Expand All @@ -821,27 +820,9 @@ version(Posix) private void writeImpl(const(char)[] name, const(FSChar)* namez,
: O_CREAT | O_WRONLY | O_TRUNC;

immutable fd = core.sys.posix.fcntl.open(namez, mode, octal!666);
cenforce(fd != -1, name, namez);
{
scope(failure) core.sys.posix.unistd.close(fd);

immutable size = buffer.length;
size_t sum, cnt = void;
while (sum != size)
{
cnt = (size - sum < 2^^30) ? (size - sum) : 2^^30;
const numwritten = core.sys.posix.unistd.write(fd, buffer.ptr + sum, cnt);
if (numwritten != cnt)
break;
sum += numwritten;
}
cenforce(sum == size, name, namez);
}
cenforce(core.sys.posix.unistd.close(fd) == 0, name, namez);
Copy link
Member

Choose a reason for hiding this comment

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

General note, separating related refactorings into their own commit makes reviews easier and faster

writeToOpenFile(fd, buffer, name, namez);
Copy link
Member

Choose a reason for hiding this comment

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

who closes fd?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at the code, writeToOpenFile does. I suppose that I could rename it to writeToOpenFileAndClose or something to make it clearer. It's just a private function that was created to refactor some code that writeImpl uses rather than duplicating it.

}

// Windows implementation helper for write and append

version(Windows) private void writeImpl(const(char)[] name, const(FSChar)* namez,
in void[] buffer, bool append) @trusted
{
Expand All @@ -868,20 +849,202 @@ version(Windows) private void writeImpl(const(char)[] name, const(FSChar)* namez
h = CreateFileW(namez, defaults);
cenforce(h != INVALID_HANDLE_VALUE, name, namez);
}
writeToOpenFile(h, buffer, name, namez);
}

private void writeToOpenFile(FD)(FD fd, const void[] buffer, const(char)[] name,
const(FSChar)* namez) @trusted
{
immutable size = buffer.length;
size_t sum, cnt = void;
DWORD numwritten = void;

while (sum != size)
{
cnt = (size - sum < 2^^30) ? (size - sum) : 2^^30;
WriteFile(h, buffer.ptr + sum, cast(uint) cnt, &numwritten, null);
if (numwritten != cnt)
cnt = size - sum < 2^^30 ? size - sum : 2^^30;
Copy link
Member

Choose a reason for hiding this comment

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

Why? The parens were redundant but they were clearly there for a reason (readability).

Copy link
Member

Choose a reason for hiding this comment

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

Also, mixing stylistic changes, refactorings, and adding new code, all in one commit, is pretty bad. Please avoid doing this in the future.


version(Posix)
immutable numWritten = core.sys.posix.unistd.write(fd, buffer.ptr + sum, cnt);
else version(Windows)
{
DWORD numWritten = void;
WriteFile(fd, buffer.ptr + sum, cast(uint) cnt, &numWritten, null);
}
else
static assert(0, "Unsupported OS");

if (numWritten != cnt)
break;
sum += numwritten;
sum += numWritten;
}
cenforce(sum == size && CloseHandle(h), name, namez);

version(Posix)
cenforce(sum == size && core.sys.posix.unistd.close(fd) == 0, name, namez);
else version(Windows)
cenforce(sum == size && CloseHandle(fd), name, namez);
else
static assert(0, "Unsupported OS");
}


/++
Creates a file with a random name in $(LREF tempDir) and returns the name
of the file. If data is passed in, then the file is written with that data;
Copy link
Member

Choose a reason for hiding this comment

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

s/returns the name of the file/returns its name/

Generally the prose could use some editing.

otherwise, it's empty.

The idea is that this creates a temporary file for testing or whatever
Copy link
Contributor

Choose a reason for hiding this comment

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

"The idea is" sounds colloquial. How about simply "This creates ..."

Copy link
Member

Choose a reason for hiding this comment

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

Bump, this wasn't addressed.

Copy link
Member

Choose a reason for hiding this comment

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

"whatever" is also odd

other purpose a temporary file might be needed for. However, it will only
be deleted if it's explicitly deleted or if the OS decides to delete it as
some OSes do with files in temp directories on startup or shutdown. So,
unlike $(REF File.tmpfile, std, stdio), the file has a name, and it can have
all of the things done to it that one might typically do with a file
Copy link
Member

Choose a reason for hiding this comment

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

again with "things"...

(including reopening it after closing it).

The file is created with R/W permissions. On POSIX systems, the permissions
are restricted to the current user, though the effective permissions are
modified by the process' umask in the usual way.
Copy link
Member

Choose a reason for hiding this comment

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

Would it be important to allow configurable permissions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Important? I don't think so. We don't provide that ability with write, and that does start getting a bit system-specific to implement. It might be a reasonable enhancement, but if we did something like that, then we'd need to do the same with other functions like write. So, if we want to do that, I think that it should be left to a separate PR.


Copy link
Member

Choose a reason for hiding this comment

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

Could you add that it uses the global rndGen to generate the file names? This should allow us to close the rndGen discussion.

$(REF rndGen, std, random) is used as the random number generator.

Params:
prefix = Prefix to the random portion of the file name.
Copy link
Member

Choose a reason for hiding this comment

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

Can this be a directory path? In some terminology (e.g. autotools) "prefix" is implied to mean a path prefix. Please clarify.

suffix = Suffix to the random portion of the file name (e.g. for a file
extension).
buffer = Data to populate the file with.

Returns:
The name of the generated file.

Throws:
$(LREF FileException) if it fails to create the file.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we are really paranoid a note could be added that this function:

This function isn't designed to be cryptographically secure, and are therefore unsuitable for cryptographic or security-related purposes

But then again Captain Obvious tells you that using temporary files in a shared directory is a pretty bad idea for anything that claims to be secure.

Copy link
Member

Choose a reason for hiding this comment

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

Sometimes it's good to state the obvious, since what's obvious to us may not be obvious to others, e.g., a new learner of D.

See_Also:
$(REF File.tmpfile, std, stdio)
+/
string createTempFile(const void[] buffer = null) @safe
Copy link
Member

@andralex andralex Apr 7, 2018

Choose a reason for hiding this comment

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

This shouldn't be an overload. Consider: createTempFile("my_prefix_"). The user will get the wrong idea. Let them type null, null if they don't care.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fine with me. I'd expect the normal use case to involve passing data anyway. I suspect that I assumed that folks would find it annoying to have to pass null in the case where they didn't want to write data, but it's been a while since I wrote this, so I don't remember for sure.

{
return createTempFile(null, null, buffer);
}

/// Ditto
string createTempFile(const(char)[] prefix, const(char)[] suffix, const void[] buffer = null) @trusted
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not string or using templates?

Copy link
Member

Choose a reason for hiding this comment

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

If it were string and the prefix/suffix weren't already immutable then it would be necessary to create copies of them to pass to the function. What benefit would immutability provide in exchange?

If the prefix/suffix were template parameters then it would be impossible for them to be specified at runtime, so I think the question is whether using templates gives some benefit that justifies that limitation.

{
import std.path : absolutePath, baseName, buildPath, dirName;

static string genTempName(const(char)[] prefix, const(char)[] suffix)
{
import std.ascii : digits, letters;
import std.random : choice, rndGen;
import std.range : chain;
import std.string : representation;

auto name = new char[](prefix.length + 15 + suffix.length);
name[0 .. prefix.length] = prefix;
name[$ - suffix.length .. $] = suffix;

auto random = &rndGen();
rndGen.popFront();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessary, choice takes rndGen by ref

Copy link
Member

Choose a reason for hiding this comment

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

I think the idea here is that the value currently in rndGen.front could have already been used by someone else who didn't then pop it off. choice uses uniform which uses front then pops afterwards.


auto chars = chain(letters.representation, digits.representation);
foreach (ref c; name[prefix.length .. $ - suffix.length])
{
c = choice(chars);
random.popFront();
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't necessary. choice takes rndGen by ref: https://dlang.org/phobos/std_random.html#.choice

}

return buildPath(tempDir, name);
}

while (1)
Copy link
Member

Choose a reason for hiding this comment

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

Using 1 as true is a C-ism (which does not have a bool type). (Since D has a bool type, numbers should be used to indicate quantity, and 1 here does not indicate the quantity of anything.) Please use while (true) in D.

{
auto filename = genTempName(prefix, suffix);

version(Posix)
{
auto fd = open(tempCString!FSChar(filename), O_RDWR | O_CREAT | O_EXCL, S_IRUSR | S_IWUSR);
if (fd == -1)
{
immutable errno = .errno;
if (errno == EEXIST)
continue;
else
throw new FileException("Failed to create a temporary file", errno);
}
}
else version(Windows)
{
auto fd = CreateFileW(tempCString!FSChar(filename), GENERIC_WRITE, 0, null, CREATE_NEW,
FILE_ATTRIBUTE_NORMAL | FILE_FLAG_SEQUENTIAL_SCAN, HANDLE.init);
if (fd == INVALID_HANDLE_VALUE)
{
immutable errno = .GetLastError();
if (errno == ERROR_FILE_EXISTS)
continue;
else
throw new FileException("Failed to create a temporary file", errno);
}
}
else
static assert(0, "Unsupported OS");

writeToOpenFile(fd, buffer, filename, null);
Copy link
Member

Choose a reason for hiding this comment

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

Is returning without closing the file descriptor okay?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch! This is a raw OS handle, not a std.stdio.File, so there is no refcounting or GC or anything of that sort. So unless I'm missing something obvious, yes we will need to close the file descriptor.

Copy link
Member Author

@jmdavis jmdavis Apr 7, 2018

Choose a reason for hiding this comment

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

Actually, looking at the code, writeToOpenFile closes the file descriptor so that the code that involves closing the file descriptor is shared with writeImpl just like the code to actually write to the open file is. I guess that I should have named it in a way that made it clear.


return filename;
}
}

///
Copy link
Member

Choose a reason for hiding this comment

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

This is a fine unit test, but not a good example. Would be nice to have a human-friendly example alongside this unit test.

@safe unittest
{
import std.algorithm.searching : startsWith;
import std.range.primitives;
import std.path : baseName, buildNormalizedPath, dirName, extension;

{
auto path = createTempFile();
scope(exit) if (path.exists) remove(path);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why if (path.exists). Should this either be scope(exit) assert(path.exists), remove(path); or scope(exit) remove(path);?

Copy link
Member Author

Choose a reason for hiding this comment

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

The path.exists portion could be removed, but I pretty much always use it before removing a file so that I don't have to worry about remove throwing due to something that happened when the code is changed later or something else unforeseen goes wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

If tempFile does fail to create the file for some reason, then one of the tests after the scope statement will fail, and the AssertError will bubble up properly, but if the test isn't there, then in that case, we'd then get a FileException being thrown instead when remove failed. So, IMHO, it is better to have the check even if it's useless if everything is working correctly.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I've run into this before, and found myself wishing for an option for remove to ignore file-not-found errors. The last time I had to deal with this, I ended up calling core.sys.posix.unistd.remove directly instead. Unfortunately that introduces OS dependence.

Perhaps this could be filed as an enhancement request? Anybody interested in that?

assert(!path.empty);
assert(buildNormalizedPath(path.dirName) == buildNormalizedPath(tempDir));
assert(!path.baseName.empty);
assert(path.exists);
assert(path.isFile);
assert(readText(path).empty);
}
{
auto path = createTempFile("hello world");
scope(exit) if (path.exists) remove(path);
assert(!path.empty);
assert(buildNormalizedPath(path.dirName) == buildNormalizedPath(tempDir));
assert(!path.baseName.empty);
assert(path.exists);
assert(path.isFile);
assert(readText(path) == "hello world");
}
{
auto path = createTempFile("prefix_", ".txt");
scope(exit) if (path.exists) remove(path);
auto name = path.baseName;
assert(name.startsWith("prefix_"));
assert(name.extension == ".txt");
assert(name.length > "prefix_.txt".length);
assert(buildNormalizedPath(path.dirName) == buildNormalizedPath(tempDir));
assert(path.exists);
assert(path.isFile);
assert(readText(path).empty);
}
{
auto path = createTempFile(null, ".txt", "D rocks");
scope(exit) if (path.exists) remove(path);
auto name = path.baseName;
assert(name.extension == ".txt");
assert(name.length > ".txt".length);
assert(buildNormalizedPath(path.dirName) == buildNormalizedPath(tempDir));
assert(path.exists);
assert(path.isFile);
assert(readText(path) == "D rocks");
}
}


/***************************************************
* Rename file $(D from) _to $(D to).
* If the target file exists, it is overwritten.
Expand Down