zvfs: improve libc FILE to integer fd abstraction

Previously, there was an implicit assumption that Zephyr's
internal struct fd_entry * was synonymous with FILE * from
the C library.

This is generally not the case and aliasing these two
distinct types was preventing a fair bit of functionality
from Just Working - namely stdio function calls like
fgets() and fopen(). The problem count be seen directly
when trying to use a function like zvfs_fdopen().

Instead of aliasing the two types, require that all Zephyr
C libraries provide

1. FILE *z_libc_file_alloc(int fd, const char *mode)
   Allocate and populate the required fields of a FILE object

2. int z_libc_file_get_fd(FILE *fp)
   Convert a FILE* object to an integer file descriptor.

For Picolibc and Newlib-based C libraries, these functions
set and get the integer file descriptor from a field of the
internal FILE object representation.

For the minimal C library, these functions convert between
array index and struct fd_entry pointers.

Signed-off-by: Chris Friedt <[email protected]>

Author: cfriedt

include/zephyr/posix/unistd.h

@@ -30,7 +30,9 @@ extern "C" {
 /* File related operations */
 int close(int file);
 ssize_t write(int file, const void *buffer, size_t count);
+ssize_t pwrite(int file, const void *buffer, size_t count, off_t offset);
 ssize_t read(int file, void *buffer, size_t count);
+ssize_t pread(int fd, void *buf, size_t count, off_t offset);
 off_t lseek(int file, off_t offset, int whence);
 int fsync(int fd);
 int ftruncate(int fd, off_t length);

include/zephyr/sys/fdtable.h

@@ -7,6 +7,7 @@
 #define ZEPHYR_INCLUDE_SYS_FDTABLE_H_
 
 #include <stdarg.h>
+#include <stdio.h>
 #include <time.h>
 
 /* FIXME: For native_posix ssize_t, off_t. */
@@ -261,6 +262,12 @@ __syscall int zvfs_select(int nfds, struct zvfs_fd_set *ZRESTRICT readfds,
 			  struct zvfs_fd_set *ZRESTRICT errorfds,
 			  const struct timespec *ZRESTRICT timeout, const void *ZRESTRICT sigmask);
 
+void zvfs_libc_file_alloc_cb(int fd, const char *mode, FILE *fp);
+int zvfs_libc_file_alloc(int fd, const char *mode, FILE **fp, k_timeout_t timeout);
+void zvfs_libc_file_free(FILE *fp);
+int zvfs_libc_file_get_fd(FILE *fp);
+FILE *zvfs_libc_file_from_fd(int fd);
+
 /**
  * Request codes for fd_op_vtable.ioctl().
  *

lib/libc/common/Kconfig

@@ -109,7 +109,7 @@ config COMMON_LIBC_THRD
 	depends on DYNAMIC_THREAD
 	# Note: the POSIX_API dependency is only necessary until common elements
 	# of C11 threads and POSIX API can be abstracted out to a common library.
-	depends on POSIX_API
+	depends on POSIX_THREADS
 	default y
 	help
 	  Common implementation of C11 <threads.h> API.

lib/libc/newlib/CMakeLists.txt

@@ -3,6 +3,8 @@
 zephyr_library()
 zephyr_library_sources(libc-hooks.c)
 
+zephyr_library_sources_ifdef(CONFIG_ZVFS zvfs_libc.c)
+
 # Do not allow LTO when compiling libc-hooks.c file
 set_source_files_properties(libc-hooks.c PROPERTIES COMPILE_OPTIONS $<TARGET_PROPERTY:compiler,prohibit_lto>)
 

lib/libc/newlib/Kconfig

@@ -70,4 +70,12 @@ config NEWLIB_LIBC_CUSTOM_SBRK
 	  Allow user to define custom version of the _sbrk function. Some application
 	  may need to use the remaining RAM for also other purposes than heap.
 
+if ZVFS
+
+config ZVFS_LIBC_FILE_SIZE
+	default 184 if 64BIT
+	default 104
+
+endif # ZVFS
+
 endif # NEWLIB_LIBC

lib/libc/newlib/zvfs_libc.c

@@ -0,0 +1,82 @@
+/*
+ * Copyright (c) 2024 Tenstorrent AI ULC
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#include <stddef.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+#include <zephyr/sys/fdtable.h>
+#include <zephyr/toolchain.h>
+
+BUILD_ASSERT(CONFIG_ZVFS_LIBC_FILE_SIZE == sizeof(__FILE),
+	     "CONFIG_ZVFS_LIBC_FILE_SIZE must match size of FILE object");
+BUILD_ASSERT(CONFIG_ZVFS_LIBC_FILE_ALIGN == __alignof(__FILE),
+	     "CONFIG_ZVFS_LIBC_FILE_SIZE must match alignment of FILE object");
+
+static int z_libc_sflags(const char *mode)
+{
+	int ret = 0;
+
+	switch (mode[0]) {
+	case 'r':
+		ret = ZVFS_O_RDONLY;
+		break;
+
+	case 'w':
+		ret = ZVFS_O_WRONLY | ZVFS_O_CREAT | ZVFS_O_TRUNC;
+		break;
+
+	case 'a':
+		ret = ZVFS_O_WRONLY | ZVFS_O_CREAT | ZVFS_O_APPEND;
+		break;
+	default:
+		return 0;
+	}
+
+	while (*++mode) {
+		switch (*mode) {
+		case '+':
+			ret |= ZVFS_O_RDWR;
+			break;
+		case 'x':
+			ret |= ZVFS_O_EXCL;
+			break;
+		default:
+			break;
+		}
+	}
+
+	return ret;
+}
+
+void zvfs_libc_file_alloc_cb(int fd, const char *mode, FILE *fp)
+{
+	/*
+	 * These symbols have conflicting declarations in upstream headers.
+	 * Use uintptr_t and a cast to assign cleanly.
+	 */
+	extern uintptr_t _close_r;
+	extern uintptr_t _lseek_r;
+	extern uintptr_t _read_r;
+	extern uintptr_t _write_r;
+
+	__ASSERT_NO_MSG(mode != NULL);
+	__ASSERT_NO_MSG(fp != NULL);
+
+	fp->_flags = z_libc_sflags(mode);
+	fp->_file = fd;
+	fp->_cookie = (void *)fp;
+
+	*(uintptr_t *)&fp->_read = (uintptr_t)&_read_r;
+	*(uintptr_t *)&fp->_write = (uintptr_t)&_write_r;
+	*(uintptr_t *)&fp->_seek = (uintptr_t)&_lseek_r;
+	*(uintptr_t *)&fp->_close = (uintptr_t)&_close_r;
+}
+
+int zvfs_libc_file_get_fd(FILE *fp)
+{
+	return fp->_file;
+}

