Stream: git-wasmtime

Topic: wasmtime / PR #2160 wasi-common: factor out common string...

Wasmtime GitHub notifications bot (Aug 24 2020 at 23:47):

pchickey opened PR #2160 from pch/wasi_common_array_writer to main:

The args and environ methods duplicated code, so I factored it into a trait impled on Vec<CString>. It could easily be impled for Vec<String> if we wanted to, but there doesn't seem to be much reason to refactor it further now.

As part of the factoring I added slice accessors get and get_range to wiggle::GuestPtr<[T]>, which seem useful.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Aug 24 2020 at 23:47):

pchickey requested iximeow for a review on PR #2160.

Wasmtime GitHub notifications bot (Aug 24 2020 at 23:48):

pchickey edited PR #2160 from pch/wasi_common_array_writer to main:

The args and environ methods duplicated code, so I factored it into a trait impled on Vec<CString>. It could easily be impled for Vec<String> if we wanted to, but there doesn't seem to be much reason to refactor it further now.

As part of the factoring I added slice accessors get and get_range to wiggle::GuestPtr<[T]>, which seem useful.

My current goal is to make snapshots/wasi_snapshot_preview1.rs as uninteresting as possible. This was one small chunk that could be moved out.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Aug 24 2020 at 23:49):

pchickey edited PR #2160 from pch/wasi_common_array_writer to main:

The args and environ methods duplicated code, so I factored it into a trait impled on Vec<CString>. It could easily be impled for Vec<String> if we wanted to, but there doesn't seem to be much reason to refactor it further now.

As part of the factoring I added slice accessors get and get_range to wiggle::GuestPtr<[T]>, which seem useful.

My current goal is to make snapshots/wasi_snapshot_preview1.rs as uninteresting as possible. This was one small chunk that could be moved out. We expect this code to work essentially the same across all snapshots, until interface types change the way we write arrays of strings into the guest.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Aug 25 2020 at 00:07):

pchickey edited PR #2160 from pch/wasi_common_array_writer to main:

The args and environ methods duplicated code, so I factored it into a trait impled on Vec<CString>. It could easily be impled for Vec<String> if we wanted to, but there doesn't seem to be much reason to refactor it further now.

As part of the factoring I added slice accessors get and get_range to wiggle::GuestPtr<[T]>, which seem useful.

My current goal is to make snapshots/wasi_snapshot_preview1.rs as uninteresting as possible. This was one small chunk that could be moved out. We expect this code to work essentially the same across all snapshots, until interface types change the way we write arrays of strings into the guest.

Based on top of #2140 and #2139
<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Aug 25 2020 at 22:36):

pchickey updated PR #2160 from pch/wasi_common_array_writer to main:

The args and environ methods duplicated code, so I factored it into a trait impled on Vec<CString>. It could easily be impled for Vec<String> if we wanted to, but there doesn't seem to be much reason to refactor it further now.

As part of the factoring I added slice accessors get and get_range to wiggle::GuestPtr<[T]>, which seem useful.

My current goal is to make snapshots/wasi_snapshot_preview1.rs as uninteresting as possible. This was one small chunk that could be moved out. We expect this code to work essentially the same across all snapshots, until interface types change the way we write arrays of strings into the guest.

Based on top of #2140 and #2139
<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Sep 01 2020 at 20:56):

pchickey updated PR #2160 from pch/wasi_common_array_writer to main:

The args and environ methods duplicated code, so I factored it into a trait impled on Vec<CString>. It could easily be impled for Vec<String> if we wanted to, but there doesn't seem to be much reason to refactor it further now.

As part of the factoring I added slice accessors get and get_range to wiggle::GuestPtr<[T]>, which seem useful.

My current goal is to make snapshots/wasi_snapshot_preview1.rs as uninteresting as possible. This was one small chunk that could be moved out. We expect this code to work essentially the same across all snapshots, until interface types change the way we write arrays of strings into the guest.

Based on top of #2140 and #2139
<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Sep 01 2020 at 21:00):

pchickey has marked PR #2160 as ready for review.

Wasmtime GitHub notifications bot (Sep 02 2020 at 00:53):

pchickey updated PR #2160 from pch/wasi_common_array_writer to main:

The args and environ methods duplicated code, so I factored it into a trait impled on Vec<CString>. It could easily be impled for Vec<String> if we wanted to, but there doesn't seem to be much reason to refactor it further now.

