Stream: git-wasmtime

Topic: wasmtime / issue #8257 FileOutputStream::write() modify t...

Wasmtime GitHub notifications bot (Mar 28 2024 at 12:11):

liutao-liu opened issue #8257:

Two modifications to the p code：
https://github.com/bytecodealliance/wasmtime/blob/9c92881d3b936b5d2e593ded370609a6e31717d8/crates/wasi/src/filesystem.rs#L339
https://github.com/bytecodealliance/wasmtime/blob/9c92881d3b936b5d2e593ded370609a6e31717d8/crates/wasi/src/filesystem.rs#L359

The former is an invalid modification, because p is not a reference of self.mode.

Wasmtime GitHub notifications bot (Mar 28 2024 at 12:14):

liutao-liu edited issue #8257:

Two modifications to the p code：
https://github.com/bytecodealliance/wasmtime/blob/9c92881d3b936b5d2e593ded370609a6e31717d8/crates/wasi/src/filesystem.rs#L339
https://github.com/bytecodealliance/wasmtime/blob/9c92881d3b936b5d2e593ded370609a6e31717d8/crates/wasi/src/filesystem.rs#L359

The former is an invalid modification, because p is not a reference of self.mode.

Please help confirm if this is a bug. If it is, I'll submit a pr with my local changes.

Wasmtime GitHub notifications bot (Mar 28 2024 at 12:14):

liutao-liu edited issue #8257:

Two modifications to the p code：
https://github.com/bytecodealliance/wasmtime/blob/9c92881d3b936b5d2e593ded370609a6e31717d8/crates/wasi/src/filesystem.rs#L339
https://github.com/bytecodealliance/wasmtime/blob/9c92881d3b936b5d2e593ded370609a6e31717d8/crates/wasi/src/filesystem.rs#L359

The former is an invalid modification, because p is not a reference of self.mode.

Please help confirm if this is a bug. If it is, I'll submit a pr with my local changes.

```[tasklist]

Tasks

~~~

Wasmtime GitHub notifications bot (Mar 28 2024 at 12:14):

liutao-liu edited issue #8257:

Two modifications to the p code：
https://github.com/bytecodealliance/wasmtime/blob/9c92881d3b936b5d2e593ded370609a6e31717d8/crates/wasi/src/filesystem.rs#L339
https://github.com/bytecodealliance/wasmtime/blob/9c92881d3b936b5d2e593ded370609a6e31717d8/crates/wasi/src/filesystem.rs#L359

The former is an invalid modification, because p is not a reference of self.mode.

Please help confirm if this is a bug. If it is, I'll submit a pr with my local changes.

Wasmtime GitHub notifications bot (Mar 28 2024 at 13:29):

liutao-liu edited issue #8257:

Two modifications to the p code：
https://github.com/bytecodealliance/wasmtime/blob/9c92881d3b936b5d2e593ded370609a6e31717d8/crates/wasi/src/filesystem.rs#L359

https://github.com/bytecodealliance/wasmtime/blob/9c92881d3b936b5d2e593ded370609a6e31717d8/crates/wasi/src/preview1.rs#L1705

The former is an invalid modification, because self.mode is not a reference of postion, which is value pass in write_via_stream .

Please help confirm if this is a bug. If it is, I'll submit a pr with my local changes.

Wasmtime GitHub notifications bot (Mar 28 2024 at 15:07):

alexcrichton commented on issue #8257:

Could you clarify with a test case what the expected behavior is? Reviewing these mutations I don't see anything amiss myself, but a test case could help understand if there's a bug here

Wasmtime GitHub notifications bot (Mar 29 2024 at 01:10):

liutao-liu edited issue #8257:

Two modifications to the p code：
https://github.com/bytecodealliance/wasmtime/blob/9c92881d3b936b5d2e593ded370609a6e31717d8/crates/wasi/src/filesystem.rs#L359

https://github.com/bytecodealliance/wasmtime/blob/9c92881d3b936b5d2e593ded370609a6e31717d8/crates/wasi/src/preview1.rs#L1705

The former is an invalid modification, because self.mode is not a reference of postion in fd_write , which is passed by value in `write_via_stream` .

Please help confirm if this is a bug. If it is, I'll submit a pr with my local changes.

Wasmtime GitHub notifications bot (Mar 29 2024 at 01:10):

liutao-liu edited issue #8257:

Two modifications to the p code：
https://github.com/bytecodealliance/wasmtime/blob/9c92881d3b936b5d2e593ded370609a6e31717d8/crates/wasi/src/filesystem.rs#L359

https://github.com/bytecodealliance/wasmtime/blob/9c92881d3b936b5d2e593ded370609a6e31717d8/crates/wasi/src/preview1.rs#L1705

The former is an invalid modification, because self.mode is not a reference of postion in fd_write , which is passed by value in write_via_stream` .

Please help confirm if this is a bug. If it is, I'll submit a pr with my local changes.

Wasmtime GitHub notifications bot (Mar 29 2024 at 01:11):

liutao-liu edited issue #8257:

Two modifications to the p code：
https://github.com/bytecodealliance/wasmtime/blob/9c92881d3b936b5d2e593ded370609a6e31717d8/crates/wasi/src/filesystem.rs#L359

