-
Notifications
You must be signed in to change notification settings - Fork 665
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Mkstemp (fixed for rust 1.2) #428
Conversation
… to be *mut u8 whereas x86_64 and i686 expect it to be *mut i8)
r? @kamalmarhubi @arcnmx Strings are involved, that's your expertise. |
ping to @kamalmarhubi @arcnmx: could you check if that implementation is safe memory wise? I guess the question is if |
Yeah no that string is dropped by the time you use |
let fd = libc::mkstemp(p); | ||
let pathname = OsStr::from_bytes(CStr::from_ptr(p).to_bytes()); | ||
try!(Errno::result(fd)); | ||
Ok((fd, PathBuf::from(pathname).to_owned())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PathBuf::from
already creates an owned PathBuf, so to_owned()
is just making an additional copy. Also, it's unnecessary since path
is already owned in the first place; you can just strip out the trailing nul byte and use OsString::from_vec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, removed the double copying and used OsString::from_vec
to avoid the constructing of the string from pointer. Nice side effects: less includes and less code in the unsafe
block. Is there a nicer way to not have to use .pop()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nice way would've been to use CString::into_bytes
(OsString::from_vec(pathname.into_bytes())
) but that's new since Rust 1.7.
pop()
seems fine to me, maybe with a debug_assert!
that it == Some(b'\0')
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the tip, I added the debug_assert
…ing::from_vec on existing path var rather than construct string from pointer
…espite its documentation\!), checking the failure of a directory now
Looks good to me. Thank you and @antifuchs. @homu r+ |
📌 Commit b47120d has been approved by |
⚡ Test exempted - status |
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
I fixed @antifuchs addition of
mkstmp
in #365 by making it compile in Rust 1.2 and adding documentation.A few remarks:
to_bytes_with_nul()
. I think the implementation is memory safe but would like to have a pair of eyes checking that