As part of the factoring I added slice accessors get and get_range to wiggle::GuestPtr<[T]>, which seem useful.

My current goal is to make snapshots/wasi_snapshot_preview1.rs as uninteresting as possible. This was one small chunk that could be moved out. We expect this code to work essentially the same across all snapshots, until interface types change the way we write arrays of strings into the guest.

Based on top of #2140 and #2139
<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Sep 02 2020 at 17:55):

pchickey updated PR #2160 from pch/wasi_common_array_writer to main:

The args and environ methods duplicated code, so I factored it into a trait impled on Vec<CString>. It could easily be impled for Vec<String> if we wanted to, but there doesn't seem to be much reason to refactor it further now.

As part of the factoring I added slice accessors get and get_range to wiggle::GuestPtr<[T]>, which seem useful.

My current goal is to make snapshots/wasi_snapshot_preview1.rs as uninteresting as possible. This was one small chunk that could be moved out. We expect this code to work essentially the same across all snapshots, until interface types change the way we write arrays of strings into the guest.

Based on top of #2140 and #2139
<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Sep 02 2020 at 19:48):

pchickey updated PR #2160 from pch/wasi_common_array_writer to main:

The args and environ methods duplicated code, so I factored it into a trait impled on Vec<CString>. It could easily be impled for Vec<String> if we wanted to, but there doesn't seem to be much reason to refactor it further now.

As part of the factoring I added slice accessors get and get_range to wiggle::GuestPtr<[T]>, which seem useful.

My current goal is to make snapshots/wasi_snapshot_preview1.rs as uninteresting as possible. This was one small chunk that could be moved out. We expect this code to work essentially the same across all snapshots, until interface types change the way we write arrays of strings into the guest.

Based on top of #2140 and #2139
<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Sep 02 2020 at 20:09):

pchickey edited PR #2160 from pch/wasi_common_array_writer to main:

The args and environ methods duplicated code, so I factored it into a trait impled on Vec<CString>. It could easily be impled for Vec<String> if we wanted to, but there doesn't seem to be much reason to refactor it further now.

As part of the factoring I added slice accessors get and get_range to wiggle::GuestPtr<[T]>, which seem useful.

My current goal is to make snapshots/wasi_snapshot_preview1.rs as uninteresting as possible. This was one small chunk that could be moved out. We expect this code to work essentially the same across all snapshots, until interface types change the way we write arrays of strings into the guest.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Sep 02 2020 at 20:13):

pchickey updated PR #2160 from pch/wasi_common_array_writer to main:

The args and environ methods duplicated code, so I factored it into a trait impled on Vec<CString>. It could easily be impled for Vec<String> if we wanted to, but there doesn't seem to be much reason to refactor it further now.

As part of the factoring I added slice accessors get and get_range to wiggle::GuestPtr<[T]>, which seem useful.

My current goal is to make snapshots/wasi_snapshot_preview1.rs as uninteresting as possible. This was one small chunk that could be moved out. We expect this code to work essentially the same across all snapshots, until interface types change the way we write arrays of strings into the guest.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Sep 02 2020 at 20:18):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Sep 02 2020 at 20:18):

alexcrichton created PR Review Comment:

Should this be index times the gues size of T?

Wasmtime GitHub notifications bot (Sep 02 2020 at 20:18):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Sep 02 2020 at 20:18):

alexcrichton created PR Review Comment:

Same question as above with get

Wasmtime GitHub notifications bot (Sep 02 2020 at 20:18):

alexcrichton created PR Review Comment:

I think this may also want to include the condition that r.start <= r.end?

Wasmtime GitHub notifications bot (Sep 02 2020 at 20:18):

alexcrichton created PR Review Comment:

How come this is a trait? Could this perhaps be a free-function in a utils-like module of some kind?

Wasmtime GitHub notifications bot (Sep 02 2020 at 20:18):

pchickey updated PR #2160 from pch/wasi_common_array_writer to main:

The args and environ methods duplicated code, so I factored it into a trait impled on Vec<CString>. It could easily be impled for Vec<String> if we wanted to, but there doesn't seem to be much reason to refactor it further now.

As part of the factoring I added slice accessors get and get_range to wiggle::GuestPtr<[T]>, which seem useful.

My current goal is to make snapshots/wasi_snapshot_preview1.rs as uninteresting as possible. This was one small chunk that could be moved out. We expect this code to work essentially the same across all snapshots, until interface types change the way we write arrays of strings into the guest.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Sep 02 2020 at 20:30):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Sep 02 2020 at 20:30):

