Skip to content

[crates] Add sworn-disk crate#342

Open
lucassong-mh wants to merge 2 commits intoocclum:masterfrom
lucassong-mh:dev-sworndisk
Open

[crates] Add sworn-disk crate#342
lucassong-mh wants to merge 2 commits intoocclum:masterfrom
lucassong-mh:dev-sworndisk

Conversation

@lucassong-mh
Copy link
Contributor

This PR adds sworn-disk to NGO's crates

RFC issue: #328

@lucassong-mh lucassong-mh force-pushed the dev-sworndisk branch 2 times, most recently from cf26c8d to 984b6b6 Compare December 4, 2022 15:53
Copy link
Contributor

@tatetian tatetian left a comment

Choose a reason for hiding this comment

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

I like the overall code structure; it is clear and modular.

The following is some of my initial comments. Most of them is about the index. I will submit more comments for other modules later.

Copy link
Contributor

@tatetian tatetian left a comment

Choose a reason for hiding this comment

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

More comments.

Copy link
Contributor

@tatetian tatetian left a comment

Choose a reason for hiding this comment

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

What I am about to say does not mean I am encouraging the PR author to take the advice into action now. Because if it was to be done, it would mean a major overhaul. But I think it is beneficial to discuss the future direction of improvement now.

Personally, I think the current codebase suffers a structural problem. The problem is that the codebase looks modular, but the modules are in fact highly entangled.

One of the main symptom is that the index cannot be tested alone. The index directly depends on the checkpoint. Without the checkpoint initialized, it is impossible to do anything on the index. The checkpoint not only contains the metadata required by the index, but also the rest of SwornDisk. So it is safe to say that the index code is entangled with the rest of SwornDisk.

I think that the current codebase is under too much influence of the paper's design. The paper's design is mainly concerned with security and clarity, not software engineering practices. So making the codebase closely follow the paper's design is harmful to the codebase.

But no worry. We have the time to figure out a plan to refactor the code structure later.

Copy link
Contributor

@tatetian tatetian left a comment

Choose a reason for hiding this comment

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

The new version looks promising. Good job on improving the code quality.

One major issue is that we need to rename SwornDisk to JinDisk. The crate name should be jindisk (Yes, no - between jin and disk) as JinDisk should be considered one name and word, not two. On the other hand, Jinzhao Disk is two words.

Copy link
Contributor

@tatetian tatetian left a comment

Choose a reason for hiding this comment

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

Thanks for submitting the updated version. Here are my comments for the first commit. I will submit the comments for the second commit later.

Copy link
Contributor

@tatetian tatetian left a comment

Choose a reason for hiding this comment

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

Well done! I really appreciate your persevere and patience in improving the code quality constantly. The code is now good enough for merge. Before we do, let's fix some trivial issues listed in the comments below. Then, you can re-submit this PR to the Occlum repo (not NGO). See you there!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants