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.

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

samples: net: echo_async_select: Use read()/write() if possible #25356

Merged
merged 1 commit into from
May 20, 2020

Conversation

pfalcon
Copy link
Contributor

@pfalcon pfalcon commented May 15, 2020

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 pfalcon requested review from jukkar and tbursztyka as code owners May 15, 2020 11:17
@pfalcon pfalcon added area: Networking area: Sockets Networking sockets area: POSIX POSIX API Library labels May 15, 2020
@zephyrbot zephyrbot added the area: Samples Samples label May 15, 2020
@zephyrbot
Copy link
Collaborator

zephyrbot commented May 15, 2020

All checks passed.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@pfalcon
Copy link
Contributor Author

pfalcon commented May 15, 2020

@cfriedt: Please review too. And I would appreciate if you performed the similar conversion for your socketpair test(s), as was suggested in the original review. (I didn't want to push with that in the original review, given the amount changes you already made, but I would expect you to follow the suggestion given then. Thanks.)

@pfalcon pfalcon force-pushed the samples-socket-read branch from 47a6682 to 17278a8 Compare May 15, 2020 11:22
@cfriedt
Copy link
Member

cfriedt commented May 15, 2020

I found some really strange behaviour when I used read() / write() with CONFIG_POSIX_API=y, in that it would actually just write to stdout instead of to the actual socket. Maybe things have changed since I tested that, so I'll have a look.

Also, looks like you need to include <posix/unistd.h> to avoid the implicit function declaration warning (promoted to error).

@pfalcon
Copy link
Contributor Author

pfalcon commented May 15, 2020

@cfriedt, thanks for looking.

I found some really strange behaviour when I used read() / write() with CONFIG_POSIX_API=y, in that it would actually just write to stdout instead of to the actual socket.

As we discussed, you found that behavior with CONFIG_POSIX_API=n (unless you have a reproduction testcase).

Also, looks like you need to include <posix/unistd.h> to avoid the implicit function declaration warning (promoted to error).

I don't see any such warning with either Zephyr or Linux, and likely reason that <unistd.h> (sic) is already included.

CI fails because platform_whitelist: qemu_x86 directive in sample.yaml doesn't seem to have effect (it starts to build for know-broken pseudo-targets like native_posix_64). Investigating.

@pfalcon
Copy link
Contributor Author

pfalcon commented May 15, 2020

Ah, d'oh, I see the problem - committed extra code lying in my workcopy. Let's call it Friday afternoon. Fixing.

@pfalcon pfalcon force-pushed the samples-socket-read branch from 17278a8 to 0da8a89 Compare May 15, 2020 13:04
@pfalcon
Copy link
Contributor Author

pfalcon commented May 15, 2020

Ok, pushed changes for just echo_async_select, as was intended.

@cfriedt
Copy link
Member

cfriedt commented May 15, 2020

Ok, I'll create a ticket for socketpair tests. Unfortunately, I have no reviewer privs here, so I can only say 👍

@pfalcon
Copy link
Contributor Author

pfalcon commented May 15, 2020

Ok, I'll create a ticket for socketpair tests.

Thanks!

Unfortunately, I have no reviewer privs here, so I can only say +1

Yep, you don't have suitable level of membership so I can add you as a reviewer. However, on any project on github, for any PR, you can go to "Files changed" tab (https://github.com/zephyrproject-rtos/zephyr/pull/25356/files here), then you'll see green "Review changes" button near top left corner. Clicking on it, you can leave you review. (And that would be appreciated, because one of the problem with POSIX subsys is that I'm the only codeowner, so if I submit PR, there can nobody else to review. So, people interested in the POSIX subsys should keep together and help each other with reviews. Thanks.)

Copy link
Member

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

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

Looks good to me. Regular read(2) write(2) support is a must for streaming APIs.

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>
@pfalcon pfalcon force-pushed the samples-socket-read branch from 0da8a89 to 19bcc60 Compare May 18, 2020 09:48
@pfalcon
Copy link
Contributor Author

pfalcon commented May 18, 2020

Created a related bug ticket: #25407 , marking for 2.3.

@pfalcon pfalcon added this to the v2.3.0 milestone May 18, 2020
@carlescufi carlescufi requested a review from rlubos May 19, 2020 17:51
@carlescufi carlescufi merged commit 8af7187 into zephyrproject-rtos:master May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Networking area: POSIX POSIX API Library area: Samples Samples area: Sockets Networking sockets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No tests/samples covering socket read()/write() calls
5 participants