[refactor] split sandbox shim #345
Conversation
jprendes
force-pushed
the
split-sandbox-shim
branch
30 times, most recently
from
4f5f18b to
1440c73
Compare
jprendes
force-pushed
the
split-sandbox-shim
branch
2 times, most recently
from
7ed6f58 to
3fe149b
Compare
|
Is this ready to review? |
|
This is waiting on #342 as it builds up on top of it. |
jprendes
force-pushed
the
split-sandbox-shim
branch
2 times, most recently
from
8097bb8 to
7d21b2e
Compare
jprendes
force-pushed
the
split-sandbox-shim
branch
from
7d21b2e to
80b1901
Compare
jprendes
force-pushed
the
split-sandbox-shim
branch
from
80b1901 to
764d6bf
Compare
This is now ready for review :-) |
There was a problem hiding this comment.
Thanks for the cleaning up! I really love the refactoring work here and the new addition of RemoteEventSender.
At the same time, I think this PR is bigger than a chore PR. I've asked some questions regarding the code difference between the old shim.rs and the refactored files. I want to make sure that as a spliting PR, no logic has changed in the shim file.
Could you please review this PR? @cpuguy83
jprendes
force-pushed
the
split-sandbox-shim
branch
2 times, most recently
from
3a2c703 to
d27c35a
Compare
jprendes
force-pushed
the
split-sandbox-shim
branch
from
d27c35a to
2126707
Compare
Changed the title to |
jprendes
force-pushed
the
split-sandbox-shim
branch
from
2126707 to
d65c7e2
Compare
There was a problem hiding this comment.
LGTM, I am hoping to get a second pair of eyes for reviewing. @jsturtevant @cpuguy83
Signed-off-by: Jorge Prendes <[email protected]>
jprendes
force-pushed
the
split-sandbox-shim
branch
from
d65c7e2 to
fa88237
Compare
| @@ -14,7 +14,7 @@ pub mod sync; | |||
| pub use error::{Error, Result}; | |||
| pub use instance::{Instance, InstanceConfig}; | |||
| pub use manager::{Sandbox as SandboxService, Service as ManagerService}; | |||
| pub use shim::{Cli as ShimCli, Local}; | |||
| pub use shim::Cli as ShimCli; | |||
There was a problem hiding this comment.
My editor is complaing about pub mod shim at line 10 now that shim.rs is gone.
I'm a bit fuzzy on module semantics, though.
There was a problem hiding this comment.
Ok, this was just an editor thing. It disappered on its own.
This PR splits the
sandbox/shim.rsmodule into smaller more maintainable submodules.In the process of doing so this PR also:
shimmodule that were publicly visible but not used outside the module.stdin,stdout,stderr, andbundle) are stored inInstanceConfigto avoid unnecessary convertions toStringback and forth. This also affects (hopefully simplifies) thestdiomodule..unwrap()in theshimmodule outside of tests and mutex poisoning.Local's event handling to avoid spining a thread just to forward messages.