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

[WIP][DNM] lib: posix: Elaborate for wider yet more selective POSIX coverage #12984

Closed
wants to merge 14 commits into from

Conversation

pfalcon
Copy link
Contributor

@pfalcon pfalcon commented Feb 1, 2019

Basic requirements:

  1. It should be possible to control whether include/posix/ is
    added to include search path or not.
  2. It should be possible to use individual parts of POSIX API,
    without bringing in everything else. For example, if user is
    interested in just sleep(), should be possible to enable just
    that.
  3. Keep existing CONFIG_POSIX_API as "master switch", which
    enables "a lot of POSIX APIs".

Fixes: #12965

Signed-off-by: Paul Sokolovsky paul.sokolovsky@linaro.org

@pfalcon pfalcon added DNM This PR should not be merged (Do Not Merge) area: POSIX POSIX API Library labels Feb 1, 2019
@pfalcon pfalcon force-pushed the posix-fine branch 2 times, most recently from 516153f to fee6124 Compare February 4, 2019 18:12
@pfalcon pfalcon changed the title [WIP][DNM] lib: posix: Use more finegrained and orthogonal Kconfig options [WIP][DNM] lib: posix: Use more fine-grained and orthogonal Kconfig options Feb 5, 2019
@aescolar
Copy link
Member

aescolar commented Feb 5, 2019

  1. It should be possible to control whether include/posix/ is
    added to include search path or not.

include/posix was moved into that posix/ folder (by Anas) purposefully to avoid include conflicts with the natiev_posix board. The idea is that people should include as "posix/something" if they want to include the Zephyr POSIX API replacement

@@ -85,6 +88,8 @@ typedef struct pthread_rwlock_obj {

#endif /* CONFIG_PTHREAD_IPC */

#endif /* CONFIG_ARCH_POSIX */
Copy link
Member

Choose a reason for hiding this comment

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

This is not nice, as you just remove that type and therefore that API when compiling for the posix arch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not nice, as you just remove that type and therefore that API when compiling for the posix arch

No, sys/types.h absolutely gotta come from the underlying POSIX OS, and I make Zephyr's POSIX subsys defines not clash with that.

(You may ask where exactly host sys/types.h is included, and that's "good question". Current situation is that it's included "somewhere", and that already causes clashes. And the right solution for now is excluding tests where the conflict happens from being run, instead of wasting time on solving such riddles.)

@@ -1,17 +1,20 @@

add_library(PTHREAD INTERFACE)

if(CONFIG_POSIX_HEADERS)
target_include_directories(PTHREAD INTERFACE ${ZEPHYR_BASE}/include/posix)
Copy link
Member

@aescolar aescolar Feb 5, 2019

Choose a reason for hiding this comment

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

this should not be here, or you will not be able to select host files instead of Zephyr posix files and viceversa

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should not be here, or you will not be able to select host files instead of Zephyr posix files and viceversa

???

Assuming you put comment on the exact line, it simply is there. Saying that it should not be there means digging in history, and addressing that comment to the author of that line. (But from my PoV, it of course should be there. But of course, it should be controlled, like my patch does.)

Copy link
Member

@aescolar aescolar Feb 6, 2019

Choose a reason for hiding this comment

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

I know it is there.. ;). Werent you trying to improve it? :p
Done like that you have the problem with inclusion order dependencies (which there is today) in the posix API files, where some of its headers and the host headers have the same name.
Knowing this, may help you understand some of the errors you see when trying to modify this code.

My mistake, I had read that line too fast

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know it is there.. ;). Werent you trying to improve it? :p

