posix: Clean up various headers #16626

pfalcon · 2019-06-04T20:18:19Z

Ok, so this started as just moving open() from unistd.h to fcntl.h, but as usual, led to chain of conflicts, which however this time were tracked down to completion.

Per POSIX, open() is defined in <fcntl.h>. fcntl.h in turn comes from
the underlying libc, either newlib, or minimal libc.

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

pfalcon · 2019-06-05T12:55:58Z

Ok, so this started as just moving open() from unistd.h to fcntl.h, but as usual, led to chain of conflicts, which however this time were tracked down to completion.

lib/libc/minimal/include/sys/stat.h

include/posix/time.h

mbolivar

If the goal is a cleanup, I think at least including kernel.h from a libc header ought to be addressed, as it does fall under that umbrella.

pfalcon · 2019-06-07T18:46:47Z

@mbolivar: I don't understand what you mean. Please review previous comments and history of title changes regarding the goal of this PR. And perhaps, post comments without hitting -1 button right away?

mbolivar · 2019-06-07T18:53:40Z

post comments without hitting -1 button right away?

I posted 3 comments; what are you talking about?

pfalcon · 2019-06-07T18:55:20Z

I posted 3 comments; what are you talking about?

Ok, sorry, I didn't get them in email, and didn't see at once here.

mbolivar · 2019-06-07T20:28:21Z

I'm done with this PR. Dismissing my stale review.

apparently I'm just bikeshedding

pabigot · 2019-06-27T11:40:02Z

lib/libc/minimal/include/sys/stat.h

