Skip to content

Add support for WASI sockets to C API#5624

Merged
peterhuene merged 3 commits into
bytecodealliance:mainfrom
font:sockets-c-api
Feb 10, 2023
Merged

Add support for WASI sockets to C API#5624
peterhuene merged 3 commits into
bytecodealliance:mainfrom
font:sockets-c-api

Conversation

@font
Copy link
Copy Markdown
Contributor

@font font commented Jan 23, 2023

This adds support for WASI sockets to the C API by adding a new API to handle preopening sockets for clients. This uses HashMap instead of Vec for preopened sockets to identify if the caller has called in more than once with the same FD number. If so, then we return false so caller is at least given a hint that they may be misusing the API e.g. attempting to overwrite an already existing socket FD.

I stuck with the same bool return value because it was already being done for the other APIs e.g. wasi_config_preopen_dir. I think it would be beneficial to have a more specific error type, if possible, but not something to address in this PR.

I don't think there are any tests for these APIs so none were added. Of course I was personally testing this with the crun container runtime that is using these APIs.

I don't know who could review this so any help identifying the right people would be greatly appreciated!

Fixes #5071

@github-actions github-actions Bot added the wasmtime:c-api Issues pertaining to the C API. label Jan 23, 2023
@github-actions
Copy link
Copy Markdown

Subscribe to Label Action

cc @peterhuene

Details This issue or pull request has been labeled: "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Add support for WASI sockets in the C API by adding a new API to handle
preopening sockets for clients. This uses HashMap instead of Vec for
preopened sockets to identify if caller has called in more than once
with the same FD number. If so, then we return false so caller is given
hint that they are attempting to overwrite an already existing socket
FD.
@font
Copy link
Copy Markdown
Contributor Author

font commented Jan 27, 2023

@peterhuene Do you think you would be able to help with this review?

Copy link
Copy Markdown
Member

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @font! Thanks for the PR. The changes look good, just some minor feedback comments to go over.

Comment thread crates/c-api/include/wasi.h Outdated
Comment thread crates/c-api/include/wasi.h Outdated
Comment thread crates/c-api/src/wasi.rs Outdated
Comment thread crates/c-api/src/wasi.rs Outdated
Comment thread crates/c-api/src/wasi.rs Outdated
Comment thread crates/c-api/src/wasi.rs Outdated
Comment thread crates/c-api/src/wasi.rs Outdated
font and others added 2 commits February 2, 2023 17:40
Co-authored-by: Peter Huene <peter@huene.dev>
Copy link
Copy Markdown
Contributor Author

@font font left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peterhuene thank you for your review! I've incorporated all of your feedback in new commits that should be squashed and this is now ready for another review.

Comment thread crates/c-api/src/wasi.rs Outdated
@font
Copy link
Copy Markdown
Contributor Author

font commented Feb 9, 2023

@peterhuene Do you think you would be able to help re-review?

@peterhuene
Copy link
Copy Markdown
Member

Shoot, sorry @font, this fell off my radar. I'll review it shortly.

Copy link
Copy Markdown
Member

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks for implementing this!

@peterhuene peterhuene enabled auto-merge (squash) February 9, 2023 23:56
@peterhuene peterhuene merged commit de68cc1 into bytecodealliance:main Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

wasmtime:c-api Issues pertaining to the C API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add WASI sockets support to the C API

2 participants