reuse definitions from libc #561

Mic92 · 2017-03-25T13:22:00Z

No description provided.

Mic92 · 2017-03-25T17:03:15Z

src/sys/statfs.rs

-}
-
-pub fn statfs<P: ?Sized + NixPath>(path: &P, stat: &mut vfs::Statfs) -> Result<()> {
+pub fn statfs<P: ?Sized + NixPath>(path: &P, stat: &mut libc::statfs) -> Result<()> {


This also requires a changelog entry then.

Yes, please add it to this PR.

Mic92 · 2017-03-25T19:47:19Z

src/sys/statfs.rs

-        pub f_frsize: FragmentSize,
-        pub f_spare: [SwordType; 5],
-    }
-
    pub const ADFS_SUPER_MAGIC : FsType =      0xadf5;


These definitions are also available in libc. Should I remove them?

Yes, the goal is to get all FFI definitions into libc.

Susurrus · 2017-03-26T14:02:42Z

This looks pretty good to me. I think we need to resolve the re-exporting of constants and then I think this is good to merge.

homu · 2017-04-15T21:12:32Z

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

Susurrus · 2017-03-26T13:59:44Z

CHANGELOG.md

@@ -12,6 +12,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 ### Changed
 - Marked `sys::mman::{ mmap, munmap, madvise, munlock, msync }` as unsafe.
  ([#559](https://github.com/nix-rust/nix/pull/559))
+- `nix::sys::statfs::{statfs,fstatfs}` uses statfs definition from `libc::statfs` instead of own type `nix::sys::Statfs`.
+  Also file system type constants like `nix::sys::statfs::ADFS_SUPER_MAGIC` were removed in favor of the libc aquivalent.


I'm wondering if it isn't nicer to re-export all relevant libc constants. It's a little weird in consuming code to have to import constants from libc to use with nix effectively. @posborne @fiveop Is there a consensus on this?

I'm fine ignoring these constants for now. Eventually I want to implement a wrapper type like this as I've done in #527 for libc::termios.

Could you fix the spelling of "equivalent" here?

The API advertised here was incorrect anyway on all platforms but linux. All the magic constants are linux specific. BSD* and OS X uses string constants: https://www.freebsd.org/cgi/man.cgi?query=statfs&sektion=2

Susurrus · 2017-06-03T19:08:08Z

I'd like if instead of making people use libc types with nix, they'd use nix types directly. So I suggest we keep a StatFs type that is a re-export of libc::sys::statfs and use that in places. I don't want our users to have to use libc for anything and instead rely on nix types explicitly. Can you change this for the functions you modified?

Mic92 · 2017-06-03T20:52:13Z

There is not feedback from the maintainers of this project. So I will not put effort into this.

Susurrus · 2017-06-20T02:05:08Z

@Mic92 Are you saying you aren't interested in getting this merged?

Mic92 · 2017-06-20T08:50:16Z

It has not highest priority for me at the moment. I could also drop the last commit and only get the others merged.

Susurrus · 2017-06-20T15:32:42Z

That'd be great if you could rebase this and keep only the first 2 commits (be sure to add a CHANGELOG entry for the first commit).

Susurrus · 2017-07-06T22:28:38Z

src/sys/stat.rs

    Mode::from_bits(prev).expect("[BUG] umask returned invalid Mode")
 }

 pub fn stat<P: ?Sized + NixPath>(path: &P) -> Result<FileStat> {
    let mut dst = unsafe { mem::uninitialized() };
    let res = try!(path.with_nix_path(|cstr| {
        unsafe {
-            ffi::stat(cstr.as_ptr(), &mut dst as *mut FileStat)
+            libc::stat(cstr.as_ptr(), &mut dst as *mut FileStat)


Please either pull the changes to use libc::stat/fstat into a separate commit or change the name of this commit as this doesn't just modify mknod/umask.

changed the title.

Susurrus · 2017-07-06T22:29:04Z

This needs that spelling fix and cleaning up that one commit, then this should be good to merge.

the previous definition were linux specific.

Mic92 · 2017-07-06T23:49:30Z

here we go.

Susurrus · 2017-07-07T03:53:47Z

Great, another little bit of cleanup. Thanks for fixing those commits too, I like a nice commit history!

bors r+

561: reuse definitions from libc r=Susurrus

bors · 2017-07-07T08:07:06Z

Timed out

Mic92 · 2017-07-07T08:10:22Z

@bors naughty bot.

asomers · 2017-07-07T15:14:24Z

Umlaut:1, Buildbot: 0

Buildbot 0.9.5 has a known problem when a commit's author contains non-ascii characters. That's the cause of the Bors timeout. I'm going to bypass Bors for this one.

Susurrus · 2017-07-07T15:24:33Z

@asomers I'm amazed at how many bugs we've found in buildbot just using it for a brief amount of time. I would've thought most of this would have been ironed out. Anyways, is there a bug filed upstream for that so we could see a fix at somepoint?

Mic92 · 2017-07-07T15:29:01Z

And I thought jenkins is buggy. Umlauts is just the basic case given how many emojis are used in rust projects.

asomers · 2017-07-07T15:33:32Z

@Susurrus here's the upstream bug. It's already fixed, but I haven't upgraded our install yet. buildbot/buildbot#3078 . I think this is the only real bug we've found so far. Everything else has been a configuration error on my part. It's still frustrating that it's so hard to come up with a good configuration, and there isn't a template for "Travis Replacement", but we can't blame BuildBot's code for those other problems.

Susurrus · 2017-07-07T15:56:47Z

@asomers Yeah, maybe that's something you can contribute to their docs in your copious spare time! ;-)

But I thought there was also the bug about PRs from branches named master?

asomers · 2017-07-07T16:04:25Z

@Susurrus I'm pretty sure that the bug about PRs from branches named master is also a configuration problem. See Issue #652 . I made a config change last night, and it looks like it's working now.

Mic92 force-pushed the cleanup branch 3 times, most recently from 04b8a34 to 45653cc Compare March 25, 2017 17:00

Mic92 commented Mar 25, 2017

View reviewed changes

Mic92 force-pushed the cleanup branch from 45653cc to b4c22df Compare March 26, 2017 11:02

Susurrus reviewed Apr 17, 2017

View reviewed changes

Susurrus suggested changes Jul 6, 2017

View reviewed changes

Mic92 force-pushed the cleanup branch 2 times, most recently from 5a7cd6a to bf82b22 Compare July 6, 2017 22:59

Mic92 added 3 commits July 7, 2017 00:00

reuse mknod/umask/fstat/lstat from libc

aead757

README.md: update gethostname api

c8267bc

sys/statfs: use statfs from libc

ad599d2

the previous definition were linux specific.

Mic92 force-pushed the cleanup branch from bf82b22 to ad599d2 Compare July 6, 2017 23:02

Susurrus approved these changes Jul 7, 2017

View reviewed changes

bors bot added a commit that referenced this pull request Jul 7, 2017

Merge #561

2fb0132

561: reuse definitions from libc r=Susurrus

asomers merged commit 45c1821 into nix-rust:master Jul 7, 2017

Mic92 deleted the cleanup branch July 7, 2017 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reuse definitions from libc #561

reuse definitions from libc #561

Mic92 commented Mar 25, 2017

Mic92 Mar 25, 2017

Susurrus Mar 26, 2017

Mic92 Mar 26, 2017

Mic92 Mar 25, 2017

Susurrus Mar 26, 2017

Mic92 Mar 26, 2017

Susurrus commented Mar 26, 2017

homu commented Apr 15, 2017

Susurrus Mar 26, 2017

Susurrus Jul 6, 2017

Mic92 Jul 6, 2017 •

edited

Loading

Susurrus commented Jun 3, 2017

Mic92 commented Jun 3, 2017

Susurrus commented Jun 20, 2017

Mic92 commented Jun 20, 2017

Susurrus commented Jun 20, 2017

Susurrus Jul 6, 2017

Mic92 Jul 6, 2017

Susurrus commented Jul 6, 2017

Mic92 commented Jul 6, 2017

Susurrus commented Jul 7, 2017

bors bot commented Jul 7, 2017

Mic92 commented Jul 7, 2017 •

edited

Loading

asomers commented Jul 7, 2017

Susurrus commented Jul 7, 2017

Mic92 commented Jul 7, 2017 •

edited

Loading

asomers commented Jul 7, 2017

Susurrus commented Jul 7, 2017

asomers commented Jul 7, 2017

reuse definitions from libc #561

reuse definitions from libc #561

Conversation

Mic92 commented Mar 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Mar 26, 2017

homu commented Apr 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Susurrus commented Jun 3, 2017

Mic92 commented Jun 3, 2017

Susurrus commented Jun 20, 2017

Mic92 commented Jun 20, 2017

Susurrus commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Jul 6, 2017

Mic92 commented Jul 6, 2017

Susurrus commented Jul 7, 2017

bors bot commented Jul 7, 2017

Timed out

Mic92 commented Jul 7, 2017 • edited Loading

asomers commented Jul 7, 2017

Susurrus commented Jul 7, 2017

Mic92 commented Jul 7, 2017 • edited Loading

asomers commented Jul 7, 2017

Susurrus commented Jul 7, 2017

asomers commented Jul 7, 2017

Mic92 Jul 6, 2017 •

edited

Loading

Mic92 commented Jul 7, 2017 •

edited

Loading

Mic92 commented Jul 7, 2017 •

edited

Loading