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.

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 wrapper for acct(2) #952

Merged
merged 1 commit into from Oct 24, 2018
Merged

Add wrapper for acct(2) #952

merged 1 commit into from Oct 24, 2018

Conversation

jabedude
Copy link
Contributor

This PR aims to add a nix wrapper for acct(2).

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.

Don't forget to add a CHANGELOG entry.

Also, might I ask what you have in mind for this function? How are you planning to parse the accounting file? Are you writing a separate crate for that, or just planning to use lastcomm(1), or will you be adding that functionality to Nix?

src/unistd.rs Outdated
///
/// See also [acct(2)](https://linux.die.net/man/2/acct)
#[cfg(any(target_os = "linux", target_os = "android"))]
pub fn acct(filename: &CString) -> Result<Void> {
Copy link
Member

Choose a reason for hiding this comment

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

The result type is wrong. Result<Void> indicates that the function will always fail. Perhaps you mean Result<()>?

Copy link
Member

Choose a reason for hiding this comment

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

Also, the argument type is wrong. This API doesn't provide anyway to disable process accounting once it's enabled. I suggest creating an acct module, and giving it two functions enable(filename: &CString), and disable().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wonderful ideas. I will definitely work on those changes.

src/unistd.rs Outdated
/// Switch process accounting on or off
///
/// See also [acct(2)](https://linux.die.net/man/2/acct)
#[cfg(any(target_os = "linux", target_os = "android"))]
Copy link
Member

Choose a reason for hiding this comment

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

acct is available on many more operating systems than this. Please enable it everywhere that it's supported. You may need to submit a separate PR to libc if there are some declarations missing.

src/unistd.rs Outdated
libc::acct(filename.as_ptr())
};

Err(Error::Sys(Errno::last()))
Copy link
Member

Choose a reason for hiding this comment

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

This is the wrong way to set the return code. For one thing, it will always indicate an error. For another, errno won't be set if acct passes. See fchmod in sys/stat.rs for an example of how to do it right.

test/test_unistd.rs Show resolved Hide resolved
src/unistd.rs Outdated
///
/// See also [acct(2)](https://linux.die.net/man/2/acct)
#[cfg(any(target_os = "linux", target_os = "android"))]
pub fn acct(filename: &CString) -> Result<Void> {
Copy link
Member

Choose a reason for hiding this comment

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

Also, the argument type is wrong. In Rust you should rarely use a &CString reference explicitly. Better to use the borrowed form &CStr. And in Nix, you should generally use NixPath instead. See mknod in sys/stat.rs for an example.

@jabedude
Copy link
Contributor Author

I am currently planning on using lastcomm, but I might make a separate crate for parsing pacct files.

@jabedude
Copy link
Contributor Author

I added acct(2) to the BSD tree of libc See here.. CI should pass now

src/unistd.rs Outdated
@@ -1439,6 +1439,32 @@ pub fn sleep(seconds: c_uint) -> c_uint {
unsafe { libc::sleep(seconds) }
}

#[cfg(target_family = "unix")]
Copy link
Member

Choose a reason for hiding this comment

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

The target_family should be unnecessary. Nix only runs on unix operating systems.

test/test_unistd.rs Show resolved Hide resolved
test/test_unistd.rs Outdated Show resolved Hide resolved
@jabedude jabedude force-pushed the acct-api branch 2 times, most recently from 632a600 to e030707 Compare October 15, 2018 21:55
test/test_unistd.rs Show resolved Hide resolved
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.

You'll need to rebase, not just merge, the master branch. Merging the master branch into your PR makes the history screwy.

test/test_unistd.rs Show resolved Hide resolved
CHANGELOG.md Outdated
@@ -25,6 +25,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
([#956](https://github.com/nix-rust/nix/pull/956))
- Added a `fchownat` wrapper.
([#955](https://github.com/nix-rust/nix/pull/955))
- Added a `acct` wrapper module for enabling and disabling process accounting ([#952](https://github.com/nix-rust/nix/pull/952))
Copy link
Member

Choose a reason for hiding this comment

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

Wrap to 80 columns, please.

test/test_unistd.rs Outdated Show resolved Hide resolved
@jabedude jabedude force-pushed the acct-api branch 2 times, most recently from 837d87c to 84811a9 Compare October 21, 2018 00:45
@asomers
Copy link
Member

asomers commented Oct 21, 2018

Hm, I tried using Github's online editor to reword a comment, but it seems that doing so creates a separate commit. Can you please squash those commits for me? I can't do it with the merge button, because Nix uses bors to merge.

@jabedude jabedude force-pushed the acct-api branch 2 times, most recently from c773ea4 to 3d56c27 Compare October 21, 2018 19:37
This patch adds a wrapper for the acct(2) syscall, with two functions
for enabling and disabling process accounting.
@jabedude
Copy link
Contributor Author

Done!

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.

Thanks for your contribution!

bors r+

bors bot added a commit that referenced this pull request Oct 24, 2018
952: Add wrapper for acct(2) r=asomers a=jabedude

This PR aims to add a nix wrapper for acct(2).

Co-authored-by: Josh Abraham <sinisterpatrician@gmail.com>
@bors
Copy link
Contributor

bors bot commented Oct 24, 2018

@bors bors bot merged commit 0ce57d9 into nix-rust:master Oct 24, 2018
@jabedude jabedude deleted the acct-api branch October 24, 2018 22:48
asomers added a commit to asomers/nix that referenced this pull request Nov 21, 2018
asomers added a commit to asomers/nix that referenced this pull request Nov 22, 2018
asomers added a commit to asomers/nix that referenced this pull request Nov 23, 2018
levex pushed a commit to levex/nix that referenced this pull request Dec 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants