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)

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

share/man/man9/dnv.9
35

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."

67

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

68

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

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

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

Please start new sentences on new lines.

s/the element/an element/

74

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

75

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

76

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.

77

Sorry, I don't understand this sentence either.

81

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

82

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)

83

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.

87

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

91

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

96

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

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

share/man/man9/dnv.9
76

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

77

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.

83

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.

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
77

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.
78

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."?

82

s/remove/removes/

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

Thanks wblock for corrections!

share/man/man9/dnv.9
78

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."

share/man/man9/dnv.9
78

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.
share/man/man9/dnv.9
74

s/will return/returns/

76

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

92

Just

.Fn free 3 .

and delete the "function." line.

96

As above, just

.Fn nvlist_destroy .

and delete the "function." line.

99

As above, just

.Fn close 2 .

and delete the "system call." line.

share/man/man9/dnv.9
34

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 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

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
739

The last "\" is one too many.

share/man/man9/dnv.9
32

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

102

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

updated

share/man/man9/dnv.9
32

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

share/man/man9/dnv.9
32

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 edited edge metadata.
This revision is now accepted and ready to land.Oct 5 2016, 7:01 PM