lib/libcasper/services/cap_net/cap_net.3
165 ↗	(On Diff #71378)	s/to set/to a set/
170 ↗	(On Diff #71378)	s/to do bind/to bind/
175 ↗	(On Diff #71378)	s/to do connect/to connect/
216 ↗	(On Diff #71378)	s/previews/preview/
221 ↗	(On Diff #71378)	s/an host/a host/

val_packett.cool added a subscriber: val_packett.cool.May 5 2020, 12:13 PM

val_packett.cool added inline comments.

lib/libcasper/services/cap_net/cap_net.3
122 ↗	(On Diff #71378)	s/revers/reverse/ (here and below) also these shouldn't begin with "the"
149 ↗	(On Diff #71378)	It's not clear what "those specific structures" means. Are we only allowing specific addresses to be looked up? Why does the signature take a pointer to a single sockaddr? Are we supposed to call this multiple times, one for each allowed address? (same applies to other similar functions)
lib/libcasper/services/cap_net/cap_net.h
83 ↗	(On Diff #71378)	s/conenct/connect/

gbe added a subscriber: gbe.May 10 2020, 12:10 PM

Man pages fixes.

Harbormaster completed remote builds in B31032: Diff 71650.May 11 2020, 8:27 PM

Thank you @bcr and @greg_unrelenting.technology

lib/libcasper/services/cap_net/cap_net.3

149 ↗

(On Diff #71378)

Yes there is not about that

Multiple calls to
.Fn cap_net_limit_addr2name_family,
.Fn cap_net_limit_addr2name,
.Fn cap_net_limit_name2addr_family,
.Fn cap_net_limit_name2addr,
.Fn cap_net_limit_conenct,
and
.Fn cap_net_limit_bind
is supported, each call is extending previews capabilities.

Looks good on the man page side of things, after removing the one extra "the" I found. Thanks for writing the man page!

lib/libcasper/services/cap_dns/cap_dns.3
61 ↗	(On Diff #71650)	You can remove the "the" here.

This revision is now accepted and ready to land.May 13 2020, 7:13 AM

freqlabs mentioned this in D24832: libcasper: Create a minimal cap_netdb service.May 14 2020, 3:56 PM

freqlabs added a subscriber: freqlabs.May 28 2020, 9:17 PM

I hadn't noticed this when I created D24832. Could you add that to cap_net instead?

emaste added inline comments.May 29 2020, 2:27 AM

lib/libcasper/services/cap_dns/cap_dns.3
61 ↗	(On Diff #71650)	Should we make it "is obsolete and will be removed in .Fx 14"?
lib/libcasper/services/cap_net/cap_net.3
246 ↗	(On Diff #71650)	igor doesn't like the contraction

emaste added inline comments.May 29 2020, 2:56 AM

lib/libcasper/services/cap_net/cap_net.3
93 ↗	(On Diff #71650)	s/cep/cap/

emaste added inline comments.May 29 2020, 3:05 AM

lib/libcasper/services/cap_net/cap_net.3
110–111 ↗	(On Diff #71650)	"small number of the network namespace" doesn't quite make sense - you might say "small portion of the network namespace" or similar.
152 ↗	(On Diff #71650)	limits the and similarly elsewhere However I'd be also happy enough to just do a pass over the man page once it's in the tree for these
207 ↗	(On Diff #71650)	need a space before the . it currently renders as `may be freed using cap_net_free.()` similarly below
218 ↗	(On Diff #71650)	Should we have a cap_enter() in the example to clearly distinguish what runs in the sandbox vs what sets up the service?

I don't quite understand why cap_dns is merged into this service. Aren't they mostly orthogonal?

lib/libcasper/services/cap_net/cap_net.3
249 ↗	(On Diff #71650)	Missing period.
lib/libcasper/services/cap_net/cap_net.h
116 ↗	(On Diff #71650)	Style: opening brace should be on its own line, there should be newlines between functions.
144 ↗	(On Diff #71650)	Why are they obsolete?

Oh sorry @emaste I some how didn't get the emails from your comments. I will address them ASAP.

markj added inline comments.Jun 28 2020, 7:04 PM

lib/libcasper/services/cap_net/cap_net.3
2 ↗	(On Diff #71650)	We should avoid adding "All rights reserved" to new copyrights.
149 ↗	(On Diff #71650)	Typo, "reverse"
lib/libcasper/services/cap_net/cap_net.c
318 ↗	(On Diff #71650)	If `addrinfo_unpack()` returns NULL, we don't return EAI_MEMORY here.
321 ↗	(On Diff #71650)	If `prevai == NULL` then it must be true that `firstai == NULL`, so the condition is redundant.
858 ↗	(On Diff #71650)	Mixing GAI error values with errno values.
1012 ↗	(On Diff #71650)	Why do we dup the socket?
1045 ↗	(On Diff #71650)	IMO it would be a bit cleaner to handle `salimits == NULL` in `net_allowed_bsaddr_impl()`.
1295 ↗	(On Diff #71650)	Shouldn't it be an errno value? Ditto above.
1339 ↗	(On Diff #71650)	I think we should start defining constants for the command strings. Otherwise the compiler can't catch typos.
1350 ↗	(On Diff #71650)	This file is inconsistent in the way that it braces single-line statements. net_command(), below, does not have any.
1376 ↗	(On Diff #71650)	Remove what?
lib/libcasper/services/cap_net/cap_net.h
39 ↗	(On Diff #71650)	Extra newline.
57 ↗	(On Diff #71650)	Maybe add `/* Capability functions. /` to complement `/ Limit functions. */` below.
106 ↗	(On Diff #71650)	Hmm, I guess this ensures that we don't trigger error handling in the caller? We could provide a static non-NULL pointer instead, like (void *)(uintptr_t)0x1. cap_sysctl has the same problem.
144 ↗	(On Diff #71650)	Mariusz replied on IRC, this just reflects the fact that getaddrinfo() and getnameinfo() are preferred over gethost*() per the man page for these functions.

emaste added inline comments.Jul 3 2020, 3:14 PM

lib/libcasper/services/cap_net/cap_net.3
249 ↗	(On Diff #71650)	Difficult to figure out what to do with a period after a domain name, in plain text :) Maybe a space after .com before the period?
lib/libcasper/services/cap_net/cap_net.h
144 ↗	(On Diff #71650)	Probably "deprecated" is better

Update after emaste, markj and bcr review.

This revision now requires review to proceed.Jul 5 2020, 10:22 AM

Harbormaster completed remote builds in B32144: Diff 74092.Jul 5 2020, 10:22 AM

oshogbo added inline comments.Jul 5 2020, 10:22 AM

lib/libcasper/services/cap_dns/cap_dns.3
61 ↗	(On Diff #71650)	Thats a good question I'm not sure. From one hand yes we would like to on the other hand there may be some consumers of it.
lib/libcasper/services/cap_net/cap_net.3
152 ↗	(On Diff #71650)	Thank you!!!
218 ↗	(On Diff #71650)	There is caph_enter_casper which is cap_enter which check if the casper is buildin.
246 ↗	(On Diff #71650)	Fixded.
249 ↗	(On Diff #71650)	I added space.
lib/libcasper/services/cap_net/cap_net.c
318 ↗	(On Diff #71650)	Nice catch.
321 ↗	(On Diff #71650)	Right.
1012 ↗	(On Diff #71650)	We don't have to we can just take it from nvlist.
1045 ↗	(On Diff #71650)	We can't do that. Here if the capdnscache is NULL we return ENOTCAPABLE, but in case of the net_allowed_bsaddr which is used much offten if the right are not defined it means that we allow all bsaddr.
1295 ↗	(On Diff #71650)	Not really sure what to do with that ;(
1339 ↗	(On Diff #71650)	Thats a good idea. I started with the limits name for know.
1350 ↗	(On Diff #71650)	Style fixed.
1376 ↗	(On Diff #71650)	The flags after the development. The CASPER_SERVICE_FD nor the CASPER_SERVICE_STDIO in not needed.
lib/libcasper/services/cap_net/cap_net.h
106 ↗	(On Diff #71650)	Yes. As well as we ensure working with the proper descriptor. For example in fileargs we are doing much more checks.

emaste added inline comments.Jul 26 2020, 5:33 PM

lib/libcasper/services/cap_dns/cap_dns.3
61 ↗	(On Diff #71650)	Perhaps say that it may be removed after FreeBSD 13, then? I think it would be good to give some encouragement for people to migrate. We could handle this in later followup though (after this gets committed.)
lib/libcasper/services/cap_net/cap_net.h
160 ↗	(On Diff #74092)	typo, "Deprecated"
39 ↗	(On Diff #71650)	Still an extra newline here?