Add mkstemp(3) #365

antifuchs · 2016-05-01T19:58:50Z

I've missed this call in a project that uses nix, so I added it to the unistd module; the placement is only a half-truth (as the header is stdlib.h on most systems, but some related functions are documented to live in unistd.h on BSD-derivatives), so I'm not sure my PR is completely right. But then, there is no stdlib module yet! Any guidance here is much appreciated! (-:

posborne · 2016-05-01T20:31:02Z

src/unistd.rs

+        }
+    }));
+    Errno::result(res)
+


Probably move the newline to before the final expression rather than after it.

posborne · 2016-05-01T20:46:51Z

Looks good to me. I agree that the placement within unistd is a bit odd. I'll let others take a look before merging to decide if we want something like a stdlib module or something else -- most stdlib.h functions have not been implemented as they have suitable alternatives in std.

Right now, I would lean toward creating that module versus putting it in unistd.

kamalmarhubi · 2016-05-01T21:42:28Z

Right now, I would lean toward creating that module versus putting it in unistd.

Same.

One thing that's slightly concerning: we are unable to communicate the actual file name back to the caller: because of how NixPath works, libc::mkstemp modifies a stack allocated buffer with a copy of the passed in template.

Pinging #221 #230 since this use case might matter for them.

kamalmarhubi · 2016-05-01T21:48:33Z

@antifuchs

I've missed this call in a project that uses nix, so I added it to the unistd module

Aside: for temporary files, I usually use either the tempdir or tempfile crates depending on the exact use case. tempfile's focus is on creating an unnamed file that is not linked into the filesystem. Most often I use tempdir and create files in there.

kamalmarhubi · 2016-05-01T21:57:43Z

Test failures are for Rust < 1.4, which didn't have Result::expect. I really like that method. :/

Ping #238.

antifuchs · 2016-05-01T22:06:04Z

