Wasip3: File handing implementation#720
Conversation
cpetig
force-pushed
the
p3_file
branch
3 times, most recently
from
30756b3 to
599aafe
Compare
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Overall looks good to me, thanks again for working on this! I've left comments here and there, but nothing major.
| #elif defined(__wasip3__) | ||
| filesystem_directory_entry_t dir_entry; | ||
| wasip3_waitable_status_t status = filesystem_stream_directory_entry_read(dirp->stream.f0, &dir_entry, 1); | ||
| ssize_t amount = wasip3_waitable_block_on(status, dirp->stream.f0); |
| filesystem_result_metadata_hash_value_error_code_t result; | ||
| status = filesystem_method_descriptor_metadata_hash_at(dir_handle, path_flags, dir_entry.name, &result); | ||
| wasip3_subtask_block_on(status); | ||
| //wasip3_string_free(&dir_entry.name); |
There was a problem hiding this comment.
Debugging perhaps? (I think this is needed right?)
| if (amount<1) | ||
| return NULL; |
There was a problem hiding this comment.
Upon reaching end-of-stream this'll want to block on the future-of-result to see if an error is going to be returned
|
|
||
| // Ensure that the dirent is large enough to fit the filename | ||
| size_t the_size = offsetof(struct dirent, d_name); | ||
| GROW(dirp->dirent, dirp->dirent_size, the_size + dir_entry.name.len + 1); |
There was a problem hiding this comment.
This is preexisting, but I'm realizing now that this GROW can leak memory because it can return-early on OOM which would leak dir_entry which has an owned string internally.
I'll file an issue to fix that.
| if (dirp->stream.f0 != 0) { | ||
| filesystem_stream_directory_entry_drop_readable(dirp->stream.f0); | ||
| dirp->stream.f0 = 0; | ||
| } |
There was a problem hiding this comment.
I think this is actually a bug of the wasip2 implementation where it doesn't close the previous handle on a call to seekdir. Could you update the wasip2 block above as well? Unfortunately hard to add a test for...
| if (dirp->stream.f0 != 0) { | ||
| filesystem_stream_directory_entry_drop_readable(dirp->stream.f0); | ||
| dirp->stream.f0 = 0; | ||
| } |
There was a problem hiding this comment.
Like below could this update the wasip2 branch above too to close the previous handle if present?
| * Returns the byte-length of the string on success, or -1 if it's invalid | ||
| * utf-8. | ||
| */ | ||
| static int validate_utf8(const char *ptr_signed) { |
There was a problem hiding this comment.
Can this be deduplicated with the wasip2 equivalent?
| static int file_set_blocking(void *data, bool blocking) { abort(); } | ||
|
|
||
| static int file_fcntl_getfl(void *data) { abort(); } | ||
|
|
||
| static int file_fcntl_setfl(void *data, int flags) { abort(); } |
There was a problem hiding this comment.
Mind tagging these abort() calls with // TODO(wasip3) so we can grep for it later?
| wasip3_write_t write; | ||
| } file3_t; | ||
|
|
||
| static void file_close_streams(void *data) { |
There was a problem hiding this comment.
Since this is purely internal could this take a file3_t *data argument instead of void *data?
| if (file->write.output != 0) { | ||
| filesystem_stream_u8_drop_writable(file->write.output); | ||
| file->write.output = 0; | ||
| wasip3_subtask_block_on(file->write.subtask); | ||
| file->write.subtask = WASIP3_SUBTASK_RETURNED; | ||
| if (file->write.pending_result.is_err) { | ||
| translate_error(file->write.pending_result.val.err); | ||
| } | ||
| } |
There was a problem hiding this comment.
Since this is in a shared wasip3_write_t, should this perhaps be a shared function on wasip3_write_t?
Also I'm a bit wary about the stray translate_error call here when this function is otherwise not returning anything. That makes it non-obvious that errno is set as a side effect and can also cause the errno set here to get lost by accident elsewhere. For example close(some_fd) might return 0 but errno would be set, losing the error.
I think that this function and/or helper may want to avoid translate_error and leave that up to the caller? This would return the code though and callers would be audited if they want to handle the error (which for most I think the answer is "no" and they want to ignore the error?)
2 participants
This builds on #718 and thus should remain draft until the other one is merged.This only implements a subset of the features, but it is nicely self-contained and makes six additional tests pass.
If we want to keep the individual PRs small/incremental this is a good start.