Skip to content
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

[catnap] Enhancement: re-architect Catnap #851

Merged
merged 4 commits into from
Jul 28, 2023
Merged

Conversation

iyzhang
Copy link
Contributor

@iyzhang iyzhang commented Jul 20, 2023

This PR re-architects the Catnap libOS such that the libOS follows an object-oriented architecture where the libOS aggregates CatnapQueues but does not know about sockets or the libc calls. The Socket is composed by CatnapQueue but CatnapQueue does not know about libc. And all libc syscalls are encapsulated inside Socket. Thus there is a strict encapsulation hierarchy, where each level only knows the level below: CatnapLibOS encapsulates-> CatnapQueue encapsulates-> Socket encapsulates-> libc syscalls.

Copy link
Contributor

@anandbonde anandbonde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks good.

src/rust/catnap/queue.rs Outdated Show resolved Hide resolved
src/rust/catnap/queue.rs Outdated Show resolved Hide resolved
src/rust/catnap/queue.rs Outdated Show resolved Hide resolved
@iyzhang iyzhang force-pushed the enhancement-catnap-reorg branch 3 times, most recently from 2071efe to 315bfaa Compare July 21, 2023 03:17
src/rust/catnap/socket.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@ppenna ppenna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good.

I noted down some changes in the semantics that we may need to review.

Also, if we are merging this, I would ask you to provide at least one-line comment for each public function that was introduced.

src/rust/catnap/mod.rs Show resolved Hide resolved
src/rust/catnap/queue.rs Outdated Show resolved Hide resolved
src/rust/catnap/queue.rs Show resolved Hide resolved
src/rust/catnap/mod.rs Outdated Show resolved Hide resolved
src/rust/catnap/socket.rs Outdated Show resolved Hide resolved
src/rust/catnap/queue.rs Show resolved Hide resolved
src/rust/catnap/mod.rs Show resolved Hide resolved
src/rust/catnap/queue.rs Show resolved Hide resolved
src/rust/catnap/mod.rs Outdated Show resolved Hide resolved
src/rust/catnap/mod.rs Outdated Show resolved Hide resolved
@ppenna ppenna added the enhancement Enhancement Request on an Existing Feature label Jul 21, 2023
@ppenna ppenna force-pushed the enhancement-catnap-reorg branch 2 times, most recently from ca2278a to 427e7d2 Compare July 24, 2023 11:26
@iyzhang iyzhang mentioned this pull request Jul 24, 2023
@ppenna ppenna force-pushed the enhancement-catnap-reorg branch 3 times, most recently from 93d0bb1 to aa7f0c1 Compare July 27, 2023 18:38
Copy link
Contributor Author

@iyzhang iyzhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol, I cannot approve this PR because it is my PR, but I "approve" it. It is definitely a step in the right direction. I think we could go ahead and merge this and then think further about how to drive the state machine.

src/rust/catnap/mod.rs Show resolved Hide resolved
src/rust/catnap/mod.rs Show resolved Hide resolved
src/rust/catnap/mod.rs Show resolved Hide resolved
@ppenna ppenna merged commit 0173559 into dev Jul 28, 2023
11 checks passed
@ppenna ppenna deleted the enhancement-catnap-reorg branch July 28, 2023 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement Request on an Existing Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants