Details
- Reviewers
emaste markj bcr - Group Reviewers
manpages - Commits
- rS364276: libcasper: Introduce cap_net a network service for Casper.
Diff Detail
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 30898 Build 28612: arc lint + arc unit
Event Timeline
| lib/libcasper/services/cap_net/cap_net.3 | ||
|---|---|---|
| 122 | s/revers/reverse/ (here and below) also these shouldn't begin with "the" | |
| 149 | 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 | s/conenct/connect/ | |
Thank you @bcr and @greg_unrelenting.technology
| lib/libcasper/services/cap_net/cap_net.3 | ||
|---|---|---|
| 149 | 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 | You can remove the "the" here. | |
| lib/libcasper/services/cap_net/cap_net.3 | ||
|---|---|---|
| 94 | s/cep/cap/ | |
| lib/libcasper/services/cap_net/cap_net.3 | ||
|---|---|---|
| 111–112 | "small number of the network namespace" doesn't quite make sense - you might say "small portion of the network namespace" or similar. | |
| 153 | limit*s* the However I'd be also happy enough to just do a pass over the man page once it's in the tree for these | |
| 208 | need a space before the . | |
| 219 | 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 | ||
|---|---|---|
| 250 | Missing period. | |
| lib/libcasper/services/cap_net/cap_net.h | ||
| 117 | Style: opening brace should be on its own line, there should be newlines between functions. | |
| 145 | Why are they obsolete? | |
Oh sorry @emaste I some how didn't get the emails from your comments. I will address them ASAP.
| lib/libcasper/services/cap_net/cap_net.3 | ||
|---|---|---|
| 3 | We should avoid adding "All rights reserved" to new copyrights. | |
| 150 | Typo, "reverse" | |
| lib/libcasper/services/cap_net/cap_net.c | ||
| 319 | If addrinfo_unpack() returns NULL, we don't return EAI_MEMORY here. | |
| 322 | If prevai == NULL then it must be true that firstai == NULL, so the condition is redundant. | |
| 859 | Mixing GAI error values with errno values. | |
| 1013 | Why do we dup the socket? | |
| 1046 | IMO it would be a bit cleaner to handle salimits == NULL in net_allowed_bsaddr_impl(). | |
| 1296 | Shouldn't it be an errno value? Ditto above. | |
| 1340 | I think we should start defining constants for the command strings. Otherwise the compiler can't catch typos. | |
| 1351 | This file is inconsistent in the way that it braces single-line statements. net_command(), below, does not have any. | |
| 1377 | Remove what? | |
| lib/libcasper/services/cap_net/cap_net.h | ||
| 40 | Extra newline. | |
| 58 | Maybe add /* Capability functions. */ to complement /* Limit functions. */ below. | |
| 107 | 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. | |
| 145 | Mariusz replied on IRC, this just reflects the fact that getaddrinfo() and getnameinfo() are preferred over gethost*() per the man page for these functions. | |
| lib/libcasper/services/cap_dns/cap_dns.3 | ||
|---|---|---|
| 61 | Thats a good question I'm not sure. | |
| lib/libcasper/services/cap_net/cap_net.3 | ||
| 153 | Thank you!!! | |
| 219 | There is caph_enter_casper which is cap_enter which check if the casper is buildin. | |
| 247 | Fixded. | |
| 250 | I added space. | |
| lib/libcasper/services/cap_net/cap_net.c | ||
| 319 | Nice catch. | |
| 322 | Right. | |
| 1013 | We don't have to we can just take it from nvlist. | |
| 1046 | 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. | |
| 1296 | Not really sure what to do with that ;( | |
| 1340 | Thats a good idea. I started with the limits name for know. | |
| 1351 | Style fixed. | |
| 1377 | 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 | ||
| 107 | Yes. As well as we ensure working with the proper descriptor. For example in fileargs we are doing much more checks. | |
| lib/libcasper/services/cap_dns/cap_dns.3 | ||
|---|---|---|
| 61 | 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 | ||
| 40 | Still an extra newline here? | |
| 161 | typo, "Deprecated" | |