https://github.com/bytecodealliance/wasmtime/blob/9c92881d3b936b5d2e593ded370609a6e31717d8/crates/wasi/src/preview1.rs#L1705

The former is an invalid modification, because self.mode is not a reference of postion in fd_write , which is passed by value in write_via_stream.

Please help confirm if this is a bug. If it is, I'll submit a pr with my local changes.

Wasmtime GitHub notifications bot (Mar 30 2024 at 09:58):

liutao-liu commented on issue #8257:

Could you clarify with a test case what the expected behavior is? Reviewing these mutations I don't see anything amiss myself, but a test case could help understand if there's a bug here

Delete the modification in here All test case can run success. The reason is what I said before:self.mode is not a reference of postion in fd_write , which is passed by value in write_via_stream.


Wasmtime GitHub notifications bot (Mar 30 2024 at 09:59):

liutao-liu edited a comment on issue #8257:

Could you clarify with a test case what the expected behavior is? Reviewing these mutations I don't see anything amiss myself, but a test case could help understand if there's a bug here

Delete the modification in here All test case can run success:

The reason is what I said before:self.mode is not a reference of postion in fd_write , which is passed by value in write_via_stream:
![Uploading image.png…]()

Wasmtime GitHub notifications bot (Mar 30 2024 at 10:03):

liutao-liu edited a comment on issue #8257:

Could you clarify with a test case what the expected behavior is? Reviewing these mutations I don't see anything amiss myself, but a test case could help understand if there's a bug here

Delete the modification in here All test case can run success:

The reason is what I said before:self.mode is not a reference of postion in fd_write , which is passed by value in write_via_stream:

Wasmtime GitHub notifications bot (Mar 31 2024 at 19:44):

alexcrichton commented on issue #8257:

Sorry yeah our test suite for preview2 is not overly comprehensive. I've added a test in https://github.com/bytecodealliance/wasmtime/pull/8271 which fails if the code you're linking to fails.

It's a bit subtle and it needs a few different things in play, but I believe that this code is still necessary. Can you elaborate more on why you're looking to see this deleted?

Wasmtime GitHub notifications bot (Apr 01 2024 at 03:16):

liutao-liu commented on issue #8257:

The postion has been modified in fd_write(fd_read is the same):
https://github.com/bytecodealliance/wasmtime/blob/9c92881d3b936b5d2e593ded370609a6e31717d8/crates/wasi/src/preview1.rs#L1705
The implementation of fd_write is as follows: first call write_via_stream, then call blocking_write_and_flush, and finally modify the position.
The test case you added in #8271: call write_via_stream once, then call blocking_write_and_flush twice.

Wasmtime GitHub notifications bot (Apr 01 2024 at 03:26):

liutao-liu edited a comment on issue #8257:

The postion has been modified in fd_write(fd_read is the same):
https://github.com/bytecodealliance/wasmtime/blob/9c92881d3b936b5d2e593ded370609a6e31717d8/crates/wasi/src/preview1.rs#L1705
The implementation of fd_write is as follows: first call write_via_stream, then call blocking_write_and_flush, and finally modify the position.
The test case you added in #8271: call write_via_stream once, then call blocking_write_and_flush twice, so, in this case, must change the position in blocking_write_and_flush.

Wasmtime GitHub notifications bot (Apr 01 2024 at 15:07):

alexcrichton commented on issue #8257:

You'll need to remember that there's a few layers here. The test I added in #8271 specifically uses preview2-based APIs and does not use preview1. You're correct that in a world where preview2 does not exist removing the write is ok, but in a world where preview2 exists removing the write is not correct.

I've asked a few times already, but can you clarify why you'd like this write removed? Is it causing performance problems or something like that? Or are you instead trying to understand why it's needed? (whatever's fine, I'm just curious!)

Wasmtime GitHub notifications bot (Apr 02 2024 at 01:02):

liutao-liu closed issue #8257:

Two modifications to the p code：
https://github.com/bytecodealliance/wasmtime/blob/9c92881d3b936b5d2e593ded370609a6e31717d8/crates/wasi/src/filesystem.rs#L359

https://github.com/bytecodealliance/wasmtime/blob/9c92881d3b936b5d2e593ded370609a6e31717d8/crates/wasi/src/preview1.rs#L1705

The former is an invalid modification, because self.mode is not a reference of postion in fd_write , which is passed by value in write_via_stream.

Please help confirm if this is a bug. If it is, I'll submit a pr with my local changes.

Wasmtime GitHub notifications bot (Apr 02 2024 at 01:02):

liutao-liu commented on issue #8257:

You'll need to remember that there's a few layers here. The test I added in #8271 specifically uses preview2-based APIs and does not use preview1. You're correct that in a world where preview2 does not exist removing the write is ok, but in a world where preview2 exists removing the write is not correct.

I've asked a few times already, but can you clarify why you'd like this write removed? Is it causing performance problems or something like that? Or are you instead trying to understand why it's needed? (whatever's fine, I'm just curious!)

Sorry, I wasn't thinking from preview2's perspective. I see what you mean, thanks for your reply.

Last updated: Nov 22 2024 at 16:03 UTC