pchickey created PR Review Comment:

My thought was we would eventually support different representations than Vec<CString> here as part of modularization. It seemed like a reasonable way to hang three methods onto some existing struct members. But maybe instead of WasiCtx { args: Vec<CString>, env: Vec<CString> } we could make this struct StringArray(Vec<String>) that just has these three pub methods?

Wasmtime GitHub notifications bot (Sep 02 2020 at 20:31):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Sep 02 2020 at 20:31):

pchickey created PR Review Comment:

I was under the mistaken impression that this was guaranteed by Range construction, I'll correct that

Wasmtime GitHub notifications bot (Sep 02 2020 at 20:39):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Sep 02 2020 at 20:39):

pchickey created PR Review Comment:

len() is in units (correcting this to elements in the doc strings, not bytes, so I dont think we need to multiply by the size.

Wasmtime GitHub notifications bot (Sep 02 2020 at 20:39):

pchickey updated PR #2160 from pch/wasi_common_array_writer to main:

The args and environ methods duplicated code, so I factored it into a trait impled on Vec<CString>. It could easily be impled for Vec<String> if we wanted to, but there doesn't seem to be much reason to refactor it further now.

As part of the factoring I added slice accessors get and get_range to wiggle::GuestPtr<[T]>, which seem useful.

My current goal is to make snapshots/wasi_snapshot_preview1.rs as uninteresting as possible. This was one small chunk that could be moved out. We expect this code to work essentially the same across all snapshots, until interface types change the way we write arrays of strings into the guest.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Sep 02 2020 at 21:12):

pchickey updated PR #2160 from pch/wasi_common_array_writer to main:

The args and environ methods duplicated code, so I factored it into a trait impled on Vec<CString>. It could easily be impled for Vec<String> if we wanted to, but there doesn't seem to be much reason to refactor it further now.

As part of the factoring I added slice accessors get and get_range to wiggle::GuestPtr<[T]>, which seem useful.

My current goal is to make snapshots/wasi_snapshot_preview1.rs as uninteresting as possible. This was one small chunk that could be moved out. We expect this code to work essentially the same across all snapshots, until interface types change the way we write arrays of strings into the guest.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Sep 03 2020 at 00:17):

pchickey updated PR #2160 from pch/wasi_common_array_writer to main:

The args and environ methods duplicated code, so I factored it into a trait impled on Vec<CString>. It could easily be impled for Vec<String> if we wanted to, but there doesn't seem to be much reason to refactor it further now.

As part of the factoring I added slice accessors get and get_range to wiggle::GuestPtr<[T]>, which seem useful.

My current goal is to make snapshots/wasi_snapshot_preview1.rs as uninteresting as possible. This was one small chunk that could be moved out. We expect this code to work essentially the same across all snapshots, until interface types change the way we write arrays of strings into the guest.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Sep 03 2020 at 00:33):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Sep 03 2020 at 00:33):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Sep 03 2020 at 00:33):

alexcrichton created PR Review Comment:

Yeah the length is in units but the GuestPtr returned here I think is offset in bytes from the memory base?

Wasmtime GitHub notifications bot (Sep 03 2020 at 18:27):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Sep 03 2020 at 18:27):

pchickey created PR Review Comment:

Thanks, I completely missed that. Clearly I'm not smart enough to write or maintain this code without tests, and since the only ones i have currently are in wasi-common and use GuestPtr<[u8]>, ill add some others to the wiggle tree that use non-byte-sized elements and make sure its done right.

Wasmtime GitHub notifications bot (Sep 03 2020 at 23:41):

pchickey updated PR #2160 from pch/wasi_common_array_writer to main:

The args and environ methods duplicated code, so I factored it into a trait impled on Vec<CString>. It could easily be impled for Vec<String> if we wanted to, but there doesn't seem to be much reason to refactor it further now.

As part of the factoring I added slice accessors get and get_range to wiggle::GuestPtr<[T]>, which seem useful.

My current goal is to make snapshots/wasi_snapshot_preview1.rs as uninteresting as possible. This was one small chunk that could be moved out. We expect this code to work essentially the same across all snapshots, until interface types change the way we write arrays of strings into the guest.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Sep 04 2020 at 01:01):

pchickey merged PR #2160.

Last updated: Dec 23 2024 at 12:05 UTC