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

Jump to bottom

No tests/samples covering socket read()/write() calls #25407

Closed
Tracked by #51211
pfalcon opened this issue May 18, 2020 · 4 comments · Fixed by #25356
Closed
Tracked by #51211

No tests/samples covering socket read()/write() calls #25407

pfalcon opened this issue May 18, 2020 · 4 comments · Fixed by #25356
Assignees
@cfriedt
Labels
area: POSIX POSIX API Library area: Tests Issues related to a particular existing or missing test Enhancement Changes/Updates/Additions to existing features
Milestone
v3.3.0

Comments

@pfalcon
Copy link
Contributor

pfalcon commented May 18, 2020

While support for POSIX read()/write() calls was implemented for some time (enabled by CONFIG_POSIX_API option), there're no tests or samples using this feature. This can lead to regressions and must be addressed.
@pfalcon pfalcon added bug The issue is a bug, or the PR is fixing a bug area: Tests Issues related to a particular existing or missing test area: POSIX POSIX API Library labels May 18, 2020
@pfalcon pfalcon self-assigned this May 18, 2020
@pfalcon pfalcon added this to the v2.3.0 milestone May 18, 2020
pfalcon added a commit to pfalcon/zephyr that referenced this issue May 18, 2020
@pfalcon
samples: net: echo_async_select: Use read()/write() if possible
19bcc60 
If build with full POSIX API, use read()/write() instead of
recv()/send() calls for sockets.

We have read()/write() support for a while, but no samples/tests
actually performed at least a build test for it (so it will be
done now).

Fixes: zephyrproject-rtos#25407
Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
This was referenced May 18, 2020
Merged
Closed
@carlescufi carlescufi added Enhancement Changes/Updates/Additions to existing features and removed bug The issue is a bug, or the PR is fixing a bug labels May 19, 2020
@pfalcon pfalcon mentioned this issue May 20, 2020
Closed
carlescufi pushed a commit that referenced this issue May 20, 2020
@pfalcon @carlescufi
samples: net: echo_async_select: Use read()/write() if possible
8af7187 
If build with full POSIX API, use read()/write() instead of
recv()/send() calls for sockets.

We have read()/write() support for a while, but no samples/tests
actually performed at least a build test for it (so it will be
done now).

Fixes: #25407
Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
@pfalcon
Copy link
Contributor Author

pfalcon commented May 20, 2020

Reopening, as there're more work to do (#25443).

@pfalcon pfalcon reopened this May 20, 2020
krip-tip pushed a commit to krip-tip/zephyr-local that referenced this issue May 30, 2020
@pfalcon
samples: net: echo_async_select: Use read()/write() if possible
00d5d7b 
If build with full POSIX API, use read()/write() instead of
recv()/send() calls for sockets.

We have read()/write() support for a while, but no samples/tests
actually performed at least a build test for it (so it will be
done now).

Fixes: zephyrproject-rtos#25407
Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
@carlescufi carlescufi modified the milestones: v2.3.0, v2.4.0 Jun 5, 2020
pfalcon added a commit to pfalcon/zephyr that referenced this issue Jun 11, 2020
@pfalcon
lib: os: fdtable: Add syscalls for POSIX read/write/close operations
c2bc3fd 
Fdtable is a kernel-level structure, so functions accessing it, like
read(), write(), close(), should be syscalls. This conversion however
emphasizes a difference between "syscall" and "C standard library
function". C stdlib functions should be represented by just a
standard linkage symbol, and can be used even without prototype.
Syscall is however a special code sequence, which may require special
attributes and thus a prototype for usage. To deal with these
distinction, we define a convention that "sys_name" is a syscall,
while "name" is a normal C function (implemented as a wrapper around
syscall). (And we also need to define "_name" to interface with Newlib
(actually, just "_write".))

Fixes: zephyrproject-rtos#25407

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
pfalcon added a commit to pfalcon/zephyr that referenced this issue Jun 11, 2020
@pfalcon
lib: os: fdtable: Add syscalls for POSIX read/write/close operations
1cd0ae8 
Fdtable is a kernel-level structure, so functions accessing it, like
read(), write(), close(), should be syscalls. This conversion however
emphasizes a difference between "syscall" and "C standard library
function". C stdlib functions should be represented by just a
standard linkage symbol, and can be used even without prototype.
Syscall is however a special code sequence, which may require special
attributes and thus a prototype for usage. To deal with these
distinction, we define a convention that "sys_name" is a syscall,
while "name" is a normal C function (implemented as a wrapper around
syscall). (And we also need to define "_name" to interface with Newlib
(actually, just "_write".))

Fixes: zephyrproject-rtos#25407

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
@cfriedt cfriedt modified the milestones: v2.4.0, v3.3.0 Nov 22, 2022
@cfriedt cfriedt mentioned this issue Nov 22, 2022
Closed
@cfriedt
Copy link
Member

cfriedt commented Nov 22, 2022

I'll add a sample demonstrating read / write functionality. Thanks for the suggestion!

@cfriedt
Copy link
Member

cfriedt commented Jan 24, 2023

This ticket actually goes far beyond simple examples (and IIRC, we do have examples of read() / write() now).

It raises the larger question.

Do POSIX calls need to be system calls or is POSIX just a library?

There is no kernel object associated with the global file descriptor table, so right now it's use is "free".

Additionally, there are no system-call level checks (Z_OOPS()) for reading or writing outside of the permitted memory region for the current thread (CONFIG_USERSPACE).

That means, for systems with an MMU / MPU we'll just get a fault, but then for systems without an MMU / MPU, it will result in random corruption.

However, that reflects the same behaviour for libraries today.

I'm tempted to close this -EWONTFIX, but I know that may be a contentious topic, so I'll mark it for review in the Architecture meeting.

@cfriedt cfriedt assigned cfriedt and unassigned pfalcon Feb 5, 2023
@cfriedt
Copy link
Member

cfriedt commented Feb 5, 2023

The Arch meeting was cancelled on the day this was supposed to be reviewed, so I'll just jot down some notes:

I've converted the socketpair example to use read / write as well. Will close this issue.

However, I think there are some valid concerns with Zephyr's fdtable.c, but at the same time, it's not clear if Zephyr is really the place to focus on this sort of thing.

  • the global collection of file descriptors is, well.. global
  • there is no separation between kernel, userspace, or even other threads in terms of file descriptor use
  • indeed, there is no file descriptor kernel object (such that permissions would be granted, etc)

This relates back to how file descriptor tables operate on POSIX systems; each process has its own file descriptor table (they are just integers) that refer to kernel file descriptor objects, a specific set of file operations.

That's a pretty heavyweight design though.

Some might argue that Zephyr's view of running in a single address space / process space is fine for its intended purposes.

Going to close this as done for now, but we can always reopen the topic in an architecture meeting.

@cfriedt cfriedt closed this as completed Feb 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cfriedt cfriedt

Labels
area: POSIX POSIX API Library area: Tests Issues related to a particular existing or missing test Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Milestone
v3.3.0
3 participants
@pfalcon @cfriedt @carlescufi