Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 14 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
67b0e1c
lib: posix: Use more fine-grained and orthogonal Kconfig options
pfalcon Feb 1, 2019
5f0abb5
lib: posix: Host fdtable.c
pfalcon Feb 4, 2019
30c5869
lib: posix: Introduce fine-grained options for fdtable.c
pfalcon Feb 4, 2019
bfdf557
lib: posix: Fine-grained options for clock_*() and timer_*().
pfalcon Feb 6, 2019
eeaacec
lib: posix: CONFIG_POSIX_API: Select NEWLIB_LIBC
pfalcon Feb 6, 2019
bb5485b
tests: posix: common: Explicitly include posix/sys/types.h
pfalcon Feb 7, 2019
5409342
tests: posix: Exclude riscv32, its toolchain lacks needed libc headers
pfalcon Feb 8, 2019
d46d930
tests: posix: Exclude qemu_x86_64
pfalcon Feb 8, 2019
9f06c3a
tests: posix: common: Increase stack size
pfalcon Feb 8, 2019
164c5fa
tests: posix: Exclude xtensa
pfalcon Feb 8, 2019
f92b18c
posix: unistd.h: Add gethostname()
pfalcon Feb 21, 2019
743eb53
libc: newlib: libc-hooks: Depend on fine-grained CONFIG_POSIX_FD_OPS
pfalcon Feb 21, 2019
a34503d
net: getaddrinfo: Parse numeric IPv4 addresses
pfalcon Feb 22, 2019
5f4fa60
tests: net: getaddrinfo: Add cases for numeric IPv4 addr
pfalcon May 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions include/posix/unistd.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ extern "C" {

#include "posix_types.h"
#include "sys/stat.h"
#include "net/socket.h"

#ifdef CONFIG_POSIX_API
#include <fs.h>
Expand All @@ -33,6 +34,11 @@ extern int mkdir(const char *path, mode_t mode);
unsigned sleep(unsigned int seconds);
int usleep(useconds_t useconds);

static inline int gethostname(char *buf, size_t len)
{
return zsock_gethostname(buf, len);
}

#ifdef __cplusplus
}
#endif
Expand Down
2 changes: 1 addition & 1 deletion lib/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
if(NOT CONFIG_NATIVE_APPLICATION)
add_subdirectory(libc)
endif()
add_subdirectory_ifdef(CONFIG_POSIX_API posix)
add_subdirectory(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.

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

Copy link
Contributor Author

@pfalcon pfalcon Feb 6, 2019

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

@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.

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

@pfalcon pfalcon Feb 6, 2019

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.

add_subdirectory_ifdef(CONFIG_CMSIS_RTOS_V1 cmsis_rtos_v1)
add_subdirectory_ifdef(CONFIG_CMSIS_RTOS_V2 cmsis_rtos_v2)
add_subdirectory(gui)
Expand Down
2 changes: 1 addition & 1 deletion lib/libc/newlib/libc-hooks.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ Z_SYSCALL_HANDLER(zephyr_write_stdout, buf, nbytes)
}
#endif

