-
Notifications
You must be signed in to change notification settings - Fork 26
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
Added support for reading from stdin. #6
Conversation
Why the memfs binary is smaller than original 🤔 |
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.
A few comments, otherwise looks pretty good!
Also, can you remove memfs
changes here from the PR?
// write buf back | ||
size += lenToWrite; | ||
this.stdinStrPos += lenToWrite; | ||
if(lenToWrite !== len){ |
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'm not sure if this is correct behavior; I think the rest of the reads should still go through, even if the previous one fails.
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.
when the lenToWrite
is not equivalent with the len
, it means that there is nothing more to read, we reached the EOF of the stdin file. I wrote break
to speed things up, but when reading from regular files as you can see here, it will return some random data when reached EOF? I'm not sure about this.
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.
Checked the memfs.c
again, I still can't figure out how the procedure of reading from a regular file works. These lines seems will write the whole file to the buf at once even the buf is smaller than the file? Have you tested this on use cases like reading large files?
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.
No you're right. These functions are meant to work like linux readv
. The confusion here is the difference between readv
and preadv
I think. ReadIovec
assumes that the offset is fixed (like preadv
), so it won't work properly if there are multiple buffers on a readv
call.
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.
So should we break here? In my opinion, we should, because we reached the end of the input string, continue reading seems useless.
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.
Yes, I think so, at least for now. Ultimately I think we may want a separate call, so preadv
and readv
can be handled separately when interacting with JS. But if most applications are treating stdin
as stream-like, then updating the offset and stopping when there's no more input seems like the right choice.
Should I make a |
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 just realized you will need to update memfs anyway; host_read
isn't defined there, so you'll need to call out from memfs to the host in the same way host_write
is implemented.
// write buf back | ||
size += lenToWrite; | ||
this.stdinStrPos += lenToWrite; | ||
if(lenToWrite !== len){ |
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.
No you're right. These functions are meant to work like linux readv
. The confusion here is the difference between readv
and preadv
I think. ReadIovec
assumes that the offset is fixed (like preadv
), so it won't work properly if there are multiple buffers on a readv
call.
Also created a PR for your fork of llvm contains the changes of memfs.c. |
Another question is that the js file (shared.js) requires the binary mapping of the iovs, such as the first 4 bytes is the |
I'd guess that there would be a lot more changes required for a different WASI version, so I'm not sure it's worth abstracting it for just this one case. |
Ready for final review and merge. |
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, lgtm!
How can I make this work in the current version without the use of any server? Because the example C code below inside the editor component doesn't prompt xterm for some stdin: #include <stdio.h>
int main() {
int num;
printf("Enter a number: ");
scanf("%d", &num);
printf("The number you entered is %d\n", num);
return 0;
} |
Added support for reading from stdin.
As described in this issue.
I only added functionality to read, didn't add an interface in HTML to specify the input. You can change the default
MemFS.stdinStr
to test this patch.