Details
- Reviewers
emaste markj bcr - Group Reviewers
manpages - Commits
- rS364276: libcasper: Introduce cap_net a network service for Casper.
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
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/ |
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. |
lib/libcasper/services/cap_net/cap_net.3 | ||
---|---|---|
93 ↗ | (On Diff #71650) | s/cep/cap/ |
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) | 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 |
207 ↗ | (On Diff #71650) | need a space before the . |
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.
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. |
lib/libcasper/services/cap_dns/cap_dns.3 | ||
---|---|---|
61 ↗ | (On Diff #71650) | Thats a good question I'm not sure. |
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. |
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? |
If we're happy with the API (aside from a couple of typos in #defines) what do you think about committing this, and iterating further in the tree?
lib/libcasper/services/cap_net/cap_net.3 | ||
---|---|---|
125–127 ↗ | (On Diff #74092) | deprecated |
lib/libcasper/services/cap_net/cap_net.h | ||
51–52 ↗ | (On Diff #74092) | deprecated |
91 ↗ | (On Diff #74092) | Deprecated |