Page MenuHomeFreeBSD

Manpage for dnvlist
ClosedPublic

Authored by starak.adam_gmail.com on Jul 19 2016, 10:04 AM.

Details

Reviewers
cem
oshogbo
Group Reviewers
manpages
Summary

Hello!
My name is Adam. This year, I participate in Google Summer of Code. I've just wrote a manpage for dnvlist. I would be thankful if you could take a look at it. Thanks in advance!

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

starak.adam_gmail.com retitled this revision from to Manpage for dnvlist.
starak.adam_gmail.com updated this object.
starak.adam_gmail.com edited the test plan for this revision. (Show Details)
wblock added a subscriber: wblock.Jul 19 2016, 10:56 AM

Please run textproc/igor on this man page, it helps to spot problems.

share/man/man9/dnv.9
36

Please do not use contractions: https://www.freebsd.org/doc/en_US.ISO8859-1/books/fdp-primer/writing-style-guidelines.html

This sentence is based on a couple of "not"s, which makes it a bit confusing. Can we state it differently to make it clearer and avoid the pause in the middle? Something like "API for name/value pairs. Nonexistent pairs do not raise an error."

68

library permits easy management of name/value pairs and can send and receive

69

Please start a new sentence on a new line. Also:

For more information, also see
.Xr nv 9 .
74

s/family/family of/
s/returns/returns the/

Please start new sentences on new lines.

s/the element/an element/

75

s/of/of the/
s/return value/return the value/

76

s/of string/of a string/
s/returned/the returned/

77

What does "should not be modified" mean? Is the user not supposed to modify it, or is this saying that the function will not modify it? "Should" is generally a recommendation to the reader, and that is probably not what was meant here.

78

Sorry, I don't understand this sentence either.

82

s/family/family of/
s/returns value/returns the value/

83

Please start new sentences on new lines.

"given name" is confusing here.
Please do not use contractions.
Please start new sentences on new lines.
Maybe:
`If an element of the supplied name does not exist, the value
provided in defval is returned.`
(defval needs markup there)

84

When the value is a string, binary, or array value, the caller is
responsible for freeing returned memory with
.Fn free 3 .

There might be a better way to show this list of what to do depending on returned values, like a table.

88

When the value is an nvlist, the caller is responsible for destroying the returned nvlist with
.Fn nvlist_destroy .

92

When the value is a descriptor, the caller is responsible for closing the returned descriptor with
.Fn close 2 .

97

These values should be sorted by section, so:
.Xr close 2 ,
.Xr free 3 ,
.Xr nv 9

starak.adam_gmail.com marked 10 inline comments as done.Jul 19 2016, 12:17 PM

wblock, thank you for the fast response. Everything is corrected.

share/man/man9/dnv.9
77

User cannot modify it, because it still belongs to the nvlist (structure which stores those pairs) and any modification may cause an undefined behavior.

78

Nvlist structure has its own error flag. If you want to use one of those functions, the flag must be equal to 0. Otherwise the program will be aborted.

84

That's a great idea, but I don't know how to make a table. :(

I made the changes, which wblock suggested. Thank you for your fast response.

I made a mistake in dnvlist_take.

checked man with igor

A couple of other comments. Please adjust these, then I can download the entire man page source and see about setting up a table. The source for gpart.8 has some examples of these.

share/man/man9/dnv.9
78

As an instruction, it should probably be more emphatic (ignoring wrapping):

Returned strings, nvlists, descriptors, binaries or arrays must not be modified by the user.
They still belong to the nvlist.
79

Still not clear on this. Maybe "The nvlist error flag must be 0, or attempts to use any of these functions will cause the program to abort."?

83

s/remove/removes/

I still think "given name" is confusing. "specified" is a little better than "given".

starak.adam_gmail.com marked 2 inline comments as done.Jul 25 2016, 10:12 AM

Thanks wblock for corrections!

share/man/man9/dnv.9
79

I'll quote 2 functions from nv(9), which use this flag. Maybe it'll clean up the situation.

The nvlist_error() function returns any error value that the nvlist
accumulated.  If the given nvlist is NULL the ENOMEM error will be
returned.

The nvlist_set_error() function sets an nvlist to be in the error state.
Subsequent calls to nvlist_error() will return the given error value.
This function cannot be used to clear the error state from an nvlist.
This function does nothing if the nvlist is already in the error state.

So, if you want to use get or take functions, nvlist_error() should return 0. Otherwise the program will be aborted, because the nvlist in is error state.

"The nvlist must not be in error state. Otherwise, attempts to use any of these functions will cause the program to abort."

Update after wblock's corrections.

wblock added inline comments.Jul 25 2016, 12:51 PM
share/man/man9/dnv.9
79

Ah, that helps. How about:

If the nvlist is in an error state, attempts to use any of these functions will cause the program to abort.
starak.adam_gmail.com marked 2 inline comments as done.Jul 26 2016, 7:28 AM

done

wblock added inline comments.Aug 5 2016, 3:09 PM
share/man/man9/dnv.9
75

s/will return/returns/

77

Needs a serial comma after "binaries":
s/binaries/binaries,/

93

Just

.Fn free 3 .

and delete the "function." line.

97

As above, just

.Fn nvlist_destroy .

and delete the "function." line.

100

As above, just

.Fn close 2 .

and delete the "system call." line.

starak.adam_gmail.com marked 5 inline comments as done.Aug 8 2016, 7:22 AM
oshogbo added inline comments.Aug 8 2016, 7:33 AM
share/man/man9/dnv.9
35

Maybe we should go with something like
library for name/value pair with default values.

Or.
API for getting name/value pairs library, for nonexistent pairs the default value is returned?

cem accepted this revision.Aug 8 2016, 4:36 PM
cem added a reviewer: cem.
cem added a subscriber: cem.

LGTM, modulo nits below.

It might also be nice to add a HISTORY section to say when the API appeared.

share/man/man9/dnv.9
75

.Fa defval .

108

This manual page was written by
.An Adam Starak ...

This revision is now accepted and ready to land.Aug 8 2016, 4:36 PM
starak.adam_gmail.com marked an inline comment as done.Aug 9 2016, 11:25 AM

oops, i put the commits wrong way. Updated

starak.adam_gmail.com edited edge metadata.

Update. Sorry for last diff. I reversed commits.

This revision now requires review to proceed.Aug 9 2016, 11:54 AM
brueffer added inline comments.
share/man/man9/Makefile
740

The last "\" is one too many.

share/man/man9/dnv.9
33

If you add ".Nm dnv" above the line here, all ".Nm dnv" lines later on can be shortened to just ".Nm".

103

Wrong commas after sorting, this one should move one line up.

starak.adam_gmail.com marked 2 inline comments as done.Aug 18 2016, 2:26 PM

updated

share/man/man9/dnv.9
33

Sorry, but I don't get the idea of using this ".Nm dnv". Can you explain it a little bit more?

starak.adam_gmail.com edited edge metadata.
brueffer added inline comments.Aug 18 2016, 2:34 PM
share/man/man9/dnv.9
33

Sure! The first .Nm call (currently ".Nm dnvlist_get") sets the default value, so any later call of simply ".Nm" will produce dnvlist_get. Most API manpages define the name of the API ("dnv" here) as default value, so they only have to call .Nm when they refer to the API.

oshogbo accepted this revision.Oct 5 2016, 7:01 PM
oshogbo edited edge metadata.
This revision is now accepted and ready to land.Oct 5 2016, 7:01 PM
oshogbo closed this revision.Oct 5 2016, 7:01 PM