Stream: git-wasmtime

Topic: wasmtime / PR #5210 c-api: add wasi_config_set_stdout_pipe

view this post on Zulip Wasmtime GitHub notifications bot (Nov 05 2022 at 12:46):

ShuP1 opened PR #5210 from c-api-stdio-pipe to main:

As discussed in issue #4372 and respective issues about python and go bindings.

Can replace wasi_config_inherit_stdout in examples/wasi/main.c:79

wasi_config_t *wasi_config = wasi_config_new();
assert(wasi_config);
wasi_read_pipe_t *wasi_stdout_reader = NULL;
wasi_write_pipe_t *wasi_stdout_writer = NULL;
wasi_pipe_new(32, &wasi_stdout_reader, &wasi_stdout_writer);
assert(wasi_stdout_reader && wasi_stdout_writer);
wasi_config_set_stdout_pipe(wasi_config, wasi_stdout_writer);
error = wasmtime_context_set_wasi(context, wasi_config);
// run and delete store
size_t expected = wasi_read_pipe_len(wasi_stdout_reader);
byte_t *buf = malloc(expected);
size_t filled = wasi_read_pipe_read(wasi_stdout_reader, buf, expected);
fprintf(stderr, "Pipe:\n%.*s\n", (int)filled, buf);
wasi_read_pipe_delete(wasi_stdout_reader);

See https://github.com/ShuP1/wasmtime/commit/d818f6b8153c71f94283aa0e9c258e57e795e019

This PR replaces #5189
(cc @alexcrichton)

view this post on Zulip Wasmtime GitHub notifications bot (Nov 05 2022 at 12:53):

ShuP1 edited PR #5210 from c-api-stdio-pipe to main:

As discussed in issue #4372 and respective issues about python and go bindings.

Can replace wasi_config_inherit_stdout in examples/wasi/main.c:79

wasi_config_t *wasi_config = wasi_config_new();
assert(wasi_config);
wasi_read_pipe_t *wasi_stdout_reader = NULL;
wasi_write_pipe_t *wasi_stdout_writer = NULL;
wasi_pipe_new(32, &wasi_stdout_reader, &wasi_stdout_writer);
assert(wasi_stdout_reader && wasi_stdout_writer);
wasi_config_set_stdout_pipe(wasi_config, wasi_stdout_writer);
error = wasmtime_context_set_wasi(context, wasi_config);
// run...
size_t expected = wasi_read_pipe_len(wasi_stdout_reader);
byte_t *buf = malloc(expected);
size_t filled = wasi_read_pipe_read(wasi_stdout_reader, buf, expected);
fprintf(stderr, "Pipe:\n%.*s\n", (int)filled, buf);
wasi_read_pipe_delete(wasi_stdout_reader);

See https://github.com/ShuP1/wasmtime/commit/d818f6b8153c71f94283aa0e9c258e57e795e019

This PR replaces #5189
(cc @alexcrichton)

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2022 at 15:46):

alexcrichton created PR review comment:

I think it's ok to use void* instead of byte_t* to be more akin to read/write

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2022 at 15:46):

alexcrichton created PR review comment:

I think it's ok to remove this "However ..." clause because pipes leading to deadlock aren't necessarily unique to WASI.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2022 at 15:46):

alexcrichton created PR review comment:

I think this can be &mut *mut wasi_write_pipe_t which should remove the need for this function to be unsafe.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2022 at 15:46):

alexcrichton created PR review comment:

I'm not sure what this is intending to communicate? Could you clarify what this is explaining?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2022 at 15:46):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2022 at 15:46):

alexcrichton created PR review comment:

I'm not sure that this read/write impl are the best for WASI. As modeled today I think it would be best to implement synchronous I/O here but this is sort of a form of nonblocking I/O without the ability to be notified when bytes are ready.

Specifically Read will not block when the deque is empty, instead returning Ok(0) which many libraries actually interpret as EOF. Additionally Write will not block when it's full, instead returning Ok(0) which I think libraries take various viewpoints on.

I think it would be better for Read to block on writing and for writing to block on Read.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2022 at 15:46):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2022 at 15:46):

alexcrichton created PR review comment:

I think this can be omitted since it doesn't do anything

view this post on Zulip Wasmtime GitHub notifications bot (Nov 08 2022 at 06:16):

ShuP1 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 08 2022 at 06:16):

ShuP1 created PR review comment:

std::io::Write::flush has not default body. IMO to force implementers to thinks about it.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 08 2022 at 06:24):

ShuP1 created PR review comment:

Wasi fd_read (like POSIX one), returns the number of byte read.
Then the queue is empty, the returned value is zero.
Effects on programs may varies.
For example, a c program without checks will just ignore this.
While rust print macro unwraps std::io::Read::read_exact and so panic.

If you have a good way to explain that I'm taking it…

view this post on Zulip Wasmtime GitHub notifications bot (Nov 08 2022 at 06:24):

ShuP1 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 08 2022 at 06:25):

ShuP1 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 08 2022 at 06:36):

ShuP1 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 08 2022 at 06:36):

ShuP1 created PR review comment:

Is it possible to "null check" a &mut since it's expected to be a valid reference ?

If not, want would append if the function is given a null pointer ? UB?

Or, is a function documentation enough to be confident than the function is used properly ?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 08 2022 at 06:51):

ShuP1 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 08 2022 at 06:51):

ShuP1 created PR review comment:

UNIX pipe returning 0 bytes read means not more data for now

Synchronous IO seems be more user friendly.
But there if not way to read all of stdin for example.
It would just be a deadlock after the last byte.

Furthermore I don't see how we can't contact the runtime to suspend the execution and run the "writer" instance.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 08 2022 at 06:56):

ShuP1 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 08 2022 at 06:57):

ShuP1 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 08 2022 at 09:00):

ShuP1 updated PR #5210 from c-api-stdio-pipe to main.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 08 2022 at 15:26):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 08 2022 at 15:26):

alexcrichton created PR review comment:

Yes that makes sense but VecDeque::flush is a noop.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 08 2022 at 15:28):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 08 2022 at 15:28):

alexcrichton created PR review comment:

My point is that the &mut layer is part of the contract of the function, it must be valid. What it points to could be invalid, hence the next layer of raw pointers.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 08 2022 at 15:29):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 08 2022 at 15:29):

alexcrichton created PR review comment:

I don't think unix pipes return 0 when there's no more data, when the write pipe is still open then it blocks waiting for more data. I think the same thing should happen here, the reader blocks until the writer has written data or the writer has been dropped.

Last updated: Nov 22 2024 at 16:03 UTC