Additional documentation additions #452

posborne · 2016-10-29T22:35:35Z

This is a second round of doc additions. There are also some funcitonal changes to APIs for getting/setting hostname that need to be reviewed.

r? @nix-rust/nix-maintainers

Signed-off-by: Paul Osborne <[email protected]>

The previous one was entirely too generic. Signed-off-by: Paul Osborne <[email protected]>

fiveop

Thanks for getting the documentation process started. In the long run, I would like to have some formal rules on how documentation should look like, because I think this will result in more consistent and more concise documentation.

fiveop · 2016-11-01T09:10:38Z

src/unistd.rs

+/// for a list of potential problems that maight cause execv to fail.
+///
+/// Both `::nix::unistd::execv` and `::nix::unistd::execve` take as arguments a
+/// slice of `::std::ffi::CString`s for `args` and `env`.  Each element in


execv does not have the env parameter

fiveop · 2016-11-01T09:12:06Z

src/unistd.rs

+/// See `::nix::unistd::execve` for additoinal details.  `execvp` behaves the
+/// sme as execv except that it will examine the `PATH` environment variables
+/// for file names not specified with a leading slash.  For example, `execv`
+/// would not work if I specified "bash" for the path argument, but `execvp`


I would prefer if the documentation where written consistently in the third person.

Sure, that can be accommodated. In general, I am of the opinion that documentation changes moving forward should be fact-checked and merged first and improved in terms of content, quality, and consistency later, particularly given the somewhat poor state of our documentation at present.

fiveop · 2016-11-01T09:17:43Z

src/unistd.rs

+/// [fork(2)](http://man7.org/linux/man-pages/man2/fork.2.html) and
+/// [setsid(2)](http://man7.org/linux/man-pages/man2/setsid.2.html) and, as
+/// such, error that could be returned by either of those functions could also
+/// show up as errors here.


What good is an abstraction, if we need to look at the implementation for documentation?

I am referring users to the libc man pages, not to an implementation. I think the former is acceptable. This is basically the same approach taking by the man pages for this call. We can go further with time, but I think this is a reasonable start.

Fair enough.

fiveop · 2016-11-01T09:18:29Z

src/unistd.rs

+/// 2. Parent process exits
+/// 3. Child process continues to run.
+///
+/// There are a couple options here whose names and meaning can be a bit


I would prefer having less prose here. Just explain what the parameters are for, not why it is necessary to document them.

fiveop · 2016-11-01T09:19:52Z

src/unistd.rs

-    let len = name.len() as size_t;
+/// Get the host name and store it int the provided buffer, returning the actual
+/// length stored in the buffer (see
+/// [gethostname(2)](http://man7.org/linux/man-pages/man2/gethostname.2.html)).


But we don't return the length (directly).

Ah, yes. This needs to be updated. I went through a few iterations and ended up changing how this worked.

fiveop · 2016-11-01T09:22:22Z

src/unistd.rs

+/// println!("Hostname: {}", hostname);
+/// ```
+pub fn gethostname<'a>(buffer: &'a mut [u8]) -> Result<&'a CStr> {
+    let ptr = buffer.as_mut_ptr() as *mut c_char;


This is an aside with respect to this PR:

Shouldn't we change this method to just return the hostname if possible, by using our own buffer?

We could and I considered this, but I wanted to provide a zero-cost abstraction first (the user may already have a buffer). I would consider adding a variant as you mentioned -- not sure what it would be named exactly.

Regarding this function, we cannot guarantee it is a valid &CStr that comes back:

gethostname() returns the null-terminated hostname in the character array name, which has a length of len bytes. If the null-terminated hostname is too large to fit, then the name is truncated, and no error is returned (but see NOTES below). POSIX.1 says that if such truncation occurs, then it is unspecified whether the returned buffer includes a terminating null byte.

―http://man7.org/linux/man-pages/man2/gethostname.2.html#DESCRIPTION

Also I'd prefer API changes in a separate PR from doc changes.

Regarding this function, we cannot guarantee it is a valid &CStr that comes back:

This is why I ensure that the last byte in the buffer is NUL.

Ah I missed that.

posborne · 2016-11-01T16:23:18Z

Thanks for getting the documentation process started. In the long run, I would like to have some formal rules on how documentation should look like, because I think this will result in more consistent and more concise documentation.

Sure, I'm not opposed to aiming for consistency, but I think our primary goal needs to be improving the state of the documentation by at least having the basics for all public functionality. Making it easy for doc changes to get merged and encouraging people to contribute are probably the things that will help us most. We can update for consistency later.

fiveop · 2016-11-01T18:18:36Z

I agree with you that something, so long as it's not wrong, is better than nothing. I was just voicing what I would like to aim for.

r=me, after you've done whatever changes you deem necessary given my comments.

posborne · 2016-11-01T18:21:26Z

I agree with you that something, so long as it's not wrong, is better than nothing. I was just voicing what I would like to aim for.

Sure, I'll try to clean stuff up tonight. Thanks for the review and the understanding!

kamalmarhubi · 2016-11-01T20:19:52Z

