Skip to content

socketpair improvements #54458

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

Conversation

cfriedt
Copy link
Member

@cfriedt cfriedt commented Feb 4, 2023

4 commits:

  • improvements for socketpair sample app (it was broken for a while)
  • rename socketpair_example.c to main.c
  • minor improvements for socketpair.c
  • additional <fcntl.h> corrections in the networking subsystem

Fixes #54347
Fixes #52360

@cfriedt cfriedt added bug The issue is a bug, or the PR is fixing a bug area: Networking area: POSIX POSIX API Library labels Feb 4, 2023
@cfriedt cfriedt force-pushed the issues/52360/samples-net-sockets-socketpair-does-not-run-as-expected branch 4 times, most recently from 545a1df to afeeced Compare February 5, 2023 00:43
@cfriedt
Copy link
Member Author

cfriedt commented Feb 5, 2023

I would really prefer to get this sample running in harness: console rather than harness: net so that we can run it on any host and have twister automatically check the output for SUCCESS using the regex property in sample.yaml. It's completely unnecessary for socketpair to depend on networking other than relying on some of the existing socket code.

Pushing that off to a later date though.

@cfriedt cfriedt marked this pull request as ready for review February 5, 2023 02:32
@zephyrbot zephyrbot added the area: Sockets Networking sockets label Feb 5, 2023
@cfriedt cfriedt force-pushed the issues/52360/samples-net-sockets-socketpair-does-not-run-as-expected branch 2 times, most recently from f5a9b6e to ebb5e58 Compare February 5, 2023 11:01
@cfriedt cfriedt requested review from stephanosio and jukkar February 5, 2023 11:11
@cfriedt cfriedt force-pushed the issues/52360/samples-net-sockets-socketpair-does-not-run-as-expected branch from ebb5e58 to 12ddcb7 Compare February 5, 2023 11:14
@cfriedt cfriedt added priority: low Low impact/importance bug and removed priority: low Low impact/importance bug labels Feb 5, 2023
@cfriedt cfriedt added this to the v3.3.0 milestone Feb 7, 2023
@rlubos
Copy link
Collaborator

rlubos commented Feb 7, 2023

@cfriedt net/lib/sockets/sockets_tls.c also needs an update.

@SeppoTakalo
Copy link
Collaborator

Is there a reason why we apply this workaround:

#ifdef CONFIG_ARCH_POSIX
#include <fcntl.h>
#else
#include <zephyr/posix/fcntl.h>
#endif

to all files, but not in the <zephyr/posix/fcntl.h> header itself?

@cfriedt
Copy link
Member Author

cfriedt commented Feb 7, 2023

Is there a reason why we apply this workaround:

#ifdef CONFIG_ARCH_POSIX
#include <fcntl.h>
#else
#include <zephyr/posix/fcntl.h>
#endif

to all files, but not in the <zephyr/posix/fcntl.h> header itself?

Yes, the eventual goal would be to include <zephyr/posix/fcntl.h> everywhere instead of <fcntl.h>[1]. However, getting there would require being able to test Zephyr's POSIX API on native_posix and we're not quite there yet. The foreseeable next plans for the posix api are to remove workarounds and to bring in slightly more mature posix headers. I would not recommend mixing them up again. It's actually somewhat helpful to identify where we conditionally include <fcntl.h> for native_posix as well rather than just continuing to sweep it under the carpet.

Specifically w.r.t. the network subsystem though, due to CONFIG_ARCH_POSIX and CONFIG_POSIX_API being mutually exclusive for the last few years, it has precipitated quite a bit of spaghetti between lib/os/, subsys/net, and lib/posix.

At the moment there is a cyclic dependency between net and posix that needs to be broken by a mutual one.

I.e. Currently, we have:

┌───────────┐       ┌───────────┐
│           ├──────►│           │
│   POSIX   │       │   NET     │
│           │◄──────┤           │
└───────────┘       └───────────┘

But we need

          ┌───────────┐
          │           │
          │    ??     │
          │           │
          └▲────────▲─┘
           │        │
           │        │
┌──────────┴┐       ├───────────┐
│           │       │           │
│   POSIX   ├───────►   NET     │
│           │       │           │
└───────────┘       └───────────┘

I'm thinking the correct name for ?? would be something like zfdio

Diagrams made with https://asciiflow.com/

[1] That assumes we don not want CONFIG_POSIX_API=y to be the default. If it were the default, that allow us to use all posix headers non-prefixed again everywhere (e.g. <fcntl.h>, <unistd.h>)

@cfriedt cfriedt force-pushed the issues/52360/samples-net-sockets-socketpair-does-not-run-as-expected branch from 12ddcb7 to 96ce8c3 Compare February 7, 2023 12:15
@cfriedt
Copy link
Member Author

cfriedt commented Feb 7, 2023

@cfriedt net/lib/sockets/sockets_tls.c also needs an update.

@rlubos - fixed!

@cfriedt cfriedt force-pushed the issues/52360/samples-net-sockets-socketpair-does-not-run-as-expected branch from 96ce8c3 to 8ce7cd2 Compare February 7, 2023 12:19
* use `read()` instead of `recv()`
* use `write()` instead of `send()`
* use `CONFIG_POSIX_API` and drop `<zephyr/posix/..>` prefix
* use `perror()`
* fix `Makefile.posix` to use `CC` instead of `CXX`
* fix race condition which caused an unhandled EOF
  and infinite loop, by adding a readback / echo
* Updated the docs to reflect the expected stdout
* Improve doc clarity

Signed-off-by: Chris Friedt <[email protected]>
The source file for most single-file sample apps is usually
`main.c`. Rename this file to be consistent with other
samples.

Signed-off-by: Chris Friedt <[email protected]>
* include `<zephyr/posix/fcntl.h>` instead of `<fcntl.h>`
* drop unused logging header and module declaration
* reorder headers alphabetically

Signed-off-by: Chris Friedt <[email protected]>
If we are using `CONFIG_ARCH_POSIX`, then include
`<fcntl.h>`. Otherwise, include `<zephyr/posix/fcntl.h>`
since there are no requirements to use `CONFIG_POSIX_API`
internally.

Signed-off-by: Chris Friedt <[email protected]>
@cfriedt cfriedt force-pushed the issues/52360/samples-net-sockets-socketpair-does-not-run-as-expected branch from 8ce7cd2 to eda4e4f Compare February 7, 2023 12:21
Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

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

LGTM

@stephanosio stephanosio merged commit c093678 into zephyrproject-rtos:main Feb 8, 2023
@cfriedt cfriedt deleted the issues/52360/samples-net-sockets-socketpair-does-not-run-as-expected branch February 8, 2023 10:06
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: Sockets Networking sockets bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
6 participants