New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make most of `kernel` crate-visible only, rather than public. #1089

Merged
merged 1 commit into from Jul 10, 2018

Conversation

Projects
None yet
6 participants
@bradjc
Copy link
Contributor

bradjc commented Jul 6, 2018

An enhancement to Rust is the crate visibility modifier (rust-lang/rust#44660). This can be used instead of pub for things which should only be available within the crate, and not publicly available to all users of the crate.

This PR updates the kernel crate to use this in many places where functions and variables shouldn't be accessible outside of the kernel crate.

This is part of tracking issue #1088.

Testing Strategy

This pull request was tested by compiling. It doesn't actually make any logic changes.

TODO or Help Wanted

n/a

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make formatall.
kernel: new pub/crate feature
Use the `crate` visibility modifier to hide functions that are internal
to kernel use only.
@ppannuto
Copy link
Member

ppannuto left a comment

You're a hero for pub(crate)ing all of these and then again crateing them

@phil-levis
Copy link
Collaborator

phil-levis left a comment

This is great. I didn't check if there are things that should be not pub but still are, but I think you made a few things crate that might want to stay pub.

@@ -23,21 +23,21 @@ struct AtomicUsize {
}

impl AtomicUsize {

This comment has been minimized.

@phil-levis

phil-levis Jul 7, 2018

Collaborator

Should this be crate-restricted? I could imagine cases when peripherals (e.g., that directly handle interrupts) might need it. Right now we put all interrupts through kernel serialization but it's important that chips be able to do this in theory. Also, depending on the memory consistency model, they might need to.

This comment has been minimized.

@alevy

alevy Jul 7, 2018

Member

You're right, though #1083 proposes getting rid of this anyway since there now there is a replacement for it in Rust's core library (which is like what other code should use).

@@ -103,7 +103,7 @@ pub fn schedule(callback: FunctionCall, appid: AppId) -> bool {
/// app owns and can write to. This includes the app's code and data and any
/// padding at the end of the app. It does not include the TBF header, or any
/// space that the kernel is using for any potential bookkeeping.
pub fn get_editable_flash_range(app_idx: usize) -> (usize, usize) {
crate fn get_editable_flash_range(app_idx: usize) -> (usize, usize) {

This comment has been minimized.

@phil-levis

phil-levis Jul 7, 2018

Collaborator

I suspect this particular method will be useful for capsules that access flash. But if there's another pub method that effectively exports it (with more checks?) that could be OK.

This comment has been minimized.

@alevy

alevy Jul 7, 2018

Member

Agreed that this is potentially useful functionality to expose.

Doing it with a raw usize for the app id isn't definitely not stable (since we have long had plans to change the underlying format and meaning of an AppId, and probably will always want to keep that opaque). It also likely should be a privileged operation of some sort (e.g. requiring a capability).

This comment has been minimized.

@bradjc

bradjc Jul 9, 2018

Contributor

We do:

pub fn get_editable_flash_range(&self) -> (usize, usize) {
process::get_editable_flash_range(self.idx)
}

self.tasks.dequeue().map(|cb| {
self.kernel.decrement_work();
cb
})
}

pub fn mem_start(&self) -> *const u8 {
crate fn mem_start(&self) -> *const u8 {

This comment has been minimized.

@phil-levis

phil-levis Jul 7, 2018

Collaborator

I'd suggest keeping this accessors about process memory layout pub. It would be useful, for example, to have a capsule that provides a system call interface allowing a process to find out its layout (e.g., for debugging).

This comment has been minimized.

@alevy

alevy Jul 7, 2018

Member

The memop system call provides this information already, and it's passed into the process's _start symbol.

There might be a use for a (probably somewhat privileged) capsule to get this kind of information regardless (e.g. dump the state of the system to a debug log) In any case capsules don't have a way to get a hold of a Process struct directly right now, so they couldn't call this method in practice anyway.

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 7, 2018

@phil-levis I agree at a high level with the concern of hiding functionality that was previously public.

I think in the case of the kernel crate, since we haven't really thought carefully about a stable public interface, it's probably actually better to expose less publicly as early as possible (i.e. now) (really almost nothing should be public at the moment).

This is more urgently the the case for things in process.rs since that module is subject of an pretty big internal redesign for portability across architectures and to support actually replacing processes at runtime.

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 7, 2018

This PR updates the kernel crate to use this in many places where functions and variables shouldn't be accessible outside of the kernel crate.

@bradjc is it possible to summarize what is left public after this PR? My sense is that it should be very very very little, but hard to tell from the GitHub diff.

@alevy alevy added the P-Significant label Jul 7, 2018

@alevy alevy changed the title kernel: Use the crate visibility modifier Make most of `kernel` crate-visible only, rather than public. Jul 7, 2018

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 8, 2018

I agree, most methods should be hidden -- I just tried to comment on a few that might not be. For Process, sure. But I'd rather have a principled discussion on what information on the kernel is public (or not), then change the APIs to reflect that.

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 9, 2018

pub on a function does not mean that it was public outside of the crate (only that it could be). If this PR changes any policy then that is a mistake, it is only intended to make visibility more explicit.

Here is roughly what is public: (thanks rust!)

tock/kernel/src/lib.rs

Lines 19 to 58 in f638195

#[macro_use]
pub mod common;
#[macro_use]
pub mod debug;
pub mod hil;
pub mod ipc;
mod callback;
mod driver;
mod grant;
mod mem;
mod memop;
mod platform;
mod process;
mod returncode;
mod sched;
mod syscall;
mod tbfheader;
pub use callback::{AppId, Callback};
pub use driver::Driver;
pub use grant::Grant;
pub use mem::{AppPtr, AppSlice, Private, Shared};
pub use platform::systick::SysTick;
pub use platform::{mpu, Chip, Platform};
pub use platform::{ClockInterface, NoClockControl, NO_CLOCK_CONTROL};
pub use returncode::ReturnCode;
pub use sched::Kernel;
// These symbols must be exported for the arch crate to access them.
pub use process::APP_FAULT;
pub use process::SYSCALL_FIRED;
// Export only select items from the process module. To remove the name conflict
// this cannot be called `process`, so we use a shortened version. These
// functions and types are used by board files to setup the platform and setup
// processes.
pub mod procs {
pub use process::{load_processes, FaultResponse, Process};
}

@alevy

alevy approved these changes Jul 9, 2018

@phil-levis
Copy link
Collaborator

phil-levis left a comment

Looks good.

@phil-levis
Copy link
Collaborator

phil-levis left a comment

Looks good.

@niklasad1
Copy link
Member

niklasad1 left a comment

Grumble, crate enum Foo is sugar for pub(crate) enum Foo?

I still find pub(crate) Foo more readable!

I wonder if they plan do same for pub(super) Foo -> super Foo that would be even worse!

I hope this syntax will never get stabilized 🦀

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 10, 2018

I'm going to call this critical mass & merge.

@ppannuto ppannuto merged commit 5f483ac into master Jul 10, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@ppannuto ppannuto deleted the explicit-pub branch Jul 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment