jail: convert several functions from int to bool

[freebsd_igalic.co]

Summary

these functions exclusively return (0) and (1), so convert them to bool

We also convert some networking related jail functions from int to bool
some of which were returning an error that was never used.

[kp]

While I think this is generally an improvement I don't know if it's significant enough to justify the churn.
The jail maintainer (@jamie) may have opinions as well.

sys/kern/kern_jail.c

3667

That's a bit redundant.

[jamie]

Yeah, I'd been meaning to get around to that ;-).

While you're at it, you could add prison_flag() and prison_allow(). prison_flag() currently returns either 0 or the flag passed in (converted from unsigned to int), but is only ever used by callers as a boolean and should become one for clarity. I was probably being lazy/micro-optimizing when I wrote it.

And then there's netinet/in_jail.c and netinet6/in6_jail.c, which have prison_restrict_ip[46]() and prison_equal_ip[46](), . They also have prison_saddrsel_ip[46]() which are funny bool/errno hybrids that can return 0, 1, or EAFNOSUPPORT, but whose callers only ever use the result as a bool (i.e. EAFNOSUPPORT is equivalent to true); that may or may not be better served as an actual boolean.

[freebsd_igalic.co]

sys/kern/kern_jail.c

3667

my instinct was to transform it into a regular if
but i wanted to keep the diff small
i did my think further, heh

[freebsd_igalic.co]

In D29659#665036, @jamie wrote:

Yeah, I'd been meaning to get around to that ;-).

I've been meaning to submit this patch since i first read thru this file

While you're at it, you could add prison_flag() and prison_allow(). prison_flag() currently returns either 0 or the flag passed in (converted from unsigned to int), but is only ever used by callers as a boolean and should become one for clarity. I was probably being lazy/micro-optimizing when I wrote it.

aye, will convert those too

And then there's netinet/in_jail.c and netinet6/in6_jail.c, which have prison_restrict_ip[46]() and prison_equal_ip[46](), . They also have prison_saddrsel_ip[46]() which are funny bool/errno hybrids that can return 0, 1, or EAFNOSUPPORT, but whose callers only ever use the result as a bool (i.e. EAFNOSUPPORT is equivalent to true); that may or may not be better served as an actual boolean.

there are other candidates that return (0) or a single error that is always the same, but here we would need modifying the call-site :

• prison_check_af() 0/EAFNOSUPPORT
• prison_canseemount() 0/ENOENT
• prison_check() 0/ESRCH

[jamie]

In D29659#665126, @me_igalic.co wrote:

there are other candidates that return (0) or a single error that is always the same, but here we would need modifying the call-site :

• prison_check_af() 0/EAFNOSUPPORT
• prison_canseemount() 0/ENOENT
• prison_check() 0/ESRCH

I think anything like should remain as is. While they may only return one possible error, they usually called with the typical errno usage:

error = prison_foo(...);
if (error != 0)
    return (error);

I wouldn't want the callers to have to worry about what error they should be reporting when my function fails.

[freebsd_igalic.co]

address reviewers comments

[freebsd_igalic.co]

address @kp's review comment.

[jamie]

sys/kern/kern_jail.c

3667

I would prefer an explicit cast to bool (even though the function signature will implicitly cast) when returning non-0/1 values. Or a "!= 0" like prison_allow() uses, which may be more clear to the reader.

Really, this whole thing is just for code readability, which is why I don't mind the churn.

[freebsd_igalic.co]

sys/netinet/in_jail.c

153

this is still saying int not bool

[freebsd_igalic.co]

address @jamie's and my own review comments

[freebsd_igalic.co]

sys/kern/kern_jail.c

2801

i reckon we could (should? ) add != 0 here too, eh?

sys/sys/jail.h

414

we could make this into a bool macro, too

[jamie]

sys/kern/kern_jail.c

2801

Yes, let's keep it consistent.

sys/netinet/in_jail.c

170–171

Even though it's redundant, there should be a "!= 0" here (and in the ip6 version), which makes it clear that prison_get_ip4() isn't boolean itself, hinting that it's a function that normally returns zero.

sys/sys/jail.h

414

No need - that's a clearly boolean expression.

[freebsd_igalic.co]

address review comments

[freebsd_igalic.co]

sys/kern/kern_jail.c

2803

I've put comments onto the functions I've touched… by copy and paste
but I'm struggling to it here.

Here's what I have so far:

/*
 * See if a prison has the specific allow flag set. The prison should be locked,
 * or only the single bit is examined, without regard to any other prison data.
 */

at least according to the jail.h documentation it should be locked, but i'm not seeing any evidence of that in the code using it.

[jamie]

sys/kern/kern_jail.c

2803

The evidence is in the lack of locking code in the function ;-).

Normally I would have some sort of locking assertion in the code, but that's prevented by the second use case (the noted PR_IP4 and PR_IP6). It's used enough that I don't want to force locking when they're only checking for these flags that are known not to change for the lifetime of the prison and thus can be read without locking. Most other flags will require locking, but I leave that up to the caller.

[kevans]

sys/kern/kern_jail.c

2803

I think we've had some slight miscommunication here, as this is a proposed comment for prison_allow just below.

Generally, this should follow the same locking model as the pr_allow field (pr_mtx or allprison_lock to read), but all or most of the existing callers appear to just accept a race here.

[jamie]

sys/kern/kern_jail.c

2803

Yes of course - I didn't catch the "allow" in the original note.

The above comment applies to this function as well. While there aren't any allow flags that are create-only, most of the prison_allow() callers check a single bit as their only interaction with the prison. An exception is some sysctl-handling functions that check the bit here and then set it there without any locking, but they would have to release the prison lock anyway, so it's still roughly equivalent.

[freebsd_igalic.co]

add comment for prison_allow()