#ifndef CONFIG_POSIX_API
#ifndef CONFIG_POSIX_FD_OPS
int _read(int fd, char *buf, int nbytes)
{
ARG_UNUSED(fd);
Expand Down
1 change: 0 additions & 1 deletion lib/os/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ zephyr_sources(
crc16_sw.c
crc8_sw.c
crc7_sw.c
fdtable.c
mempool.c
rb.c
thread_entry.c
Expand Down
13 changes: 8 additions & 5 deletions lib/posix/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,21 @@

add_library(PTHREAD INTERFACE)

target_include_directories(PTHREAD INTERFACE ${ZEPHYR_BASE}/include/posix)
if(CONFIG_POSIX_HEADERS)
zephyr_include_directories(${ZEPHYR_BASE}/include/posix)
endif()

zephyr_library()
zephyr_library_sources(pthread_common.c)
zephyr_library_sources(fdtable.c)
zephyr_library_sources_ifdef(CONFIG_NEWLIB_LIBC pthread_common.c)
zephyr_library_sources_ifdef(CONFIG_PTHREAD_IPC pthread_cond.c)
zephyr_library_sources_ifdef(CONFIG_PTHREAD_IPC pthread_mutex.c)
zephyr_library_sources_ifdef(CONFIG_PTHREAD_IPC pthread_barrier.c)
zephyr_library_sources_ifdef(CONFIG_PTHREAD_IPC pthread.c)
zephyr_library_sources(pthread_sched.c)
zephyr_library_sources(clock.c)
zephyr_library_sources(sleep.c)
zephyr_library_sources(timer.c)
zephyr_library_sources_ifdef(CONFIG_POSIX_CLOCK clock.c)
zephyr_library_sources_ifdef(CONFIG_POSIX_SLEEP sleep.c)
zephyr_library_sources_ifdef(CONFIG_POSIX_TIMER timer.c)
zephyr_library_sources_ifdef(CONFIG_PTHREAD_IPC pthread_rwlock.c)
zephyr_library_sources_ifdef(CONFIG_PTHREAD_IPC semaphore.c)
zephyr_library_sources_ifdef(CONFIG_PTHREAD_IPC pthread_key.c)
Expand Down
49 changes: 46 additions & 3 deletions lib/posix/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,56 @@ config POSIX_MAX_FDS
config POSIX_API
depends on !ARCH_POSIX
bool "POSIX APIs"
select NEWLIB_LIBC
Copy link
Contributor

@andyross andyross Feb 8, 2019

Choose a reason for hiding this comment

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

This isn't correct, and as you've discovered has broken a bunch of tests that were passing on platforms that don't provide newlib.

I mean... the commit message is correct but sort of missing the point. POSIX does indeed depend on a bunch of stuff from the C standard. But our POSIX subset currently provides some of the POSIX standard above our minimal libc subset. And that has value, because those features work on platforms that don't provide newlib.

Please yank this bit and file bugs against libc features that are missing from minimal libc, or alternatively I guess you could advocate for making newlib the default everywhere (which would be a ton of work, but possible).

Copy link
Contributor Author

@pfalcon pfalcon Feb 8, 2019

Choose a reason for hiding this comment

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

POSIX does indeed depend on a bunch of stuff from the C standard.

No quite, POSIX itself includes definitions and requirements for libc (and they're even so kind as to point where they differ from ANSI C standard).

But our POSIX subset currently provides some of the POSIX standard above our minimal libc subset.

Let me translate that to a language which might be understood by a wider audience: we have not-really-working POSIX subsystem, and we brag that half of that works with minimal libc. Quite an accomplishment, but can't go to a conference with that, need something better ;-).

or alternatively I guess you could advocate for making newlib the default everywhere (which would be a ton of work, but possible).

No, I'm vocally against that. But have to conclude that for POSIX compatibility, newlib is a must.

Please yank this bit and file bugs against libc features that are missing from minimal libc

No, my plan is the opposite - first make POSIX work in no-nonsense way, then look which subset of that we can offer with minimal libc.

select POSIX_HEADERS
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.

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

@pfalcon pfalcon Feb 6, 2019

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

@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.

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
Contributor

@ulfalizer ulfalizer Feb 7, 2019

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."

select POSIX_SLEEP
select POSIX_CLOCK
select POSIX_TIMER
select POSIX_FD_OPS
select POSIX_FCNTL_IOCTL
select POSIX_STDIN_OUT_ERR
help
Enable mostly-standards-compliant implementations of
various POSIX (IEEE 1003.1) APIs.

if POSIX_API
config POSIX_HEADERS
bool "Make available POSIX headers"
help
If enabled, applications will be able to include <unistd.h>,
<sys/socket.h> and all other POSIX headers under their native
names. If disabled, all the headers are still available, with
"posix/" prefix, e.g. <posix/unistd.h>.

config POSIX_SLEEP
bool "POSIX sleep()/usleep() calls"
help
Provide sleep()/usleep() calls.

config POSIX_CLOCK
bool "POSIX clock_*() calls"
help
Provide clock_*() calls.

config POSIX_TIMER
bool "POSIX timer_*() calls"
help
Provide timer_*() calls.

config POSIX_FD_OPS
bool "Basic POSIX file descriptor operations"
help
Provide close(), read(), write(), lseek().

config POSIX_FCNTL_IOCTL
bool "POSIX fcntl() and ioctl() calls"
help
Provide fcntl() and ioctl().

config POSIX_STDIN_OUT_ERR
bool "stdin/stdout/stderr POSIX file descriptors"
help
Pre-initialize file descriptors 0, 1, and 2 for console
standard input/output.

config PTHREAD_IPC
bool "POSIX pthread IPC API"
Expand Down Expand Up @@ -95,5 +140,3 @@ config POSIX_MAX_OPEN_FILES
is additionally bounded by CONFIG_POSIX_MAX_FDS.
endif
endif # FILE_SYSTEM

endif # POSIX_API
16 changes: 12 additions & 4 deletions lib/os/fdtable.c → lib/posix/fdtable.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,12 @@ struct fd_entry {
#define FD_OBJ_STDOUT (void *)0x11
#define FD_OBJ_STDERR (void *)0x12

#ifdef CONFIG_POSIX_API
#ifdef CONFIG_POSIX_STDIN_OUT_ERR
static const struct fd_op_vtable stdinout_fd_op_vtable;
#endif

static struct fd_entry fdtable[CONFIG_POSIX_MAX_FDS] = {
#ifdef CONFIG_POSIX_API
#ifdef CONFIG_POSIX_STDIN_OUT_ERR
/*
* Predefine entries for stdin/stdout/stderr. Object pointer
* is unused and just should be !0 (random different values
Expand Down Expand Up @@ -154,7 +154,7 @@ int z_alloc_fd(void *obj, const struct fd_op_vtable *vtable)
return fd;
}

#ifdef CONFIG_POSIX_API
#ifdef CONFIG_POSIX_FD_OPS

ssize_t read(int fd, void *buf, size_t sz)
{
Expand Down Expand Up @@ -211,6 +211,10 @@ off_t lseek(int fd, off_t offset, int whence)
}
FUNC_ALIAS(lseek, _lseek, off_t);

#endif /* CONFIG_POSIX_FD_OPS */

#ifdef CONFIG_POSIX_FCNTL_IOCTL

int ioctl(int fd, unsigned long request, ...)
{
va_list args;
Expand Down Expand Up @@ -259,6 +263,10 @@ int fcntl(int fd, int cmd, ...)
}
#endif

#endif /* CONFIG_POSIX_FCNTL_IOCTL */

#ifdef CONFIG_POSIX_STDIN_OUT_ERR

/*
* fd operations for stdio/stdout/stderr
*/
Expand Down Expand Up @@ -294,4 +302,4 @@ static const struct fd_op_vtable stdinout_fd_op_vtable = {
.ioctl = stdinout_ioctl_vmeth,
};

#endif /* CONFIG_POSIX_API */
#endif /* CONFIG_POSIX_STDIN_OUT_ERR */
39 changes: 39 additions & 0 deletions subsys/net/lib/sockets/getaddrinfo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

/* libc headers */
#include <stdlib.h>
#include <ctype.h>

/* Zephyr headers */
#include <logging/log.h>
Expand All @@ -19,6 +20,13 @@ LOG_MODULE_REGISTER(net_sock_addr, CONFIG_NET_SOCKETS_LOG_LEVEL);

#if defined(CONFIG_DNS_RESOLVER)

/* Helper macros which take into account the fact that ai_family as passed
* into getaddrinfo() may take values AF_INET, AF_INET6, or AF_UNSPEC, where
* AF_UNSPEC means resolve both AF_INET and AF_INET6.
*/
#define RESOLVE_IPV4(ai_family) (IS_ENABLED(CONFIG_NET_IPV4) && (ai_family) != AF_INET6)
#define RESOLVE_IPV6(ai_family) (IS_ENABLED(CONFIG_NET_IPV6) && (ai_family) != AF_INET)

struct getaddrinfo_state {
const struct zsock_addrinfo *hints;
struct k_sem sem;
Expand Down Expand Up @@ -128,6 +136,7 @@ int z_impl_z_zsock_getaddrinfo_internal(const char *host, const char *service,
struct zsock_addrinfo *res)
{
int family = AF_UNSPEC;
int ai_flags = 0;
long int port = 0;
int st1 = DNS_EAI_ADDRFAMILY, st2 = DNS_EAI_ADDRFAMILY;
struct sockaddr *ai_addr;
Expand All @@ -136,6 +145,7 @@ int z_impl_z_zsock_getaddrinfo_internal(const char *host, const char *service,

if (hints) {
family = hints->ai_family;
ai_flags = hints->ai_flags;
}

if (service) {
Expand All @@ -155,6 +165,35 @@ int z_impl_z_zsock_getaddrinfo_internal(const char *host, const char *service,
return getaddrinfo_null_host(port, hints, res);
}

#define SIN_ADDR(ptr) (((struct sockaddr_in *)(ptr))->sin_addr)

/* Check for IPv4 numeric address. Start with a quick heuristic check,
* of first char of the address, then do long validating inet_pton()
* call if needed.
*/
if (RESOLVE_IPV4(family)
&& isdigit((int)*host)
&& zsock_inet_pton(AF_INET, host,
&SIN_ADDR(&res->_ai_addr)) == 1) {
struct sockaddr_in *addr =
(struct sockaddr_in *)&res->_ai_addr;

addr->sin_port = htons(port);
addr->sin_family = AF_INET;
INIT_ADDRINFO(res, addr);
res->ai_family = AF_INET;
res->ai_socktype = SOCK_STREAM;
res->ai_protocol = IPPROTO_TCP;
return 0;
}

if (ai_flags & AI_NUMERICHOST) {
/* Asked to resolve host as numeric, but it wasn't possible
* to do that.
*/
return DNS_EAI_FAIL;
}

ai_state.hints = hints;
ai_state.idx = 0U;
ai_state.port = htons(port);
Expand Down
54 changes: 51 additions & 3 deletions tests/net/socket/getaddrinfo/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,15 +255,63 @@ void test_getaddrinfo_no_host(void)
freeaddrinfo(res);
}

void test_getaddrinfo_num_ipv4(void)
{
struct zsock_addrinfo *res = NULL;
struct sockaddr_in *saddr;
int ret;

ret = zsock_getaddrinfo("1.2.3.255", "65534", NULL, &res);

zassert_equal(ret, 0, "Invalid result");
zassert_not_null(res, "");
zassert_is_null(res->ai_next, "");

zassert_equal(res->ai_family, AF_INET, "");
zassert_equal(res->ai_socktype, SOCK_STREAM, "");
zassert_equal(res->ai_protocol, IPPROTO_TCP, "");

saddr = (struct sockaddr_in *)res->ai_addr;
zassert_equal(saddr->sin_family, AF_INET, "");
zassert_equal(saddr->sin_port, htons(65534), "");
zassert_equal(saddr->sin_addr.s4_addr[0], 1, "");
zassert_equal(saddr->sin_addr.s4_addr[1], 2, "");
zassert_equal(saddr->sin_addr.s4_addr[2], 3, "");
zassert_equal(saddr->sin_addr.s4_addr[3], 255, "");

zsock_freeaddrinfo(res);
}

void test_getaddrinfo_flags_numerichost(void)
{
int ret;
struct zsock_addrinfo *res = NULL;
struct zsock_addrinfo hints = {
.ai_flags = AI_NUMERICHOST,
};

ret = zsock_getaddrinfo("foo.bar", "65534", &hints, &res);
zassert_equal(ret, DNS_EAI_FAIL, "Invalid result");
zassert_is_null(res, "");

ret = zsock_getaddrinfo("1.2.3.4", "65534", &hints, &res);
zassert_equal(ret, 0, "Invalid result");
zassert_not_null(res, "");

zsock_freeaddrinfo(res);
}

void test_main(void)
{
k_thread_system_pool_assign(k_current_get());

ztest_test_suite(socket_getaddrinfo,
ztest_unit_test(test_getaddrinfo_setup),
ztest_user_unit_test(test_getaddrinfo_ok),
ztest_user_unit_test(test_getaddrinfo_cancelled),
ztest_user_unit_test(test_getaddrinfo_no_host));
ztest_unit_test(test_getaddrinfo_ok),
ztest_unit_test(test_getaddrinfo_cancelled),
ztest_unit_test(test_getaddrinfo_no_host),
ztest_unit_test(test_getaddrinfo_num_ipv4),
ztest_unit_test(test_getaddrinfo_flags_numerichost));

ztest_run_test_suite(socket_getaddrinfo);
}
1 change: 1 addition & 0 deletions tests/posix/common/prj.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@ CONFIG_SEM_VALUE_MAX=32767
CONFIG_POSIX_MQUEUE=y
CONFIG_HEAP_MEM_POOL_SIZE=4096
CONFIG_MAX_THREAD_BYTES=4
CONFIG_MAIN_STACK_SIZE=4096

CONFIG_SMP=n
1 change: 1 addition & 0 deletions tests/posix/common/src/clock.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*
* SPDX-License-Identifier: Apache-2.0
*/
#include <posix/sys/types.h>
#include <ztest.h>
#include <posix/time.h>
#include <posix/sys/time.h>
Expand Down
1 change: 1 addition & 0 deletions tests/posix/common/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

#include <posix/sys/types.h>
#include <ztest.h>
#include <pthread.h>

Expand Down
1 change: 1 addition & 0 deletions tests/posix/common/src/mqueue.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

#include <posix/sys/types.h>
#include <ztest.h>
#include <zephyr.h>
#include <misc/printk.h>
Expand Down
1 change: 1 addition & 0 deletions tests/posix/common/src/mutex.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

#include <posix/sys/types.h>
#include <ztest.h>
#include <errno.h>
#include <pthread.h>
Expand Down
1 change: 1 addition & 0 deletions tests/posix/common/src/posix_rwlock.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

#include <posix/sys/types.h>
#include <ztest.h>
#include <pthread.h>

Expand Down
1 change: 1 addition & 0 deletions tests/posix/common/src/pthread.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

#include <posix/sys/types.h>
#include <ztest.h>
#include <kernel.h>
#include <pthread.h>
Expand Down
1 change: 1 addition & 0 deletions tests/posix/common/src/pthread_key.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

#include <posix/sys/types.h>
#include <ztest.h>
#include <kernel.h>
#include <pthread.h>
Expand Down
Loading