So, let's start from the start. When someone wants to use a POSIX function, they go to a reference and look it up. E.g. for sleep() http://pubs.opengroup.org/onlinepubs/9699919799/functions/sleep.html , it says #include <unistd.h>. So, using #include <unistd.h> for sleep() should just work, ditto for all other POSIX functions. That's the baseline requires for POSIX subsystem (and POSIX subsystem, Zephyr's included).

Being able to use #include <posix/unistd.h> is convenience hack, until the above works perfectly. What my patch does is to allow to control of automatic include path for include/posix/ in fine-grained way (as required to to make it work for hundreds of POSIX symbols, one by one).

@@ -1,7 +1,7 @@
if(NOT CONFIG_NATIVE_APPLICATION)
add_subdirectory(libc)
endif()
add_subdirectory_ifdef(CONFIG_POSIX_API posix)
add_subdirectory(posix)
Copy link
Member

Choose a reason for hiding this comment

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

If you include this always, you should have all its individual c files gated with something else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you include this always, you should have all its individual c files gated with something else

Probably yes, and eventually yes. But what happens when all those gates are enabled? Random build failures from native_posix. And that's the problem and means to address it which we discuss.

Copy link
Member

Choose a reason for hiding this comment

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

My comment was related to a) code side & b) if you include them without any Kconfig option, then you need to be careful that today posix_cheats only does the renaming if POSIX_API is set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then you need to be careful that today posix_cheats only does the renaming if POSIX_API is set

One way to be careful is again to never include posix_cheats when anything POSIX subsys related is used. As posix_cheats is mandatory for ARCH_POSIX, that leads to obvious conclusions.

@@ -14,11 +14,43 @@ config POSIX_MAX_FDS

config POSIX_API
bool "POSIX APIs"
select POSIX_HEADERS
Copy link
Member

Choose a reason for hiding this comment

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

I think Ulf would tell you to not use select in this way if you can avoid it.
Would a "default y if POSIX_API" in this option below be good enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of that config option is to select a bunch of other options.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I understand, but the keyword "select" has a meaning beyond the english meaning of select.
At least Ulf recommends to use select only to set promtless symbols when possible, as otherwise it can be very confusing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

select might be alright as currently used here. Could turn messy if dependencies are added. The Kconfig best practices page has this:

Rare exceptions might include cases where you’re sure that the dependencies of the selecting
and selected symbol will never drift out of sync, e.g. when dealing with two simple symbols defined
close to one another within the same if.

I'd add this to the help text too: "Particular POSIX APIs can still be enabled separately when this option is disabled."

@pfalcon
Copy link
Contributor Author

pfalcon commented Feb 6, 2019

@aescolar

include/posix was moved into that posix/ folder (by Anas) purposefully to avoid include conflicts with the natiev_posix board. The idea is that people should include as "posix/something" if they want to include the Zephyr POSIX API replacement

Yes, it it was done Anas. But no, not for being used like you wrote. You can see that yourself: 274ad46 .

@aescolar: I much appreciate prompt response to this matter. But let me be straight, the whole discussion shows that: I don't understand how ARCH_POSIX works; you don't understand how Zephyr POSIX subsys works. We definitely should improve each of out areas, actually, I'm set to do just that for posix subsys. But first and formost, we should break the deadlock both POSIXes put on each other. So, please continue with #13068, but "arch_exclude: posix" at the right places (and we have it in a bunch of places already) is the thing for the time being.

@pfalcon pfalcon force-pushed the posix-fine branch 2 times, most recently from 7b133c2 to 66bca9a Compare February 7, 2019 17:32
@zephyrbot
Copy link
Collaborator

zephyrbot commented Feb 7, 2019

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

@pfalcon
Copy link
Contributor Author

pfalcon commented Feb 7, 2019

@aescolar, @ulfalizer: Thanks for the suggestions, this is partly PoC and WIP PR, I'll be moving it into suggested and discussed directions over time.

Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

Let's not blacklist x86_64. Those tests all build and run successfully now, so this is going to reduce coverage. And the diagnosis in the patch here is actually wrong.

The proper fix, if you can't find a way to arrange the includes to not hit the missing limits.h issue, is to add and maintain our own copies of those headers for minimal libc builds.