+extern "C" {
+#endif
+
+#include <kernel.h>


I'm with @mbolivar from the perspective of minimal libc headers should not be including kernel.h. However I'll say it shouldn't have been put into the Zephyr's POSIX material in the first place, so that ship has sailed and though removing the include should happen it's not in scope for this PR.

Should not be including kernel.h here

Should not be including kernel.h here

Again, I just moved the file around, and made minimal change to accompany that, if any. Let me try to remove that and see if it leads to cascade of unexpected CI breaks (which is very common so far with POSIX subsys stuff, and what I'm trying to fix - step by step).

pabigot · 2019-06-27T11:43:12Z

I would like to see this merged soon since it's required as the basis of reworking #17003.

pfalcon · 2019-06-27T15:10:42Z

@pabigot: Thanks for approval.

pabigot · 2019-07-01T19:34:55Z

@pfalcon Thanks. My rework of #17003 is complete and waiting for this to be merged so I can submit those changes and DS3231 support. I'm hoping that'll happen soon. Enjoy your vacation.

andrewboie · 2019-07-01T19:47:42Z

@nashif can you re-visit your review?

The remark was already included in the recent update.

mgielda

LGTM

mgielda · 2019-07-25T12:22:57Z

(I know Anas is on holiday and double checked that his remark was included, so I dismissed it - hope that's OK)

pfalcon · 2019-07-25T14:22:06Z

@mgielda, Thanks for review.

Also, the PR is rebased against master, added a commit to fix issue mentioned in #17768 (comment) (cc: @PiotrZierhoffer).

lib/libc/minimal/include/time.h

include/posix/time.h

lib/libc/minimal/include/sys/stat.h

galak

A few minor comments, but otherwise looks good.

Newlib libc already provides sys/stat.h, so trying to have sys/stat.h on the level of POSIX subsys inevitable leads to include order and definition conflicts. Instead (as most of other sys/* includes) should come from the underlying libc. While moving, made unrelated change of removing #include <kernel.h>, to accommodate the change reviewers. Signed-off-by: Paul Sokolovsky <[email protected]>

POSIX subsys defines struct timespec in <time.h> (as POSIX public API requires), but newlib defines in in sys/_timespec.h, which inevitably leads to inclusion order and redifinition conflicts. Follow newlib way and define it in single place, sys/_timespec.h, which belongs to libc namespace. Thus, we move current definition to minimal libc, and will use either minlibc's or newlib's definition, instead of trying to redefine it. This is similar to the introduction of sys/_timeval.h done earlier. Signed-off-by: Paul Sokolovsky <[email protected]>

Unfortunately, Zephyr SDK 0.10.0 ships with outdated Newlib 2.0.0 (from 2015 or earlier) which lacks sys/_timespec.h header, requiring ugly workaround of defining struct timespec inline (the whole idea was to standardize on sys/_timespec.h header for different libc's). This is similar to earlier workaround for struct timeval definition introduced in a6aee9b. Zephyr SDK ticket for this issue is zephyrproject-rtos/sdk-ng#64, and it will ve possible to remove both workarounds when Xtensa toolchain will be upgraded to newlib version consistent with other architectures. Signed-off-by: Paul Sokolovsky <[email protected]>

That's the header which is supposed to define them, there was even FIXME on that in mqueue.h. Signed-off-by: Paul Sokolovsky <[email protected]>

Per POSIX, open() is defined in <fcntl.h>. fcntl.h in turn comes from the underlying libc, either newlib, or minimal libc. Signed-off-by: Paul Sokolovsky <[email protected]>

By the latest convention, libc's define struct timespec in sys/_timespec.h. This is consistent with Newlib and ensures about errors due to redefinitions. Signed-off-by: Paul Sokolovsky <[email protected]>

pfalcon · 2019-07-25T16:28:39Z

A few minor comments, but otherwise looks good.

Should be addressed now.

pfalcon added the area: POSIX POSIX API Library label Jun 4, 2019

pfalcon requested review from d3zd3z, mgielda, nashif, andrewboie and galak June 4, 2019 20:18

pfalcon force-pushed the posix-open branch 6 times, most recently from 42a480b to 6dcacd0 Compare June 5, 2019 11:59

pfalcon changed the title ~~posix: unistd.h: open() doesn't belong here~~ posix: Clean up various headers Jun 5, 2019

nashif added the Maintainer At least 2 days before merge, maintainer review required label Jun 7, 2019

mbolivar reviewed Jun 7, 2019

View reviewed changes

lib/libc/minimal/include/sys/stat.h Outdated Show resolved Hide resolved

include/posix/time.h Show resolved Hide resolved

include/posix/time.h Show resolved Hide resolved

mbolivar previously requested changes Jun 7, 2019

View reviewed changes

pfalcon requested a review from mbolivar June 7, 2019 18:46

pfalcon mentioned this pull request Jun 7, 2019

[RFC] Missing parts of libc required for CivetWeb #16683

Closed

pfalcon mentioned this pull request Jun 26, 2019

Enhancements for civil time conversions #17003

Closed

pabigot self-requested a review June 26, 2019 14:47

pabigot mentioned this pull request Jun 27, 2019

Zephyr SDK ships with very outdated newlib for Xtensa zephyrproject-rtos/sdk-ng#64

Closed

pabigot approved these changes Jun 27, 2019

View reviewed changes

andrewboie approved these changes Jul 1, 2019

View reviewed changes

pfalcon mentioned this pull request Jul 2, 2019

posix: Add headers related to BSD Sockets API #16621

Merged

pabigot mentioned this pull request Jul 10, 2019

libc/minimal: add support for gmtime and its reverse "timegm" #17468

Merged

pfalcon mentioned this pull request Jul 22, 2019

[RFC] Proposed development plan for Zephyr's POSIX subsystem #17706

Closed

PiotrZierhoffer mentioned this pull request Jul 25, 2019

minimal-libc: Add define guard around struct timespec #17768

Closed

mgielda approved these changes Jul 25, 2019

View reviewed changes

carlescufi added the dev-review To be discussed in dev-review meeting label Jul 25, 2019

pfalcon force-pushed the posix-open branch from fb2b82b to 5db699f Compare July 25, 2019 12:57

galak reviewed Jul 25, 2019

View reviewed changes

lib/libc/minimal/include/time.h Show resolved Hide resolved

galak reviewed Jul 25, 2019

View reviewed changes

include/posix/time.h Show resolved Hide resolved

galak reviewed Jul 25, 2019

View reviewed changes

lib/libc/minimal/include/sys/stat.h Show resolved Hide resolved

galak requested changes Jul 25, 2019

View reviewed changes

galak removed the dev-review To be discussed in dev-review meeting label Jul 25, 2019

pfalcon added 6 commits July 25, 2019 19:28

posix: mqueue.h: Move O_CREAT and friends to fcntl.h

fa79039

That's the header which is supposed to define them, there was even FIXME on that in mqueue.h. Signed-off-by: Paul Sokolovsky <[email protected]>

posix: unistd.h: open() doesn't belong here

66a0f0d

Per POSIX, open() is defined in <fcntl.h>. fcntl.h in turn comes from the underlying libc, either newlib, or minimal libc. Signed-off-by: Paul Sokolovsky <[email protected]>

libc: minimal: time.h: Don't (re)define struct timespec.

69b2bf8

By the latest convention, libc's define struct timespec in sys/_timespec.h. This is consistent with Newlib and ensures about errors due to redefinitions. Signed-off-by: Paul Sokolovsky <[email protected]>

pfalcon force-pushed the posix-open branch from 5db699f to 69b2bf8 Compare July 25, 2019 16:28

galak approved these changes Jul 25, 2019

View reviewed changes

galak merged commit 6b90a02 into zephyrproject-rtos:master Jul 25, 2019

This was referenced Aug 28, 2019

samples: net: sockets: Allow to build and test with POSIX subsys #18736

Merged

posix: Kconfig: Allow to enable individual components, POSIX_API just enables all #18780

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

posix: Clean up various headers #16626

posix: Clean up various headers #16626

pfalcon commented Jun 4, 2019 •

edited

Loading

pfalcon commented Jun 5, 2019

mbolivar left a comment

pfalcon commented Jun 7, 2019

mbolivar commented Jun 7, 2019

pfalcon commented Jun 7, 2019

mbolivar commented Jun 7, 2019

pabigot Jun 27, 2019

nashif Jun 27, 2019

pfalcon Jun 27, 2019

pabigot commented Jun 27, 2019

pfalcon commented Jun 27, 2019

pabigot commented Jul 1, 2019

andrewboie commented Jul 1, 2019

mgielda left a comment

mgielda commented Jul 25, 2019

pfalcon commented Jul 25, 2019

galak left a comment

pfalcon commented Jul 25, 2019

posix: Clean up various headers #16626

posix: Clean up various headers #16626

Conversation

pfalcon commented Jun 4, 2019 • edited Loading

pfalcon commented Jun 5, 2019

mbolivar left a comment

Choose a reason for hiding this comment

pfalcon commented Jun 7, 2019

mbolivar commented Jun 7, 2019

pfalcon commented Jun 7, 2019

mbolivar commented Jun 7, 2019

pabigot Jun 27, 2019

Choose a reason for hiding this comment

nashif Jun 27, 2019

Choose a reason for hiding this comment

pfalcon Jun 27, 2019

Choose a reason for hiding this comment

pabigot commented Jun 27, 2019

pfalcon commented Jun 27, 2019

pabigot commented Jul 1, 2019

andrewboie commented Jul 1, 2019

mgielda left a comment

Choose a reason for hiding this comment

mgielda commented Jul 25, 2019

pfalcon commented Jul 25, 2019

galak left a comment

Choose a reason for hiding this comment

pfalcon commented Jul 25, 2019

pfalcon commented Jun 4, 2019 •

edited

Loading