Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Wrap libc::statfs #928

Merged
merged 1 commit into from
Jun 20, 2019
Merged

Wrap libc::statfs #928

merged 1 commit into from
Jun 20, 2019

Conversation

alesharik
Copy link
Contributor

@alesharik alesharik commented Jul 11, 2018 •
edited
Loading

Fix for #926

@alesharik alesharik changed the title Wrap libc::statfs and fix #926 Wrap libc::statfs Jul 11, 2018
asomers
asomers requested changes Jul 11, 2018
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

I think you should

  • Eliminate the _new variants and make the regular functions return by result instead of by return arguments.
  • Don't assume anything about /proc.
  • Don't hardcode magic numbers
  • Lift the #[cfg()] restriction in src/sys/mod.rs so statfs will build on every platform. This will require a little bit of research on your part to get the different fields right on different platforms.

src/sys/statfs.rs Outdated Show resolved Hide resolved
src/sys/statfs.rs Show resolved Hide resolved
src/sys/statfs.rs Show resolved Hide resolved
src/sys/statfs.rs Outdated Show resolved Hide resolved
src/sys/statfs.rs Outdated Show resolved Hide resolved
src/sys/statfs.rs Outdated Show resolved Hide resolved
src/sys/statfs.rs Outdated Show resolved Hide resolved
src/sys/statfs.rs Outdated Show resolved Hide resolved
src/sys/statfs.rs Outdated Show resolved Hide resolved
@asomers
Copy link
Member

asomers commented Jul 11, 2018

Yeah, /tmp should be safe. But you don't know what filesystem it will be using.

asomers
asomers requested changes Jul 11, 2018
View reviewed changes
src/sys/statfs.rs Show resolved Hide resolved
src/sys/statfs.rs Outdated Show resolved Hide resolved
src/sys/statfs.rs Outdated Show resolved Hide resolved
src/sys/statfs.rs Outdated Show resolved Hide resolved
src/sys/statfs.rs Outdated Show resolved Hide resolved
asomers
asomers requested changes Jul 12, 2018
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

The FsType looks good. Now you just need to make it work on all platforms.

src/sys/statfs.rs Show resolved Hide resolved
src/sys/mod.rs Outdated Show resolved Hide resolved
asomers
asomers reviewed Jul 26, 2018
View reviewed changes
src/sys/mod.rs Outdated Show resolved Hide resolved
src/sys/statfs.rs Outdated Show resolved Hide resolved
src/sys/statfs.rs Outdated Show resolved Hide resolved
asomers
asomers requested changes Aug 5, 2018
View reviewed changes
src/sys/statfs.rs Outdated Show resolved Hide resolved
src/sys/statfs.rs Outdated Show resolved Hide resolved
src/sys/statfs.rs Outdated Show resolved Hide resolved
asomers
asomers requested changes Aug 8, 2018
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

In the future, please don't squash your commits until the end. It makes reviewing incremental changes harder.

src/sys/statfs.rs Outdated Show resolved Hide resolved
src/sys/statfs.rs Outdated Show resolved Hide resolved
src/sys/statfs.rs Outdated Show resolved Hide resolved
asomers
asomers reviewed Nov 9, 2018
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/sys/mod.rs Outdated Show resolved Hide resolved
src/sys/statfs.rs Show resolved Hide resolved
src/sys/statfs.rs Outdated Show resolved Hide resolved
src/sys/statfs.rs Outdated Show resolved Hide resolved
@asomers
Copy link
Member

asomers commented May 2, 2019

Since it's been so long since the original submission, I'm going to ask that you rebase onto master before I review it. Be sure to rebase, don't just merge. Merging makes the changes harder to review. And I probably won't have time to review before Sunday night. If I still haven't reviewed by Monday, feel free to ping me.

@alesharik
Copy link
Contributor Author

Hi, @asomers . Any updates on reviewing my changes?

asomers
asomers reviewed May 7, 2019
View reviewed changes
src/sys/statfs.rs Outdated Show resolved Hide resolved
src/sys/statfs.rs Outdated Show resolved Hide resolved
src/sys/statfs.rs Outdated Show resolved Hide resolved
src/sys/statfs.rs Show resolved Hide resolved
src/sys/statfs.rs Show resolved Hide resolved
src/sys/statfs.rs Outdated Show resolved Hide resolved
@alesharik
Copy link
Contributor Author

alesharik commented May 26, 2019 •
edited
Loading

Hi, @asomers . File src/sys/signal.rs is broken an android therefore Travis fails the build. Can you do something about it? Work In Progress Fixed

@asomers
Copy link
Member

asomers commented May 28, 2019

It looks like this is still a WIP; let me know when you're ready for a review. Also, you'll need to rebase. Be sure to fix the CHANGELOG entry when you rebase.

@alesharik
Copy link
Contributor Author

@asomers I'm ready for review

asomers
asomers reviewed Jun 7, 2019
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/sys/statfs.rs Show resolved Hide resolved
@alesharik
Copy link
Contributor Author

@asomers Ready for review

asomers
asomers approved these changes Jun 7, 2019
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

This looks good now. But I want to merge #1035 first.

@alesharik
Copy link
Contributor Author

Ok, no problem. I'll fix conflicts when #1035 is merged.

@asomers
Copy link
Member

asomers commented Jun 12, 2019

Ok, 1035 is merged. You should be able to rebase now. You should no longer need impl Debug for StatFs.

@asomers
Copy link
Member

asomers commented Jun 12, 2019

Please squash your commits.

@alesharik
Copy link
Contributor Author

@asomers any updates on merging this PR?

asomers
asomers approved these changes Jun 20, 2019
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot added a commit that referenced this pull request Jun 20, 2019
@bors @alesharik
Merge #928
7fa4f23 
928: Wrap libc::statfs r=asomers a=alesharik

Fix for [#926](#926)

Co-authored-by: alesharik <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jun 20, 2019

Build succeeded

@bors bors bot merged commit e6d810d into nix-rust:master Jun 20, 2019
@asomers asomers mentioned this pull request Sep 17, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asomers asomers asomers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alesharik @asomers