Thanks a lot for the comments! Those are some excellent points (and agreed to a new module)! My use case is specifically for a fresh file descriptor (not a File) opened to an unlinked file, so this exact library call was perfect for me (and e.g. tempfile wouldn't be).

While I'm not sure what one would use the filename of an unlinked file for, losing the value isn't great either - we could either require a &mut str as the template, or return the actual path as a fresh string, both approximately equally displeasing for different reasons (-:

I'm going to remove the depentency on Result::expect, and will move that pesky newline, too.

kamalmarhubi · 2016-05-01T22:46:51Z

I thought that mkstemp will result in a linked file that you have to unlink yourself.

/me checks

kamalmarhubi · 2016-05-01T23:01:47Z

Yeah, mkstemp results in a linked file:

> cat tmpfiletest.c
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>
#include <unistd.h>

int main(void) {
        char filename[128];
        int fd;
        struct stat stat_buf;

        strncpy(filename, "/tmp/my-awesome-tmpfile.XXXXXX", sizeof(filename));

        fd = mkstemp(filename);
        if (fd == -1) {
                perror("mkstemp");
                exit(EXIT_FAILURE);
        }
        printf("file name is %s\n", filename);

        if (stat(filename, &stat_buf)) {
                perror("stat");
                exit(EXIT_FAILURE);
        }
        printf("file's inode: %ld\n", stat_buf.st_ino);
        pause();

        return 0;
}
$ gcc -Wall -Wextra -Werror tmpfiletest.c
$ ./a.out &
file name is /tmp/my-awesome-tmpfile.bwEU7q
file's inode: 3830951
$ ls -l /tmp/my-awesome-tmpfile.bwEU7q
-rw------- 1 kamal kamal 0 May  1 19:00 /tmp/my-awesome-tmpfile.bwEU7q
$ man ls
$ ls -il /tmp/my-awesome-tmpfile.bwEU7q
3830951 -rw------- 1 kamal kamal 0 May  1 19:00 /tmp/my-awesome-tmpfile.bwEU7q

kamalmarhubi · 2016-05-01T23:03:52Z

Non-portably, the best approach on Linux is to use open(2) with O_TMPFILE if an unlinked file is what you want:

O_TMPFILE (since Linux 3.11)

Create an unnamed temporary file. The pathname argument specifies a directory; an unnamed inode will be created in that directory's filesystem. Anything written to the resulting file will be lost when the last file descriptor is closed, unless the file is given a name.

―http://man7.org/linux/man-pages/man2/open.2.html

kamalmarhubi · 2016-05-01T23:10:17Z

we could either require a &mut str as the template

ugh strings and nix are so annoying. this is not ideal because str is always valid unicode, and we'd have to document the nul-termination and no inner nul requirement on the str.

/me goes to finish setting up the RFCs repo so we can get past #221.

antifuchs · 2016-05-01T23:39:23Z

Ahaha, eep, I totally missed the non-unlinkedness. You're completely right (and gosh, I should have taken a look at my /tmp). I'll adjust the test accordingly.

kamalmarhubi · 2016-05-02T21:47:41Z

The best I can think of for the template right now is to take it as &mut CStr. I think once we figure out NixString (#221, #230) we may be able to open it up to &mut T where T: NixString, but not sure.

kamalmarhubi · 2016-05-02T21:52:36Z

src/unistd.rs

-    let res = try!(template.with_nix_path(|path| {
-        let mut path_copy = path.to_bytes_with_nul().to_owned();
-        let c_template: *mut c_char = path_copy.as_mut_ptr() as *mut c_char;
+pub fn mkstemp<P: ?Sized + NixPath>(template: &P) -> Result<(RawFd, PathBuf)> {


I don't like this too much because it forces an allocation and copy. See #365 (comment) for another option.

Yeah, I don't much like that. Mutating the input parameter feels like a hack and a bit of a safety hazard (though the mut mark makes it a bit less terrifying), to be honest. Plus, CStr input parameters are always kind of messy - I just had reason to use memfd_create, and its signature is kind of a pain.

It won't be a safety hazard unless you unsafely obtained the mutable reference :-)

memfd_create is like open so it should be fine?

arcnmx · 2016-05-03T07:47:01Z

The best I can think of for the template right now is to take it as &mut CStr

As unfortunate as the type is, it does fit best... There's no way to create one though, even CString doesn't impl DerefMut in the way String does (and &mut str isn't even that well of a supported type). nix would have to provide some extension traits, at which point we may just be better off going with &mut [u8], and panic/error on invalid input (null bytes in wrong spot).

I think once we figure out NixString (#221, #230) we may be able to open it up to &mut T where T: NixString, but not sure.

that will be very tricky, especially if we continue to support fallible one-way conversions...

kamalmarhubi · 2016-05-03T15:32:41Z

@arcnmx

There's no way to create one though, even CString doesn't impl DerefMut in the way String does (and &mut str isn't even that well of a supported type).

Huh, that's unfortunate. I'll take a look at adding at least the DerefMut impl, which seems like an oversight.

It would be nice to have an RFC like rust-lang/rfcs#1309 for CString to fill in some gaps. Not all the gaps, since some of it doesn't make sense with the specified API of CStr not storing a length.

that will be very tricky, especially if we continue to support fallible one-way conversions...

Maybe a different trait for the mutable argument case?

arcnmx · 2016-05-03T18:20:57Z

Huh, that's unfortunate. I'll take a look at adding at least the DerefMut impl, which seems like an oversight.

At the same time maybe try adding some &mut self methods to CStr like unsafe to_bytes_mut and unsafe to_bytes_with_nul_mut. from_bytes_with_nul_mut and from_bytes_with_null_mut_unchecked would be nice too..! Otherwise there's not much you can really do with a &mut CStr...

(oh god these long names though, why couldn't we have just named them bytes and inner_bytes instead...)

It would be nice to have an RFC like rust-lang/rfcs#1309 for CString to fill in some gaps. Not all the gaps, since some of it doesn't make sense with the specified API of CStr not storing a length.

I'm not sure this is necessary, since the conversion from &CStr to &OsStr is already cost-free (minus the length check). Maybe we could add from_c_str() to std::os::unix::ffi::OsStrExt for convenience, since the current conversion is OsStrExt::from_bytes(cstr.to_bytes())

homu · 2016-09-07T16:27:55Z

☔ The latest upstream changes (presumably #416) made this pull request unmergeable. Please resolve the merge conflicts.

philippkeller · 2016-09-12T16:15:56Z

Since I also wanted to implement mkstemp in unistd I found this open PR.

I fixed it. This is the working diff: nix-mkstemp.diff

A few remarks

@antifuchs if you don't care about this open PR then I can start a new one, I didn't want to steal away your code and claim it's mine. Your code only needed like 2-3 line of fixing.
made it working with Rust 1.2 which needed to_bytes_with_nul(). I think the implementation is memory safe but would like to have a pair of eyes checking that
added documentation
replaced Path by PathBuf so it's more in line with getcwd
I didn't move it into another module, I would like to do that but in a separate PR (probably moving all stdio methods out)
it's true that unistd doesn't need mkstmp since there is the crate tempdir and tempfile, but I'd love to see this here for completeness

fiveop · 2016-09-15T19:58:44Z

You can create a fresh PR that builds on antifuchs's work:

add antifuchs' fork as a remote to your local git repository (edit .git/config)
[remote "antifuchs"] url = [email protected]:antifuchs/nix.git fetch = +refs/heads/*:refs/remotes/antifuchs/*
fetch from him: git fetch antifuchs
checkout his branch: git checkout youllhavetolookupthebranchnameyourself
add your commits (and whatever els is needed, a rebase probably)
push to your fork: git push -u yourremotename whateverthebranchiscalled
and open a PR

(I'm sorry, if I bore you with things you already know).

philippkeller · 2016-09-15T20:21:15Z

@fiveop thanks for the explanation. No, I didn't know this already. What's the advantage of doing it the way you suggested against just doing a 100% fresh PR with his mkstemp and test_mkstemp?

fiveop · 2016-09-15T20:53:36Z

His commits will show up in the git history (instead of just his code).

philippkeller · 2016-09-16T05:40:30Z

@fiveop thanks, I opened #428 now, I guess this PR can be closed then

antifuchs · 2016-09-16T15:30:20Z

Ahh, sorry for letting this sit so long & thanks for working on this, @philippkeller! I'll close this PR now in favor of yours!

kamalmarhubi · 2016-09-18T03:40:38Z

@fiveop @philippkeller an improvement on the get-the-PR workflow:

git fetch origin refs/pulls/365/head && git checkout -b my-new-branch-name FETCH_HEAD

:-)

fiveop · 2016-09-18T07:03:29Z

thanks :)

@antifuchs

Add Mkstemp (fixed for rust 1.2) I fixed @antifuchs addition of `mkstmp` in #365 by making it compile in Rust 1.2 and adding documentation. A few remarks: - made it working with Rust 1.2 which needed `to_bytes_with_nul()`. I think the implementation is memory safe but would like to have a pair of eyes checking that - replaced Path by PathBuf so it's more in line with getcwd - I didn't move it into another module. If this still the consensus then I would like to do that but in a separate PR (probably moving all stdio methods out) - it's true that unistd doesn't need mkstmp since there is the crate tempdir and tempfile, but I'd love to see this here for completeness

Add mkstemp(3)

a821f96

posborne reviewed May 1, 2016
View reviewed changes

src/unistd.rs

}

}));

Errno::result(res)

Copy link

Member

posborne May 1, 2016

Choose a reason for hiding this comment

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

Probably move the newline to before the final expression rather than after it.

Remove dependency on Result::expect

e42183f

Return both the fd and the created path

23a7ea6

kamalmarhubi mentioned this pull request May 2, 2016

Revisit passing of strings: redesign and replace NixPath #221

Open

kamalmarhubi reviewed May 2, 2016
View reviewed changes

philippkeller mentioned this pull request Sep 16, 2016

Add Mkstemp (fixed for rust 1.2) #428

Merged

antifuchs closed this Sep 16, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mkstemp(3) #365

Add mkstemp(3) #365

antifuchs commented May 1, 2016

posborne May 1, 2016

posborne commented May 1, 2016

kamalmarhubi commented May 1, 2016

kamalmarhubi commented May 1, 2016

kamalmarhubi commented May 1, 2016

antifuchs commented May 1, 2016

kamalmarhubi commented May 1, 2016

kamalmarhubi commented May 1, 2016

kamalmarhubi commented May 1, 2016 •

edited

Loading

O_TMPFILE (since Linux 3.11)

kamalmarhubi commented May 1, 2016

antifuchs commented May 1, 2016

kamalmarhubi commented May 2, 2016 •

edited

Loading

kamalmarhubi May 2, 2016

antifuchs May 2, 2016

kamalmarhubi May 2, 2016

kamalmarhubi May 2, 2016

arcnmx commented May 3, 2016

kamalmarhubi commented May 3, 2016

arcnmx commented May 3, 2016

homu commented Sep 7, 2016

philippkeller commented Sep 12, 2016

fiveop commented Sep 15, 2016 •

edited

Loading

philippkeller commented Sep 15, 2016

fiveop commented Sep 15, 2016

philippkeller commented Sep 16, 2016

antifuchs commented Sep 16, 2016

kamalmarhubi commented Sep 18, 2016 •

edited

Loading

fiveop commented Sep 18, 2016

Add mkstemp(3) #365

Add mkstemp(3) #365

Conversation

antifuchs commented May 1, 2016

posborne May 1, 2016

Choose a reason for hiding this comment

posborne commented May 1, 2016

kamalmarhubi commented May 1, 2016

kamalmarhubi commented May 1, 2016

kamalmarhubi commented May 1, 2016

antifuchs commented May 1, 2016

kamalmarhubi commented May 1, 2016

kamalmarhubi commented May 1, 2016

kamalmarhubi commented May 1, 2016 • edited Loading

O_TMPFILE (since Linux 3.11)

kamalmarhubi commented May 1, 2016

antifuchs commented May 1, 2016

kamalmarhubi commented May 2, 2016 • edited Loading

kamalmarhubi May 2, 2016

Choose a reason for hiding this comment

antifuchs May 2, 2016

Choose a reason for hiding this comment

kamalmarhubi May 2, 2016

Choose a reason for hiding this comment

kamalmarhubi May 2, 2016

Choose a reason for hiding this comment

arcnmx commented May 3, 2016

kamalmarhubi commented May 3, 2016

arcnmx commented May 3, 2016

homu commented Sep 7, 2016

philippkeller commented Sep 12, 2016

fiveop commented Sep 15, 2016 • edited Loading

philippkeller commented Sep 15, 2016

fiveop commented Sep 15, 2016

philippkeller commented Sep 16, 2016

antifuchs commented Sep 16, 2016

kamalmarhubi commented Sep 18, 2016 • edited Loading

fiveop commented Sep 18, 2016

kamalmarhubi commented May 1, 2016 •

edited

Loading

kamalmarhubi commented May 2, 2016 •

edited

Loading

fiveop commented Sep 15, 2016 •

edited

Loading

kamalmarhubi commented Sep 18, 2016 •

edited

Loading