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.

Sign up for GitHub

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

Jump to bottom

sandbox code optimization #55

Merged
merged 1 commit into from
Nov 8, 2023
Merged

sandbox code optimization #55

merged 1 commit into from
Nov 8, 2023

Conversation

dierbei
Copy link
Contributor

@dierbei dierbei commented Jul 28, 2023

There still seems to be a problem, see details: rust-lang/rust-clippy#11247

In the future, I will rebase after the bug fix is completed to keep the commit record clear.

@dierbei dierbei force-pushed the op-feature branch 3 times, most recently from 70c7e7f to 4378503 Compare August 3, 2023 08:24
@dierbei
Copy link
Contributor Author

dierbei commented Aug 3, 2023

Now that features have been removed, the project can be built with the following command:

  • cargo build --bin qemu
  • cargo build --bin stratovirt
  • cargo build --bin cloud_hypervisor

@dierbei dierbei force-pushed the op-feature branch 3 times, most recently from 33ee3ee to 8d61a43 Compare August 3, 2023 09:35
@dierbei dierbei force-pushed the op-feature branch 3 times, most recently from 24ca8f8 to 840dd43 Compare August 11, 2023 13:24
@dierbei
Copy link
Contributor Author

dierbei commented Aug 19, 2023

Seems like it could be further optimized.

I will try to move qumu, stratovirt and cloud_hypervisor to separate folders under bin.

The advantage of this is that you can use the command directly: cargo build

@dierbei dierbei force-pushed the op-feature branch 3 times, most recently from 832af0a to b9acec0 Compare August 21, 2023 08:53
@dierbei dierbei requested review from a team as code owners September 1, 2023 06:20
@dierbei dierbei force-pushed the op-feature branch 2 times, most recently from 43162d0 to 76d6a38 Compare September 1, 2023 06:34
@Burning1020
Copy link
Member

Will take a look.

@dierbei dierbei force-pushed the op-feature branch 6 times, most recently from b9acec0 to bdf27fa Compare September 1, 2023 06:58
Burning1020
Burning1020 reviewed Sep 9, 2023
View reviewed changes
.gitignore Outdated Show resolved Hide resolved
vmm/sandbox/src/lib.rs Outdated
pub const CONFIG_STRATOVIRT_PATH: &str = "/var/lib/kuasar/config_stratovirt.toml";
pub const CONFIG_CLH_PATH: &str = "/var/lib/kuasar/config_clh.toml";

pub async fn init_cloud_hypervisor_sandboxer(
Copy link
Member

Choose a reason for hiding this comment

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

why is this init function needed? maybe move it to cloud_hypervisor folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried moving the initialization functions for qemu,cloud_hypervisor, and stratovirt in lib.rs to their own folders.

vmm/sandbox/Cargo.toml
@@ -7,12 +7,6 @@ edition = "2021"
[profile.release]
panic = 'abort'

[features]
default = []
qemu = ["qapi/qmp", "qapi/async-tokio-all", "unshare", "os_pipe"]
Copy link
Member

Choose a reason for hiding this comment

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

Please try to add the mapping of binary name and its main.rs path. https://doc.rust-lang.org/cargo/reference/cargo-targets.html#configuring-a-target

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added configuration for qemu, cloud_hypervisor, stratovirt.

@Burning1020
Copy link
Member

One more thing is to check Makefile targets.

@dierbei dierbei force-pushed the op-feature branch 5 times, most recently from 8fbdad4 to 1aeaae3 Compare September 12, 2023 08:52
@dierbei
Copy link
Contributor Author

dierbei commented Sep 12, 2023

One more thing is to check Makefile targets.

I added the --bin argument to the bin/vmm-sandboxer fragment in the Makefile.

Burning1020
Burning1020 reviewed Oct 18, 2023
View reviewed changes
Copy link
Member

@Burning1020 Burning1020 left a comment

Choose a reason for hiding this comment

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

/cc @abel-von @Vanient @flyflypeng

vmm/sandbox/src/bin/cloud_hypervisor/main.rs Outdated Show resolved Hide resolved
abel-von
abel-von reviewed Oct 28, 2023
View reviewed changes
wasm/src/sandbox.rs Show resolved Hide resolved
wasm/src/sandbox.rs Show resolved Hide resolved
vmm/sandbox/src/bin/cloud_hypervisor/main.rs Outdated Show resolved Hide resolved
@dierbei
Copy link
Contributor Author

dierbei commented Nov 7, 2023

@Burning1020 @abel-von Thanks for the suggestion, I'll revise it soon.

@dierbei
Sandboxer: Replace features with bin parameter
b111fc9 
Signed-off-by: dierbei <1628652790@qq.com>
Burning1020
Burning1020 approved these changes Nov 7, 2023
View reviewed changes
Copy link
Member

@Burning1020 Burning1020 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Good job. Thank you.

@Burning1020 Burning1020 merged commit ef6a9e2 into kuasar-io:main Nov 8, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abel-von abel-von abel-von left review comments

@Burning1020 Burning1020 Burning1020 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dierbei @Burning1020 @abel-von