Skip to content
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.

Sign up for GitHub

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

Read stdin initialized file #8

Open
wants to merge 1 commit into
base: dramfs
Choose a base branch
from
Open

Read stdin initialized file #8

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 32 additions & 27 deletions libgloss/dramfs/sys_read.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,35 +6,40 @@
/* Read from a file. */
ssize_t _read(int fd, void *ptr, size_t len)
{
if(dramfs_check_fd(fd) < 0) {
return -1;
if (dramfs_check_fd(fd) < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a semantic change. If check_fd < 0, we used to return -1. What's the difference in behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If no stdin was created in the lfs.c file(we're building as baremetal), check_fd is going to return -1, but we might want to read the host getchar.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm think I'm missing something here. The MKLFS tool doesn't always define the STDIN/STDOUT to exist in lfs.c?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MKLFS always will create an stdin in lfs.c. If we're using BAREMETAL perch then we're not using any lfs.c files so there's no stdin file descriptor.

Copy link
Contributor

@dpetrisko dpetrisko Jun 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, so:

  • if we do have an LFS.c, there is no change
  • If we do not have an LFS.c:
  • previously, we would return -1
  • now, we attempt to do host_getchar regardless?

Copy link
Contributor Author

@farzamgl farzamgl Jun 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Not exactly. If there is an lfs.c, we read the stdin file before doing the host_getchar. If not, we would only do host_getchar. Before this change, we only did host_getchar if there was a stdin file and we didn't actually read the file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I'm just talking about this if statement right now. If we don't call dramfs_init, will this go to host read or fail? Can we call dramfs_init without an LFS? I'm trying to nail down does this change fully enable users to use stdin/stdout without messing with lfs or will something down the line fail because of not having LFS or compiling with BAREMETAL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't call dramfs_init this will go to host_getchar. We cannot call the non-dummy dramfs_init without an lfs.c, for this reqason there's a default lfs.c with just the stdin/out/err in the repo. This if statement convers the BAREMETAL case. The other if statment after lfs_file_read covers the default lfs.c case.

if(fd == 0)
goto host_getchar;
else
return -1;
}

if(fd == 0) {
uint8_t *data = (uint8_t *)ptr;

// Return early on len == 0
if (len == 0) return (ssize_t) 0;

int ch;
if (dramfs_nonblock_fd(fd) == 0) {
// Block to read just 1 character to start
while ((ch = dramfs_getchar()) == -1);
} else {
// Read the first character, and return immediately if it's EOF
if ((ch = dramfs_getchar()) == -1) return (ssize_t) 0;
}

// Keep reading until new
int i = 0;
do {
data[i++] = ch;
if (i == len) break;
} while ((ch = dramfs_getchar()) != -1);

return (ssize_t) i;
lfs_file_t *fptr = dramfs_get_file(fd);
ssize_t size = (ssize_t) lfs_file_read(&dramfs_fs, fptr, ptr, (lfs_size_t) len);
Copy link
Contributor

Choose a reason for hiding this comment

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

lfs_file_read will always return 0 for stdin and have no side effects? (an actual read?)


if ((fd != 0) || (size != 0))
return size;

host_getchar:
uint8_t *data = (uint8_t *)ptr;

// Return early on len == 0
if (len == 0) return (ssize_t) 0;

int ch;
if (dramfs_nonblock_fd(fd) == 0) {
// Block to read just 1 character to start
while ((ch = dramfs_getchar()) == -1);
} else {
// Read the first character, and return immediately if it's EOF
if ((ch = dramfs_getchar()) == -1) return (ssize_t) 0;
}

lfs_file_t *fptr = dramfs_get_file(fd);
return (ssize_t) lfs_file_read(&dramfs_fs, fptr, ptr, (lfs_size_t) len);
// Keep reading until new
int i = 0;
do {
data[i++] = ch;
if (i == len) break;
} while ((ch = dramfs_getchar()) != -1);

return (ssize_t) i;
}