cpu/native: make use of stdio_read() / stdio_write() by benpicco · Pull Request #16822 · RIOT-OS/RIOT

benpicco · 2021-09-07T14:35:34Z

Contribution description

On native the functions stdio_read() / stdio_write() were not used.
Those functions are intended for alternative stdio implementations.
As a result, no alternative stdio could be used on native.

To fix this, call the functions in _native_read() / _native_write() when dealing with stdio fds.

Testing procedure

native applications should work as before.
But now alternative stdio implementations are possible.

Issues/PRs references

needed for #16723 to work on native

benpicco · 2021-09-08T13:35:29Z

Ok turns out guarding the read / write with _native_syscall_enter()/_native_syscall_leave() breaks tests/sys_atomic_utils

maribu

looks good to me. Some comments inline.

cpu/native/stdio_native/stdio_native.c

cpu/native/syscalls.c

kfessel · 2021-09-17T15:52:53Z

I had some hope this would help with #16834 but it does not.
i tried to readd the native_syscall_enter and leave but that did not help either

kfessel

This is missing STDERR, which i assume should be handled similar to STDOUT.

Having a deeper look it i think:
the whole printf and getchar system seems to avoid going through riots VFS making them kind of disconnected from each other.

Shouldn't _native_write call vfs.write?

Maybe someone with a better insight into this VFS (e.g.: @kaspar030) can comment on that.

miri64 · 2021-09-20T11:56:43Z

the whole printf and getchar system seems to avoid going through riots VFS making them kind of disconnected from each other.

That's somewhat the point, as vfs is supposed to be optional (you do not always want to have a file system). Please let's keep that as is. We do not want make file systems accidentally mandatory and then have a no-op filesystem in case we do not have one, like we currently have with stdio (see stdio_null).

benpicco · 2021-09-20T12:07:40Z

This is missing STDERR, which i assume should be handled similar to STDOUT.

I was thinking of that as a feature, but you are right that it's more consistent that way, fixed.

kaspar030 · 2021-09-20T18:49:00Z

* Shouldn't _native_write call vfs.write?
Maybe someone with a better insight into this VFS (e.g.: @kaspar030) can comment on that.

The issue is that read() etc. are blocking syscalls, even when used as _native_read() etc.
The solution would be to port this to use async io. There's already native_async_read() etc. in place and used by other modules.

kfessel

With comments by miri64 and kaspar030, i think the non-blocking read is out of scope for this PR. I tried something with async_read in stdio_native and this patch helped in doing so.

This PR also changes how stdio works into the form someone reading the directory structure in native would expect it to do.

Lets avoid the scope creep. 🚀

kfessel · 2021-09-20T20:36:51Z

cpu/native/syscalls.c

+            ssize_t res = stdio_write(iov->iov_base, iov->iov_len);
+            iov++;


I think res needs to be compared to iov->iov_len before advancing to next element,
(there are stdio implementations in RIOT that might not write everything given to them (avoid blocking))

2402326 should fix this

(Is this function actually ever used in RIOT?)

(Is this function actually ever used in RIOT?)

afaik there is/was something in glibc that uses it for something more common

kfessel · 2021-09-20T21:38:38Z

cpu/native/syscalls.c