@@ -7,3 +7,6 @@ tests:
# riscv toolchain lacks sys/timespec.h (maybe something else), so not
# good for POSIX.
arch_exclude: posix riscv32
# qemu_x86_64 currently uses Linux host toolchains, which leaks host
# headers into the build.
platform_exclude: qemu_x86_64
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's not what happening here. What's actually happening is that the toolchain is trying to include a limits.h header from the "platform", and not finding one. Our SDK compilers provide this header somehow (probably coming out of the sysroot they were built for), but a bare toolchain does not.

Technically this is a zephyr bug, I think. The limits.h header is part of POSIX (as well as ISO C) and if we want to provide POSIX we need to have our own (or piggy back on the one from newlib where practical, I guess).

@pfalcon
Copy link
Contributor Author

pfalcon commented Feb 8, 2019

Let's not blacklist x86_64.

@andyross, I didn't want to say that, but our further conversation will 100% follow the same pattern as we had a few days earlier with @aescolar here and in #13054, because his first phrase was also "Let's not blacklist native_posix" (or the same in meaning) ;-).

Here's the matter: I'm working on improving POSIX subsys, and some archs and boards work along with me, and some greet me with random build crashes on each step. I won't be telling maintainers of those archs and boards "you should fix that", I am telling "can we please backlog these unneeded complications and revisit it later, when Zephyr's POSIX subsys is actually improved". If you say no, fine. The only risk here is that Zephyr POSIX subsys won't be improved (much/enough). Someone will certainly solve it all later ;-).

pfalcon added 14 commits May 28, 2019 17:30
Basic requirements:
1. It should be possible to control whether include/posix/ is
added to include search path or not.
2. It should be possible to use individual parts of POSIX API,
without bringing in everything else. For example, if user is
interested in just sleep(), should be possible to enable just
that.
3. Keep existing CONFIG_POSIX_API as "master switch", which
enables "a lot of POSIX APIs".

Fixes: zephyrproject-rtos#12965

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
After previous change to make POSIX-related settings more
fine-grained, this module rather belongs to lib/posix/ than
just lib/os/.

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
Add:

CONFIG_POSIX_FD_OPS to enable close(), read(), write(), lseek().
CONFIG_POSIX_FCNTL_IOCTL to enable fcntl() and ioctl().
CONFIG_POSIX_STDIN_OUT_ERR to initialize fd's 0-2.

Separate option for fcntl() and ioctl() is provided, because we also
have socket-specific implementations of these calls (so, allow to
select basic fd operations from POSIX, but fcntl() from sockets).

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
CONFIG_POSIX_CLOCK and CONFIG_POSIX_TIMER.

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
Libc is part of POSIX, you can't have POSIX API without sizable share
of that API being implemented on libc level.

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
Depending on the fine details of configuration, these header may be
not pulled in for all setups, and missing defines will lead to build
errors.

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
E.g. sys/timespec.h.

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
qemu_x86_64 currently uses Linux host toolchains, which leaks host
headers into the build.

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
Similar to what previously done to tests/posix/fs, as it starts to
fail on some targets too.

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
Also lacks needed libc headers.

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
Per POSIX, gethostname() is declared in unistd.h.

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
Instead of older too coarse CONFIG_POSIX_API.

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
If a valid numeric IP address is provided as argument, it should
be resolved without contacting DNS server.

Also, implement handling of AI_NUMERICHOST.

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
And also for AI_NUMERICHOST flag.

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
@zephyrproject-rtos zephyrproject-rtos deleted a comment from codecov-io Jun 18, 2019
@github-actions github-actions bot added area: API Changes to public APIs area: C Library C Standard Library area: Networking area: Tests Issues related to a particular existing or missing test has-conflicts Issue/PR has conflicts with another issue/PR and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jun 29, 2020
@nashif nashif added the Stale label Sep 4, 2020
@github-actions github-actions bot closed this Sep 22, 2020
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: C Library C Standard Library area: Networking area: POSIX POSIX API Library area: Tests Issues related to a particular existing or missing test DNM This PR should not be merged (Do Not Merge) has-conflicts Issue/PR has conflicts with another issue/PR Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

POSIX subsys: Need more fine-grained enable options
6 participants