CHANGELOG.md

+  written from the underlying system call at all.  This has been updated to
+  return a `&CStr` within the provided buffer that is always properly
+  NUL-terminated (this is not guaranteed by the call with all platforms/libc
+  implementations).


I would like both of these changes moved to a non-doc PR. That PR can include the docs for those functions, but I think it's best to have API changes not happen in a PR that only advertises doc changes.

Sure, I can do that. They are separate commits now.

kamalmarhubi · 2016-11-01T20:20:13Z

src/unistd.rs

+/// println!("Hostname: {}", hostname);
+/// ```
+pub fn gethostname<'a>(buffer: &'a mut [u8]) -> Result<&'a CStr> {
+    let ptr = buffer.as_mut_ptr() as *mut c_char;


Ah I missed that.

These make the language consistent in the 3rd person, fix a few minor mistakes, and remove some superflous prose. Signed-off-by: Paul Osborne <[email protected]>

posborne · 2016-11-02T04:15:41Z

I stripped this down to just the documentation commits and addressed comments, so getting this merged up. @homu r=fiveop

homu · 2016-11-02T04:15:43Z

📌 Commit 173f2ac has been approved by fiveop

…iveop Additional documentation additions This is a second round of doc additions. There are also some funcitonal changes to APIs for getting/setting hostname that need to be reviewed. r? @nix-rust/nix-maintainers

kamalmarhubi · 2016-11-02T04:34:34Z

@homu retry 173f2ac

homu · 2016-11-02T04:34:48Z

⚡ Test exempted - status

…iveop Additional documentation additions This is a second round of doc additions. There are also some funcitonal changes to APIs for getting/setting hostname that need to be reviewed. r? @nix-rust/nix-maintainers

kamalmarhubi · 2016-11-02T04:35:04Z

@homu r=fiveop

@kamalmarhubi

…lmarhubi Gethostname sethostname updates These changes have been previously discussed some with #452 but we decided it made sense to separate things out a bit more. r? @kamalmarhubi @fiveop

posborne added 3 commits October 29, 2016 17:24

unistd: add docs for exec* functions

c270fd2

Signed-off-by: Paul Osborne <[email protected]>

unistd: add documentation for the daemon function

4b6b7b4

Signed-off-by: Paul Osborne <[email protected]>

unistd: add module-level doc comment that makes sense

2f220fc

The previous one was entirely too generic. Signed-off-by: Paul Osborne <[email protected]>

fiveop suggested changes Nov 1, 2016

View reviewed changes

fiveop approved these changes Nov 1, 2016

View reviewed changes

kamalmarhubi requested changes Nov 1, 2016

View reviewed changes

posborne force-pushed the additional-documentation-additions branch from 99a1726 to 2f220fc Compare November 2, 2016 04:08

unistd: doc updates based on review comments

173f2ac

These make the language consistent in the 3rd person, fix a few minor mistakes, and remove some superflous prose. Signed-off-by: Paul Osborne <[email protected]>

kamalmarhubi approved these changes Nov 2, 2016

View reviewed changes

homu merged commit 173f2ac into nix-rust:master Nov 2, 2016

posborne deleted the additional-documentation-additions branch November 2, 2016 04:37

posborne mentioned this pull request Nov 2, 2016

Gethostname sethostname updates #455

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Additional documentation additions #452

Additional documentation additions #452

posborne commented Oct 29, 2016

fiveop left a comment

fiveop Nov 1, 2016

fiveop Nov 1, 2016

posborne Nov 1, 2016

fiveop Nov 1, 2016

posborne Nov 1, 2016

fiveop Nov 1, 2016

fiveop Nov 1, 2016

fiveop Nov 1, 2016

posborne Nov 1, 2016

fiveop Nov 1, 2016

posborne Nov 1, 2016

kamalmarhubi Nov 1, 2016 •

edited

Loading

kamalmarhubi Nov 1, 2016

posborne Nov 1, 2016

kamalmarhubi Nov 1, 2016

posborne commented Nov 1, 2016

fiveop commented Nov 1, 2016

posborne commented Nov 1, 2016

kamalmarhubi Nov 1, 2016

posborne Nov 1, 2016

kamalmarhubi Nov 1, 2016

posborne commented Nov 2, 2016

homu commented Nov 2, 2016

kamalmarhubi commented Nov 2, 2016

homu commented Nov 2, 2016

kamalmarhubi commented Nov 2, 2016

Additional documentation additions #452

Additional documentation additions #452

Conversation

posborne commented Oct 29, 2016

fiveop left a 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

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

Choose a reason for hiding this comment

kamalmarhubi Nov 1, 2016 • edited Loading

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

posborne commented Nov 1, 2016

fiveop commented Nov 1, 2016

posborne commented Nov 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

posborne commented Nov 2, 2016

homu commented Nov 2, 2016

kamalmarhubi commented Nov 2, 2016

homu commented Nov 2, 2016

kamalmarhubi commented Nov 2, 2016

kamalmarhubi Nov 1, 2016 •

edited

Loading