Page MenuHomeFreeBSD

jail: convert several functions from int to bool
AcceptedPublic

Authored by me_igalic.co on Apr 8 2021, 8:00 PM.

Details

Reviewers
jamie
Group Reviewers
Jails
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.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 38578
Build 35467: arc lint + arc unit

Event Timeline

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
3279

That's a bit redundant.

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.

sys/kern/kern_jail.c
3279

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

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

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.

address @kp's review comment.

sys/kern/kern_jail.c
3279

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.

sys/netinet/in_jail.c
220

this is still saying int not bool

address @jamie's and my own review comments

me_igalic.co retitled this revision from kern_jail: convert several functions from int to bool to jail: convert several functions from int to bool.Apr 10 2021, 10:39 AM
me_igalic.co edited the summary of this revision. (Show Details)
me_igalic.co changed the repository for this revision from rS FreeBSD src repository - subversion to R10 FreeBSD src repository.
me_igalic.co marked an inline comment as done.
sys/kern/kern_jail.c
2517

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

sys/sys/jail.h
405

we could make this into a bool macro, too

sys/kern/kern_jail.c
2517

Yes, let's keep it consistent.

sys/netinet/in_jail.c
237–238

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
405

No need - that's a clearly boolean expression.

sys/kern/kern_jail.c
2519

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.

sys/kern/kern_jail.c
2519

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 added inline comments.
sys/kern/kern_jail.c
2519

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.

sys/kern/kern_jail.c
2519

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.

add comment for prison_allow()

This revision is now accepted and ready to land.Sep 4 2021, 10:26 PM