Stream: git-wasmtime

Topic: wasmtime / PR #1395 [wasi-common] Clean up the use of mut...

Wasmtime GitHub notifications bot (Mar 24 2020 at 20:35):

kubkon edited PR #1395 from mut-cleanup to master:

Until now, several syscalls including fd_pwrite etc. were relying on mutating &mut Entry by mutating its inner file handle. This is unnecessary in almost all cases since all methods mutating std::fs::File in Rust's libstd are also implemented for &std::fs::File. In part, this will prepare us to handle Entrys behind an Rc and RefCell combo.

While here, I've also modified OsHandle in BSD to include RefCell<Option<Dir>> rather than Option<Mutex<Dir>> as was until now. While RefCell could easily be replaced with Mutex, since going multithreading will require a lot of conceptual changes to wasi-common, I thought it'd be best not to mix single- with multithreading contexts and swap all places at once when it comes to it. If y'all feel this is not the right approach, lemme know!

I've also had to make some modifications to virtual FS which mainly swaps mutability for interior mutability in a handful of places.

Wasmtime GitHub notifications bot (Mar 24 2020 at 20:55):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 20:55):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 20:55):

sunfishcode created PR Review Comment:

What is the mut here for?

Wasmtime GitHub notifications bot (Mar 24 2020 at 21:20):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 21:20):

alexcrichton created PR Review Comment:

The way this technically works is it's dispatching to the impl Seek for &File where the seek method takes &mut self. This means that we're calling seek on &mut &File, so the mut here is needed to create the &mut &File binding.

Wasmtime GitHub notifications bot (Mar 24 2020 at 21:21):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 21:21):

kubkon created PR Review Comment:

This is to be able to mutate the reference to reference to File. Another way of expressing it would be

(&mut (&*fd as &File)).read_vectored(...)

This looks too cluttered for me, hence why I’ve decided to let-bind it first. However, if you know of a better way of handling this, lemme know!

Wasmtime GitHub notifications bot (Mar 24 2020 at 21:21):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 21:21):

alexcrichton created PR Review Comment:

For one-liners like this it should suffice to do (&fd).seek(pos)

Wasmtime GitHub notifications bot (Mar 24 2020 at 21:22):

kubkon created PR Review Comment:

@alexcrichton again beat me to it! Seconds, seconds!

Wasmtime GitHub notifications bot (Mar 24 2020 at 21:22):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 21:23):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 21:25):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 21:25):

kubkon created PR Review Comment:

I tried but alas wouldn’t work. The compiler complained that cannot mutate File behind a & ref. I think this might be since fd is not a File but rather an entity that implements Deref<Target=File> so would need something like (&mut (&*fd as &File)). Unless I’m doing something wrong here.

Wasmtime GitHub notifications bot (Mar 25 2020 at 09:14):

kubkon updated PR #1395 from mut-cleanup to master:

Until now, several syscalls including fd_pwrite etc. were relying on mutating &mut Entry by mutating its inner file handle. This is unnecessary in almost all cases since all methods mutating std::fs::File in Rust's libstd are also implemented for &std::fs::File. In part, this will prepare us to handle Entrys behind an Rc and RefCell combo.

While here, I've also modified OsHandle in BSD to include RefCell<Option<Dir>> rather than Option<Mutex<Dir>> as was until now. While RefCell could easily be replaced with Mutex, since going multithreading will require a lot of conceptual changes to wasi-common, I thought it'd be best not to mix single- with multithreading contexts and swap all places at once when it comes to it. If y'all feel this is not the right approach, lemme know!

I've also had to make some modifications to virtual FS which mainly swaps mutability for interior mutability in a handful of places.

Wasmtime GitHub notifications bot (Mar 25 2020 at 09:16):

kubkon created PR Review Comment:

Ok, I've convinced myself that this is due to the fact that fd is an OsHandle and not a File nor &File. Hence, we need to force it to coerce to &File before calling the trait method such as seek on it. The minimum working one-liner I got down to is (fd as &File).seek(pos)?. I'm happy to leave several as one-liners such as this one, unless we re-use the fd in a couple of places within the same scope, in which case let mut fd: &File = fd; seems more natural.

Wasmtime GitHub notifications bot (Mar 25 2020 at 09:16):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 25 2020 at 13:00):

kubkon merged PR #1395.

Last updated: Oct 23 2024 at 20:03 UTC