Page MenuHomeFreeBSD

MAC/do: Use prison_lock()/prison_unlock()
AcceptedPublic

Authored by olce on Fri, Nov 15, 5:06 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 20, 8:01 AM
Unknown Object (File)
Tue, Nov 19, 9:41 AM
Unknown Object (File)
Mon, Nov 18, 9:47 PM
Unknown Object (File)
Mon, Nov 18, 9:25 PM
Unknown Object (File)
Mon, Nov 18, 4:11 PM
Unknown Object (File)
Mon, Nov 18, 3:38 PM
Unknown Object (File)
Sun, Nov 17, 1:41 PM
Unknown Object (File)
Sat, Nov 16, 4:27 AM

Details

Reviewers
bapt
Summary

This revision is part of a series. Click on the Stack tab below to see the context.
This series has also been squeezed into D47633 to provide an overall view.

Commit message:
Instead of fiddling directly with 'pr_mtx'.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 60586
Build 57470: arc lint + arc unit

Event Timeline

olce requested review of this revision.Fri, Nov 15, 5:06 PM

It looks like prison_lock/prison_unlock is not widely used, other than in sys/kern/sysv_msg.c. Should this be done more widely?

It looks like prison_lock/prison_unlock is not widely used, other than in sys/kern/sysv_msg.c. Should this be done more widely?

In sys/sys/jail.h:

/*
 * Lock/unlock a prison.
 * XXX These exist not so much for general convenience, but to be useable in
 *     the FOREACH_PRISON_DESCENDANT_LOCKED macro which can't handle them in
 *     non-function form as currently defined.
 */

But I think there is no harm to use them in general. Is there any concern? Or we can adjust this comment?

Is there any concern? Or we can adjust this comment?

No concern, we should just be (eventually) consistent and either prefer and use these here and in general, or if we don't prefer them we should abandon this review.

Is there any concern? Or we can adjust this comment?

No concern, we should just be (eventually) consistent and either prefer and use these here and in general, or if we don't prefer them we should abandon this review.

I have the same thought. Although these two functions are trivial, I like its encapsulation and think it should be used in general when possible. It seems we have many mtx_{,un}lock wraps in the code, although mostly in the form of macro, and seems limited to the scope of a single driver.

This revision is now accepted and ready to land.Tue, Nov 19, 7:54 AM

No concern, we should just be (eventually) consistent and either prefer and use these here and in general, or if we don't prefer them we should abandon this review.

I have the same thought. Although these two functions are trivial, I like its encapsulation and think it should be used in general when possible. It seems we have many mtx_{,un}lock wraps in the code, although mostly in the form of macro, and seems limited to the scope of a single driver.

I had seen the existing comment, and do not have a strong opinion (but a slight bias towards using prison_lock()/prison_unlock() obviously). Encapsulation is great (in particular it makes it easier to find uses of prison locks) but at the same time it obfuscates the underlying meaning, which in this case is also very simple. It could prove useful if the jail locking protocol changes (i.e., we switch it to R/W locks) but I'm not aware of any such need/plan at this point.

If you're OK with the change, let's keep this revision like that and convert the other uses of pr_mtx separately.