lib/libc/picolibc/CMakeLists.txt

@@ -11,6 +11,8 @@ zephyr_library_sources(
   stdio.c
   )
 
+zephyr_library_sources_ifdef(CONFIG_ZVFS zvfs_libc.c)
+
 zephyr_library_compile_options($<TARGET_PROPERTY:compiler,prohibit_lto>)
 
 # define __LINUX_ERRNO_EXTENSIONS__ so we get errno defines like -ESHUTDOWN

lib/libc/picolibc/Kconfig

@@ -200,4 +200,12 @@ config PICOLIBC_GLOBAL_ERRNO
 
 endif # PICOLIBC_USE_MODULE
 
+if ZVFS
+
+config ZVFS_LIBC_FILE_SIZE
+	default 144 if 64BIT
+	default 80
+
+endif # ZVFS
+
 endif # PICOLIBC

lib/libc/picolibc/zvfs_libc.c

@@ -0,0 +1,59 @@
+/*
+ * Copyright (c) 2024 Tenstorrent AI ULC
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#include "stdio-bufio.h"
+
+#include <stddef.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+#include <zephyr/sys/fdtable.h>
+#include <zephyr/toolchain.h>
+
+BUILD_ASSERT(CONFIG_ZVFS_LIBC_FILE_SIZE == sizeof(struct __file_bufio),
+	     "CONFIG_ZVFS_LIBC_FILE_SIZE must match size of FILE object");
+BUILD_ASSERT(CONFIG_ZVFS_LIBC_FILE_ALIGN == __alignof(struct __file_bufio),
+	     "CONFIG_ZVFS_LIBC_FILE_SIZE must match alignment of FILE object");
+
+#define FDEV_SETUP_ZVFS(fd, buf, size, rwflags, bflags)                                            \
+	FDEV_SETUP_BUFIO(fd, buf, size, zvfs_read_wrap, zvfs_write_wrap, zvfs_lseek, zvfs_close,   \
+			 rwflags, bflags)
+
+int __posix_sflags(const char *mode, int *optr);
+
+/* FIXME: do not use ssize_t or off_t */
+ssize_t zvfs_read(int fd, void *buf, size_t sz, const size_t *from_offset);
+ssize_t zvfs_write(int fd, const void *buf, size_t sz, const size_t *from_offset);
+off_t zvfs_lseek(int fd, off_t offset, int whence);
+int zvfs_close(int fd);
+
+static ssize_t zvfs_read_wrap(int fd, void *buf, size_t sz)
+{
+	return zvfs_read(fd, buf, sz, NULL);
+}
+
+static ssize_t zvfs_write_wrap(int fd, const void *buf, size_t sz)
+{
+	return zvfs_write(fd, buf, sz, NULL);
+}
+
+void zvfs_libc_file_alloc_cb(int fd, const char *mode, FILE *fp)
+{
+	struct __file_bufio *const bf = (struct __file_bufio *)fp;
+	int __maybe_unused unused;
+
+	*bf = (struct __file_bufio)FDEV_SETUP_ZVFS(fd, (char *)(bf + 1), BUFSIZ,
+						   __posix_sflags(mode, &unused), __BFALL);
+
+	__bufio_lock_init(&(bf->xfile.cfile.file));
+}
+
+int zvfs_libc_file_get_fd(FILE *fp)
+{
+	const struct __file_bufio *const bf = (const struct __file_bufio *)fp;
+
+	return POINTER_TO_INT(bf->ptr);
+}