+                ssize_t res = stdio_write((void *)((uintptr_t)iov->iov_base + offset),
+                                          iov->iov_len - offset);
+                if (res < 0) {
+                    return res;


Suggested change

return res;

if (r > 0 )return r; else return res;

Is that how writev is supposed to behave oO?

Manpage says:

On success, readv(), preadv(), and preadv2() return the number of bytes read;\ writev(), pwritev(), and pwritev2() return the number of bytes written. Note that it is not an error for a successful call to transfer fewer bytes than\ requested (see read(2) and write(2))

Spec is a bit different though.

I am not sure about that since the the spec says return bytes written on success, which it does not defined by it, but the manpage does so, as any byte written is a success.

On the other hand the specs define the unsuccess (error) as the fd is not changed -> nothing is written?

Please rephrase... not really sure what you mean.

the spec says:

Upon successful completion, writev() shall return the number of bytes actually written.

but it does not define success,
but it defines the unsuccess (error) case

Otherwise, it shall return a value of -1, the file-pointer shall remain unchanged,\ and errno shall be set to indicate an error.

this says the the file-pointer shall remain unchanged on error i am not sure if there is a file-point file/char-device content equality but if there is this means the nothing has to be written if -1 if returned.

the manpage on the other hand defines the success as a partial write

Note that it is not an error for a successful call to transfer fewer bytes than requested

the write manpage (advertised by the writev page) goes into detail howto get the error state:

In the event of a partial write, the caller can make another write()\ call to transfer the remaining bytes. \ The subsequent call will either transfer further bytes or may result \ in an error (e.g., if the disk is now full)

kfessel · 2021-09-21T16:05:41Z

cpu/native/syscalls.c

+            size_t offset = 0;
+            while (iov->iov_len > offset) {
+                ssize_t res = stdio_write((void *)((uintptr_t)iov->iov_base + offset),
+                                          iov->iov_len - offset);
+                if (res < 0) {
+                    return res;
+                }
+                offset += res;
+            }
+            r += offset;


i would avoid the inner loop: (return on the first write that is less than it was asked to the app would be expected to repead the write as it is written in the manpage)

Suggested change

size_t offset = 0;

while (iov->iov_len > offset) {

ssize_t res = stdio_write((void *)((uintptr_t)iov->iov_base + offset),

iov->iov_len - offset);

if (res < 0) {

return res;

}

offset += res;

}

r += offset;

ssize_t res = stdio_write((void *)((uintptr_t)iov->iov_base),

iov->iov_len);

if( res >= 0 ){

r += res;

}

if (res < iov->iov_len) {

return (r>=0) ? r : res;

}

Ok, changed it - this is a strange API

cpu/native/syscalls.c

kfessel

This PR also changes how stdio works into the form someone reading the directory structure in native would expect it to do.

I tried something with async_read in stdio_native and this patch helped to do so.

kfessel · 2021-09-22T15:22:08Z

please squash

On `native` the functions stdio_read() / stdio_write() were not used. Those functions are intended for alternative stdio implementations. As a result, no alternative stdio could be used on `native`. To fix this, call the functions in `_native_read()` / `_native_write()` when dealing with stdio fds.

github-actions bot added Area: cpu Area: CPU/MCU ports Platform: native Platform: This PR/issue effects the native platform labels Sep 7, 2021

benpicco mentioned this pull request Sep 7, 2021

sys/net/application_layer: add telnet server module & example #16723

Merged

benpicco force-pushed the cpu/native-stdio branch from 33827e3 to 1f50d08 Compare September 7, 2021 14:48

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 7, 2021

benpicco requested review from LudwigKnuepfer, kaspar030, maribu and miri64 and removed request for miri64 September 8, 2021 13:36

maribu reviewed Sep 8, 2021

View reviewed changes

cpu/native/stdio_native/stdio_native.c Outdated Show resolved Hide resolved

cpu/native/stdio_native/stdio_native.c Outdated Show resolved Hide resolved

cpu/native/syscalls.c Outdated Show resolved Hide resolved

benpicco requested review from bergzand, fjmolinas and kfessel September 14, 2021 21:18

kfessel mentioned this pull request Sep 17, 2021

native getchar is blocking RIOT #16834

Closed

kfessel reviewed Sep 20, 2021

View reviewed changes

kfessel reviewed Sep 21, 2021

View reviewed changes

benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 22, 2021

kfessel reviewed Sep 22, 2021

View reviewed changes

cpu/native/syscalls.c Outdated Show resolved Hide resolved

kfessel approved these changes Sep 22, 2021

View reviewed changes

benpicco force-pushed the cpu/native-stdio branch from 05f65a2 to 13e16fa Compare September 22, 2021 15:22

benpicco merged commit cc3df3e into RIOT-OS:master Sep 22, 2021

benpicco deleted the cpu/native-stdio branch September 22, 2021 18:47

benpicco added this to the Release 2021.10 milestone Oct 20, 2021

		ssize_t res = stdio_write(iov->iov_base, iov->iov_len);
		iov++;

Conversation

benpicco commented Sep 7, 2021

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

benpicco commented Sep 8, 2021

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kfessel commented Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kfessel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Sep 20, 2021

Uh oh!

benpicco commented Sep 20, 2021

Uh oh!

kaspar030 commented Sep 20, 2021

Uh oh!

kfessel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfessel Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfessel Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfessel Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfessel Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfessel Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kfessel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfessel commented Sep 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

kfessel commented Sep 17, 2021 •

edited

Loading

kfessel left a comment •

edited

Loading

kfessel Sep 21, 2021 •

edited

Loading

kfessel Sep 21, 2021 •

edited

Loading

kfessel Sep 21, 2021 •

edited

Loading

kfessel Sep 21, 2021 •

edited

Loading

kfessel Sep 21, 2021 •

edited

Loading

kfessel left a comment •

edited

Loading