Implement issue# 13996. Add File.tempFile. #2956
Conversation
| version(Posix) | ||
| { | ||
| import core.sys.posix.sys.stat, core.sys.posix.fcntl; | ||
| enum flags = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH; |
There was a problem hiding this comment.
Quoting mktemp here.
When creating a file, the resulting file has read and write permissions for the current user, but no permissions for the group or others; these permissions are reduced if the current umask is more restrictive.
There was a problem hiding this comment.
So, I take it that you're suggesting that this just be
enum flags = S_IRUSR | S_IWUSR;
I guess that that makes sense. It's been so long since I wrote that that I don't even remember why I picked those permissions - probably because it was the least restrictive.
|
Great looks really good, much simpler than I thought due to the existing fdopen and name. |
| chars[0 .. letters.length] = letters[]; | ||
| chars[letters.length .. $] = digits[]; | ||
|
|
||
| foreach(ref c; name[prefix.length .. $]) |
There was a problem hiding this comment.
I guess that that's what I get for just quickly throwing the suffix in there, since all it had before today was a prefix.
|
Okay. I made changes per the comments. |
| dir = Directory of the temporary file. Defaults to the result of | ||
| $(XREF file, tempDir). | ||
| */ | ||
| static File tempFile(string prefix = null, string suffix = null) |
There was a problem hiding this comment.
If the documentation is generated with warnings it will complain about no parameter for dir:
Warning: Ddoc: function declaration has no parameter 'dir'
If you put the documentation on the other overload instead it will not complain.
There was a problem hiding this comment.
Well, that's ugly, since the overload without dir is the short one. I'd have hoped that the compiler would be smart enough to not give a warning when one of the documented items matched the documentation - though honestly, even having warnings for documentation like that is a bit off when you consider cases like when a struct and function share documentation as sometimes occurs when a function returns a specific struct. So, while I can certainly see how such warnings could be useful, I question that they actually make sense ultimately.
Regardless, as ugly as it may be, I can rearrange the functions to avoid the warning.
|
So now we'll have both File.tmpfile and File.tempFile, each with slightly different behaviour? :( |
I would love to get rid of |
|
I think we should rename |
I don't think that is a name a developer would look for when trying to create a temporary file. |
|
@jmdavis Isn't reopening a tempfile with a fixed filename vulnerable to security holes? E.g., if somebody successfully guesses the filename and creates it between the time it's closed and the time it's open, with maliciously-crafted file permissions/ownerships? |
|
Andrei has a point, because if the file is not automatically deleted, it's not really temporary. Anyway, maybe there could be a compromise, but I don't know whether it's possible: Could the file be deleted at first, so that it automatically gets released when the program ends, but as soon as someone accesses the file name, a link (hardlink) in [1] http://man7.org/linux/man-pages/man2/open.2.html |
|
@quickfur It's opened with |
I don't really care. It's functionality itself that matters, but I've never heard anyone use the term "scratch file, " and I'd expect anyone looking for this sort of functionality to be looking for something referring to "temporary files." So, I would think that it would be missed much more easily if it were called
@quickfur I honestly have no idea what the purpose of a file is if you can't close it and reopen it. Obviously, someone has a use for that, or
It's my understanding that most temporary files go into a directory where the OS will delete it later (typically when rebooting). Anyone who doesn't want to rely on that can just delete it themselves when they're done with it. But having the file deleted when it has been closed is something that I have never found a use for, and I have frequently needed to be able create temporary files that I've needed to reopen (especially in unit tests, though the need does come up occasionally as part of the normal operation of a program). So, I see no need to complicate things by trying to figure out how to the delete the file automatically, especially when it's trivial enough for the progam to take care of that itself if it cares. |
|
Okay. It's now named |
jmdavis
force-pushed
the
tempFile
branch
2 times, most recently
from
c2ec5f9
to
75102a5
Compare
| return retval; | ||
| } | ||
|
|
||
| throw new Exception("Failed to create a temporary file."); |
There was a problem hiding this comment.
I'm not too happy with this kind of exception usage. Basically you tried 3 times and then fail with an unspecific error, that doesn't indicate how to recover from the issue.
There was a problem hiding this comment.
Given the odds and an added errno check, failing 3 times would indicate a bad RNG or a hacking attempt.
There was a problem hiding this comment.
I'm not too happy with this kind of exception usage. Basically you tried 3 times and then fail with an unspecific error, that doesn't indicate how to recover from the issue.
I agree. I commented on the ErrnoException that was used previously. Throwing a plain Exception is not any better.
There was a problem hiding this comment.
Okay. It now throws an ErrnoException for the last failed open attempt. So, it won't tell you why the others failed, but you could check the error code of the last one to see whether it was a permissions problem or whatever. I'm not sure what else you might want me to do here. The only other thing that I can think of would be to create a new exception type, which would be overkill IMHO.
There was a problem hiding this comment.
ErrnoException is just the right thing, because it contains a human-readable error message.
There was a problem hiding this comment.
ErrnoException is just the right thing, because it contains a human-readable error message.
ErrnoException is always the wrong thing and shouldn't exist in the first place, see my comment.
There was a problem hiding this comment.
I'm not sure what else you might want me to do here. The only other thing that I can think of would be to create a new exception type, which would be overkill IMHO.
Yes, that's exactly what you should do. In general there's way to much laziness when it comes to exceptions in Phobos. Basically everywhere enforce is used which by default throws a plain Exception. It's not possible to do good error handling with exception types that are loosely defined on what they mean.
This adds a File.scratchFile, which generates a random file name and returns an open std.stdio.File for it. Unlike with File.tmpfile, it's a normal file which is _not_ deleted when the file is closed, and you actually have access to the file's name, which is necessary in many situations - particularly when writing unit tests that need to write to a file and then read from it.
|
Updated per most recent comments. |
|
Auto-merge toggled on |
Implement issue# 13996. Add File.tempFile.
| limit, filename); | ||
| throw new ErrnoException(msg); | ||
| } | ||
| continue; |
There was a problem hiding this comment.
Would be slightly better to only retry when errno was EEXISTS, i.e. replace the if block above with errnoEnforce(errno == EEXISTS, msg), because none of the other open errors will go away by retying (http://linux.die.net/man/2/open).
|
This pull request caused a +508KB (684KB -> 1191KB) increase in the size of a compiled "Hello, world" program. |
|
Are static functions not emitted to a separate object like free standing functions? |
|
It's probably due to the new imports. |
|
Treemaps: It is now pulling in:
|
|
I hate static constructors and how they bloat statically linked binaries. |
|
There's also Object.factory, which is pulling in all declared classes' virtual methods and everything they use, but I'm not sure if it's at play here. |
A lot of this stuff should be done using weak-linkage, but we should also finally gear towars a shared phobos library. |
|
I'm reverting this because of the disastrous executable file size increase in code that doesn't use this new function. The functionality needs a redesign. |
|
A simple but hackish fix would be to make the functions templated (add empty TemplateArgumentList). The imports will only be pulled in when the function is called (and the template instantiated). The downside is that things such as taking the address of the functions will fail and require an explicit instantiation ( |
I wouldn't really consider that to be much of a downside, since I can't imagine that it's a common thing to do with a function like |
|
Right, I'm just saying that this isn't a universal fix, and doesn't handle the root of the issue. |
|
Wait a second, this means anyone using the function would have to run semantic, import those module, and optimize the code. Right now this only has to be done one when compiling phobos. Please let's address the real cause. |
|
digger bisect returned this commit as cause of https://issues.dlang.org/show_bug.cgi?id=14828 |
|
Binary size increase mostly seems to come from using std.file (tempDir). Maybe we can move the implementation of that into a std.internal.file module. Of course we could also split std.file into a package, but that wasn't received too well lately. |
|
Also weird that a static function of a struct get's dragged into the executable without that function being used. Maybe we need to tweak our multilib code in the compiler. |
This adds a File.tempFile, which generates a random file name and
returns an open std.stdio.File for it. Unlike with File.tmpfile, it's a
normal file which is not deleted when the file is closed, and you
actually have access to the file's name, which is necessary in many
situations - particularly when writing unit tests that need to write to
a file and then read from it.