lib/os/fdtable.c

@@ -75,6 +75,47 @@ static struct fd_entry fdtable[CONFIG_ZVFS_OPEN_MAX] = {
 
 static K_MUTEX_DEFINE(fdtable_lock);
 
+#ifdef CONFIG_MINIMAL_LIBC
+void zvfs_libc_file_alloc_cb(int fd, const char *mode, FILE *fp)
+{
+	ARG_UNUSED(fd);
+	ARG_UNUSED(mode);
+	ARG_UNUSED(fp);
+}
+
+int zvfs_libc_file_alloc(int fd, const char *mode, FILE **fp, k_timeout_t timeout)
+{
+	ARG_UNUSED(mode);
+	ARG_UNUSED(timeout);
+
+	__ASSERT_NO_MSG(mode != NULL);
+	__ASSERT_NO_MSG(fp != NULL);
+
+	*fp = (FILE *)&fdtable[fd];
+
+	return 0;
+}
+
+void zvfs_libc_file_free(FILE *fp)
+{
+	ARG_UNUSED(fp);
+}
+
+int zvfs_libc_file_get_fd(FILE *fp)
+{
+	return (const struct fd_entry *)fp - fdtable;
+}
+
+FILE *zvfs_libc_file_from_fd(int fd)
+{
+	if ((fd < 0) || (fd >= ARRAY_SIZE(fdtable))) {
+		return NULL;
+	}
+
+	return (FILE *)&fdtable[fd];
+}
+#endif
+
 static int z_fd_ref(int fd)
 {
 	return atomic_inc(&fdtable[fd].refcount) + 1;
@@ -406,30 +447,41 @@ int zvfs_close(int fd)
 	}
 	k_mutex_unlock(&fdtable[fd].lock);
 
+	zvfs_libc_file_free(zvfs_libc_file_from_fd(fd));
 	zvfs_free_fd(fd);
 
 	return res;
 }
 
 FILE *zvfs_fdopen(int fd, const char *mode)
 {
-	ARG_UNUSED(mode);
+	int ret;
+	FILE *fp = NULL;
 
 	if (_check_fd(fd) < 0) {
 		return NULL;
 	}
 
-	return (FILE *)&fdtable[fd];
+	ret = zvfs_libc_file_alloc(fd, mode, &fp, K_NO_WAIT);
+	if (ret < 0) {
+		errno = ENOMEM;
+	} else {
+		__ASSERT(fp != NULL, "zvfs_libc_file_alloc() returned an invalid file pointer");
+	}
+
+	return fp;
 }
 
 int zvfs_fileno(FILE *file)
 {
-	if (!IS_ARRAY_ELEMENT(fdtable, file)) {
+	int fd = zvfs_libc_file_get_fd(file);
+
+	if (fd < 0 || fd >= ARRAY_SIZE(fdtable)) {
 		errno = EBADF;
 		return -1;
 	}
 
-	return (struct fd_entry *)file - fdtable;
+	return fd;
 }
 
 int zvfs_fstat(int fd, struct stat *buf)