Stream: git-wasmtime

Topic: wasmtime / PR #11965 c-api: wasi: Add custom callbacks fo...

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2025 at 20:12):

MangoPeachGrape opened PR #11965 from MangoPeachGrape:c-api/wasi/custom-callbacks to bytecodealliance:main:

Fixes #11963

I'm quite unsure on some things. I split the implementation out between the wasi and c-api crates, because I would've needed to add some dependencies into c-api crate to implement the required traits, not sure what's better?

Also, I don't particularly like the Arc<Mutex>, but I guess it's required for the async version?

Also also, const void* vs const unsigned char* for the buffer? (https://github.com/bytecodealliance/wasmtime/issues/11963#issuecomment-3476575567)

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2025 at 20:12):

MangoPeachGrape requested wasmtime-wasi-reviewers for a review on PR #11965.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2025 at 20:12):

MangoPeachGrape requested alexcrichton for a review on PR #11965.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2025 at 20:12):

MangoPeachGrape requested wasmtime-core-reviewers for a review on PR #11965.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2025 at 22:55):

alexcrichton submitted PR review:

Thanks! Mind adding a test for this too?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2025 at 22:55):

alexcrichton created PR review comment:

We've probably got 1-too-many traits already in the wasmtime-wasi crate, so mind moving this directly into the c-api crate? It should be fine to implement pollable/outputstream/asyncwrite directly on the c-api types and avoid the trait indirection here (should avoid arc/mutex as well I think)

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2025 at 13:34):

MangoPeachGrape requested pchickey for a review on PR #11965.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2025 at 13:34):

MangoPeachGrape requested wasmtime-default-reviewers for a review on PR #11965.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2025 at 13:34):

MangoPeachGrape updated PR #11965.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2025 at 13:42):

MangoPeachGrape submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2025 at 13:42):

MangoPeachGrape created PR review comment:

Moved everything into the c-api crate now. I was able to get rid of the mutex, but unless I'm mistaken, I think the arc is necessary for the clone in StdoutStream::async_stream()?

Also for tests, I see that they are now C++, does that mean I would need to add a C++ API for it to be tested?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2025 at 15:55):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2025 at 15:55):

alexcrichton created PR review comment:

Ah true yeah, Arc is still required. For tests it's ok to skip C++, but you'll probably be using a C++ wrapper mostly and then in the guts do someting C-like. Would that work ok?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2025 at 15:57):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2025 at 15:58):

alexcrichton created PR review comment:

For WASIp2 I'd then also say that a partial write just gets turned into an error (generated in the bindings here) saying that partial writes aren't supported in WASIp2's C API at this time.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2025 at 15:58):

alexcrichton created PR review comment:

For this it's ok to remove the Option in the function signature to have this statically be non-null. That can't be reflected in C's type system but it can be reflected in the documentation.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2025 at 15:58):

alexcrichton created PR review comment:

For this, WDYT about using a read/write-syscall-style interface? Something where ssize_t is returned and negative numbers are system errno-like things while positive numbers are how many bytes were written?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2025 at 22:39):

MangoPeachGrape submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2025 at 22:39):

MangoPeachGrape created PR review comment:

Seems like a good idea, but it seems like ssize_t is somehow not a standard type... is ptrdiff_t the correct choice then?

What do you mean exactly by "errno-like" -- Should negative numbers be just be arbitrarily user specified error codes, as things like io::Result::from_raw_os_error() is OS specific? If so, how should I create the io::Result, maybe io::ErrorKind::Other with "Write failed with error {code}"?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2025 at 22:57):

MangoPeachGrape updated PR #11965.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2025 at 23:01):

MangoPeachGrape submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2025 at 23:01):

MangoPeachGrape created PR review comment:

data and finalizer arguments are specified as "An optional ...", I'm wondering if that is enough to indicate that the others are required, or should I add a note that a callback is required?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 01:31):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 01:31):

alexcrichton created PR review comment:

Finalizer I'd leave as optional yeah, I'd only change the signature of callback. For docs it should be fine to say that callback is required to be non-null.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 01:46):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 01:46):

alexcrichton created PR review comment:

Oh? For me man 2 write shows the return being ssize_t...

But yeah I'm imaginig that negative errors turn into io::Error things. You should be able to use io::Error::raw_os_error and then plumb that error around I think? (I may be missing something though)

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 16:54):

MangoPeachGrape updated PR #11965.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 17:04):

MangoPeachGrape submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 17:04):

MangoPeachGrape created PR review comment:

Unless I'm completely mistaken, ssize_t in only a POSIX thing, so it isn't defined on Windows for example?

But is using io::Error::from_raw_os_error(wrote.abs()) fine? I guess the embedder will return different codes on different platforms, but that function will map them correctly?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 19:40):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 19:40):

alexcrichton created PR review comment:

This'll want to use LastOperationFailed rather than Trap to match the behavior of AsyncWrite below

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 19:40):

alexcrichton created PR review comment:

Mind making these optional and having the wasi feature activate them?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 19:40):

alexcrichton created PR review comment:

This is fine to use #[derive(Clone)] for -- that's not used in the wasi crates as it would erroneously place a Clone bound on the generics where they were otherwise behind an Arc but that's not a concern here.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 19:40):

alexcrichton created PR review comment:

Mind deduplicating this with the above by having a helper fn raw_write(&self, bytes: &[u8]) -> io::Result<usize> or similar?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 19:40):

alexcrichton created PR review comment:

Oh oops I didn't realize Windows didn't have that... I think ptrdiff_t should match roughly what we're looking for yeah (although it's a bit funky alas...)

And yeah io::Error should do the right platform-specific thing.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 20:45):

MangoPeachGrape updated PR #11965.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 20:46):

MangoPeachGrape submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 20:46):

MangoPeachGrape created PR review comment:

What about the wrote != bytes.len() branch below, should that use Trap or LastOperationFailed?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 20:56):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 20:56):

alexcrichton created PR review comment:

Oh, sorry, yeah that should be LastOperationFailed as well

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 21:00):

MangoPeachGrape updated PR #11965.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 21:03):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 21:03):

alexcrichton has enabled auto merge for PR #11965.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 21:19):

alexcrichton has disabled auto merge for PR #11965.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 21:19):

MangoPeachGrape updated PR #11965.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 21:46):

alexcrichton has enabled auto merge for PR #11965.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 21:46):

alexcrichton has disabled auto merge for PR #11965.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 21:46):

alexcrichton closed without merge PR #11965.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 21:46):

alexcrichton reopened PR #11965.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 21:46):

alexcrichton has enabled auto merge for PR #11965.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 21:46):

MangoPeachGrape commented on PR #11965:

Did I break CI... :laughing:

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2025 at 22:34):

alexcrichton merged PR #11965.

Last updated: Dec 06 2025 at 06:05 UTC