WIP Cross-platform support. #1
Conversation
|
As an alternative to sysinfo, there is also https://crates.io/crates/proclist which has a narrower set of functionality, but seems to be less used/supported. |
|
That looks really good from a first glance. 👍 I'll take a deeper look later today. |
| } | ||
| pid: Pid, | ||
| handle: ProcessHandle, | ||
| modules: HashMap<String, Address> |
There was a problem hiding this comment.
Out of these, pid is the only necessary variable. We probably shouldn't assume that modules never changes, and handle (on some platforms) is literally the same as pid. We may want to consider making pid be the base type and creating an extension trait.
There was a problem hiding this comment.
Here are my thoughts on what we can do with modules:
- Keep it as-is: we won't be able to detect plugins loading/unloading.
- Re-parse every time: could be a bit slow
- re-parse in
Process::module_address(): we'll recompute in a loop... - re-parse in a new
Process::modules(), move logic of module_address into runtime: better, but we're still gonna be calling this many times per second.
- re-parse in
- Caching? parse once at creation, re-parse if Error::ModuleDoesntExist or if a read based off of a module fails?
There was a problem hiding this comment.
Currently, this is setup with Process::modules.
| @@ -519,7 +519,7 @@ fn read_into_buf(ctx: &mut Ctx, address: u64, buf: u32, buf_len: u32) { | |||
| let buf = &memory[buf..buf + buf_len]; | |||
| if let Some(process) = &env.process { | |||
| let mut byte_buf = vec![0; buf.len()]; | |||
| process.read_buf(address, &mut byte_buf).unwrap(); | |||
| process.read_buf(address as usize, &mut byte_buf).unwrap(); | |||
There was a problem hiding this comment.
TODO: may want to use TryInto here
kitlith
force-pushed
the
wasmer-crossplatform
branch
from
a08e602
to
80660f6
Compare
| .or(Err(Error::ProcessDoesntExist))? | ||
| .into_iter() | ||
| .filter(|range| range.filename().is_some()) | ||
| .map(|range| (range.filename().clone().unwrap(), range.start())) |
There was a problem hiding this comment.
assuming the module string we're testing against is just supposed to be the filename without the path (e.g., "hl2.exe") we should fix this so that only the filename is returned here, not the whole path.
There was a problem hiding this comment.
should this also be without the extension? e.g. "hl2" instead of "hl2.exe"?
|
BTW, wasmer apparently already has WASI support: so you shouldn't need to implement the WASI api yourself... i think. |
|
Yeah we really shouldn't be implementing it ourselves, BUT for that we need theirs to fit our use case. And that is really tight control over what the auto splitter is allowed to do and how it does so. So all the printing for example goes straight to our log instead of to the actual terminal, only certain files can be read, none can be written to by default, and so on. So unless theirs is really customizable (and I think back then when I checked it wasn't), we may need to have our own implementation where we have full control over what we allow. |
|
So, it's partially customizable. You can specify folders and such it has access to... which passes through libpreopen, which I saw you were having issues with elsewhere. We also don't have access to the internals to do stuff like manually add a file descriptor to the internal table and then pass it to our code... I'd probably start by forking wasmer-wasi and increasing our access to the internals so we can at least perform workarounds like the above. We should probably also open a discussion with the people behind wasmer for creating an api that we can work with to accomplish what we need. In the meantime though: is this good to be merged into your development branch? We can continue with additional PRs. |
The auto splitter runtime will now gracefully shut down the auto splitters if they fail to execute instead of panicking like it did before. Also all the message are now reported using the log crate instead of being printed.
I don't see any reason to introduce lambdas when not necessary.
- Process::modules, dynamically get modules. - Port commented stuff to work with the wasmer stuff. - Others?
kitlith
force-pushed
the
wasmer-crossplatform
branch
from
8b6dadc
to
9a1bf87
Compare
Not sure what's up with the windows CI failures, time to muck with different versions of wasmer-runtime i guess.
0.5.7 -> 0.13.1
|
Closing in favor of #2 |
To achieve feature parity with existing implementation, we still need to implementProcess::with_name. Either we need to bring in a crate that can do cross-platform process listing, or implement it ourselves with the platform specific APIs.Process::pathis also unimplemented, but I notice that it's currently unused, so I'm not sure how necessary it is.EDIT: These have now been reimplemented.
I should note that I have only compiled this, I have not tried to run/test this.
I'd also like to comment that some modules are loaded/unloaded dynamically, so we may want to query them on-request, instead of querying them once when we start working with the program.