Skip to content

Conversation

@jukkar
Copy link
Member

@jukkar jukkar commented Jul 4, 2019

Provide POSIX headers for BSD Sockets API.

Signed-off-by: Paul Sokolovsky [email protected]

This contains the relevant code from #16621 except the empty header files as TSC does not want those in the system.

Provide POSIX headers for BSD Sockets API.

Signed-off-by: Paul Sokolovsky <[email protected]>
@zephyrbot zephyrbot added the area: API Changes to public APIs label Jul 4, 2019

#include <net/socket.h>

static inline char *inet_ntop(sa_family_t family, const void *src, char *dst,
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't sa_family_t and any other structures needed here be also defined in this namespace rather than in zephyr specific headers under include/net/?

Copy link
Member Author

Choose a reason for hiding this comment

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

All of zsock_* functions come from net/socket.h together with sa_family_t, to me this looks just fine.

@nashif
Copy link
Member

nashif commented Jul 4, 2019

I am wondering if we should just put those files directly into include/, there is really nothing that relates them to whatever we have inside include/posix which is implemented in lib/posix and just covers parts of PSE52 profile

@pfalcon
Copy link
Contributor

pfalcon commented Jul 4, 2019

I am wondering if we should just put those files directly into include/

No, we shouldn't. It should go where the other POSIX headers went. One sweet day, we'll switch them around - all POSIX headers will go under include/, and all Zephyr - under include/zephyr/. We're definitely not ready to that any time soon.

@nashif
Copy link
Member

nashif commented Jul 5, 2019

No, we shouldn't. It should go where the other POSIX headers went. One sweet day, we'll switch them around - all POSIX headers will go under include/, and all Zephyr - under include/zephyr/. We're definitely not ready to that any time soon.

that one sweet day will not come, we already spent too much time on cleaning up include/ and we should not start adding new technical debt that would need to be resolved later. Since you agree the above is actually the end-goal, this and other changes related to headers and additions to include/ need some more thought and longevity. We should not merge this just to satisfy some short term goals.

This and other changes related to headers need to be discussed in one of the upcoming project meetings.

and all Zephyr - under include/zephyr/.

We already decided not to go this route. This case is closed.

@nashif nashif added the dev-review To be discussed in dev-review meeting label Jul 5, 2019
Copy link
Member

@nashif nashif left a comment

Choose a reason for hiding this comment

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

-1 until further discussion is done in the dev-review meeting.

@jukkar
Copy link
Member Author

jukkar commented Jul 5, 2019

If we are not merging this, then we need to revert #16557. I will prepare a PR for that.

@jukkar
Copy link
Member Author

jukkar commented Jul 5, 2019

If we are not merging this, then we need to revert #16557. I will prepare a PR for that.

Just submitted a PR #17362 that will revert #16557

@jukkar
Copy link
Member Author

jukkar commented Jul 11, 2019

Closing this as agreed on dev meeting. I will re-open #17362 and rework it a bit (adding one more revert there).

@pfalcon
Copy link
Contributor

pfalcon commented Jul 11, 2019

Closing this as agreed on dev meeting.

I missed this "agreement".

The summary of the dev meeting from my PoV is: @nashif reiterated his generic concern of "what if, what if, putting these headers under include/posix/ is wrong?". No specific concerns were raised. The whole matter appears to be about "let's think this up more, because what if, what if we've been doing it all wrong all this time, and need to do it differently now".

No specific proposals beyond dumping POSIX headers straight into include/, despite the fact that existing POSIX headers are in include/posix/. No specific pro/cons analysis, only a hunch that "what if it was all wrong all this time, and we don't want to do redo it later, so let's drop any work and start thinking how to not make it so we redo it again later".

So, from my PoV, it's blocked on further, more complete proposals and their discussions.

@jukkar
Copy link
Member Author

jukkar commented Jul 12, 2019

Well, it we are not putting the these posix header anywhere atm then this PR can be closed. And if we want to somehow solve #17353 then only solution seems to be to revert the relevant patches as done in #17362

@pfalcon
Copy link
Contributor

pfalcon commented Jul 23, 2019

@jukkar

Well, it we are not putting the these posix header anywhere

We've been putting POSIX headers in include/posix/ for a year or so (since the inception of the POSIX subsystem).

atm then this PR can be closed.

This PR does improve some points of #16621 which some reviewers found problematic. #16621 isn't supposed to be closed, as it contains very important fixes and developments for POSIX subsystem. As #16621 may take some time to be elaborated, this PR tries to resolve at least some issues, by going for compromise of merging just subset of #16621 changes. Thanks for stepping up to prepare it, and I don't see reasons for it to be closed.

Most importantly, #17706 was posted, which contains the relevant information regarding POSIX subsys further development, which hopefully should unblock further progress with these patches. Reopening.

@pfalcon pfalcon reopened this Jul 23, 2019
@pfalcon
Copy link
Contributor

pfalcon commented Jul 23, 2019

@nashif: Please re-review based on the information in #17706. Thanks.

@galak
Copy link
Contributor

galak commented Jul 25, 2019

Closing in favor of the original PR as its now in sync with not having empty files.

@galak galak closed this Jul 25, 2019
@pabigot pabigot removed the dev-review To be discussed in dev-review meeting label Oct 24, 2019
@jukkar jukkar deleted the pfalcon-posix-socket-headers branch February 29, 2024 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: API Changes to public APIs area: Networking

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants