-
Notifications
You must be signed in to change notification settings - Fork 267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wasi: Implement random_get
function
#241
Conversation
Signed-off-by: Constantine Kryvomaz <ckryvomaz@gmail.com>
Signed-off-by: Constantine Kryvomaz <ckryvomaz@gmail.com>
Signed-off-by: r8d8 <ckryvomaz@gmail.com> Signed-off-by: Constantine Kryvomaz <ckryvomaz@gmail.com>
* Remove random generator from WASM structure. * Adjust formatting and naming. Signed-off-by: r8d8 <ckryvomaz@gmail.com> Signed-off-by: Constantine Kryvomaz <ckryvomaz@gmail.com>
Use pseudo-random source in tests. Signed-off-by: r8d8 <ckryvomaz@gmail.com> Signed-off-by: Constantine Kryvomaz <ckryvomaz@gmail.com>
Signed-off-by: Constantine Kryvomaz <ckryvomaz@gmail.com>
wasi/wasi.go
Outdated
random_bytes := make([]byte, bufLen) | ||
_, err := a.randSource.Read(random_bytes) | ||
if err != nil { | ||
return ErrnoInval |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ErronoInval is for invalid arguments right? Is this the correct error for here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend take a look at spec and also WASI-libc and see how other impl(e.g. Wasm time) handle errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Wasmer return ErrnoIO for this case
https://github.com/wasmerio/wasmer/blob/53c2c29bed1cf1fee8c5616632ee8c2bccc27880/lib/wasi/src/syscalls/mod.rs#L2541 -
wasi-libc just cast error fron syscal, and it can return bunch of them
https://man7.org/linux/man-pages/man2/getrandom.2.html -
wasmtime also maps syscal error into Errno
https://github.com/bytecodealliance/wasmtime/blob/853a025613e012c6c29002ddcccfced67073a8d0/crates/wasi-common/src/snapshots/preview_1.rs#L111
Used ErrnoIO
, added TODO in source
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI for bad memory offsets, we discussed offline to use ErrnoFault. Main thing is to document what is used and be consistent even if no one else is :D
Later, people can argue validly with these choices and probably we need to raise an issue on the wasi spec as it is underspecified. Main thing now is to document what you do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codefromthecrypt thanks for info.
So, for invalid buf
, bufLen
return error is ErrnoFault
. Am I getting right?
And how to deal with errors from random source ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ErrnoIO for the source is fine for now. we can change it easily later if we find a most standard code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the issue that has been punted quite a while so far WebAssembly/WASI#215
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error codes stuff gets you confused even more in functions that a more compilated than random_get
. Spec need to address it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totally agree. appreciate if you can make your case on the spec issue because it was 2 years already so somehow they aren't prioritizing. maybe if more scream they will 🤷
Signed-off-by: Constantine Kryvomaz <ckryvomaz@gmail.com>
Remove random source interface, use functor instead Signed-off-by: Constantine Kryvomaz <ckryvomaz@gmail.com>
Signed-off-by: Constantine Kryvomaz <ckryvomaz@gmail.com>
Signed-off-by: Constantine Kryvomaz <ckryvomaz@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nearly there!
drift alert and apologies for it (again blush) #246 if you need help resolving conflicts ask and I can do that for you. |
Add test case for random source error Signed-off-by: Constantine Kryvomaz <ckryvomaz@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great work.
Signed-off-by: Constantine Kryvomaz <ckryvomaz@gmail.com>
Thanks for patience @codefromthecrypt @mathetake |
Thank you for you contribution! 💯🙌🏄♂️ @r8d8 |
ps if you are using something besides tinygo and/or need a WASI API not defined here, please pipe up in the issue #271 |
Fixed sign-off from original PR #237