zvfs: improve libc FILE to integer fd abstraction (v3)

[fkokosinski]

Follow-up to the work in #83386 and #90916.

[fkokosinski]

FYI @cfriedt

[cfriedt]

Looks very well done 👍

[fkokosinski]

CI issues are unrelated to the changes introduced here and is caused by #96787.

[de-nordic]

I am not sure what you do here is good for the future of Zephyr, I think it may erode the embedded nature of Zephyr.
If what is stated here:

is true, then we are basically discouraging people from using native Zephyr API at a seemingly convenient usage of C and POSIX equivalent call, at the cost of bloated code, RAM and stack usage.
The POSIX has been created to allow app development for NIX machines in the late 80s, when everybody was developing own API making it impossible to develop commercial app working on nixes from different vendors; note though, that these were interactive devices where you could tell an owner to stick another RAM card into, in case your POSIX compliant app did not work - something we may not do.
C lib is another thing we try to bend to work here, and, honestly, while C as a language is fine here, some of the libc stuff we could left out

Features like pipes, stdin, stdout, universal file descriptors, etc, are mostly designed for either interactive use or environment where you can interconnect processes to process data passed from one process to another - again feature that came with the original Unix design prerogative to allow processing data in complex workflows with use of simpler tools. Nothing Zephyr is supposed to do.

SoC resource are not ours, they belong to these who bought the chip and Zephyr should take as small part of it as possible; Zephyr should aid the programmer in using the chip as efficiently as possible, whether that is in RAM, code, bluetooth energy or other resource.

We should encourage two write subsystems and libraries as native for Zephyr as possible.

We start bloating the system, the hardware requirements will cut off the lower spec devices for basically no reason, except hubris, and vendors, that got hurt, will start to look around.

I do not think there is place for POSIX in Zephyr, the POSIX subsystem should be killed off and buried.

Even the fact that we have the thing is backwards to what the POSIX compliant operating systems have, where it is better to use the native system API if you want to have best performance.

This entire thing is like "hold my beer" approach to can we glue another thing here, regardless of consequences, on something that has a real value for others.

[sonarqubecloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
4.8% Duplication on New Code

See analysis details on SonarQube Cloud

[cfriedt]

In spite of the blanket generalization above, there are some who would like to see Zephyr become a POSIX conformant RTOS, and to be competitive in industries previously dominated by one or two companies.

Keeping the POSIX layer as thin as possible is a goal of the subsystem, as is keeping it optional.

That's why, for example, the network subsystem has been converted to use native API calls, and the related POSIX API functions are just a thin wrapper around native calls.

It's also why all POSIX system interfaces are disabled by default, and can be enabled by Option Group.

Historically, Zephyr has certainly blurred lines and so more of the POSIX API leaked into regular applications than necessary. However, that should be mostly fixed in #88547. The CONFIG_POSIX_API Kconfig option itself should be deprecated by 4.3.0.

[cfriedt]

Also, IIRC, we should be ok to use picolibc instead of the native libc when building for native_sim. Is that right @fkokosinski @aescolar ?

[carlescufi]

About the NATIVE_LIBC_INCOMPATIBLE , not sure if I understand what the issue is @de-nordic. If I understand correctly, selecting this option only means that you cannot use the Host's native C library when compiling for native_sim (i.e. glibc or similar) and that instead you need to use an embedded library (something that wasn't possible with native posix). I don't think this has any serious impact on anything? EDIT: I stand corrected, not being able to link against the native C library will be an issue for many if such a feature (ZVFS) is a central part of Zephyr in the future.

However, I do share a concern about ZVFS being mandatory in the future for all file-level interactions @cfriedt. IIUC, today we have a lightweight file API that I think should remain available to smaller applications that do not require the full abstraction of file descriptors that comes with POSIX.

[aescolar]

I really think we should not do things like this in Zephyr.
Here we are adding to a component that claims to be central and "must use" for all, yet it seems to set quite a lot of requirements on the C library integration.
And it seems to be done in a very fragile way: Knowing a lot about those libraries and expecting a lot from them.

Imho, basic Zephyr code should be nimble, as simple as possible, with as few dependencies as possible, work with any C library or target, and setting the minimal amount of requirements on other components. If that means we cannot provide some standard API that is provided in higher level systems, so be it.
If we really want to provide those APIs with tough requirements on other components, those must be optional and isolated from the core functionality.

Note: I have only done a quick very partial review, and stopped due to the fundamental issues.

[aescolar]

This changes to posix_clock seem unrelated to the commit and PR?
zvfs: improve libc FILE to integer fd abstraction

[aescolar]

Major: This seems a pretty tight assumption about the picolibc implementation (same for newlib)

[cfriedt]

Also accurate. And also easily adjustable.

[aescolar]

the commit message refers to z_libc_file_alloc & z_libc_file_get_fd

[aescolar]

Fundamental: All these seems like a very tight coupling into the picolibc implementation, using its internals. Which is likely to force us to update both sides simultaneously.

Even though this is picolibc specific, having code in the zephyr tree that is very coupled to an external module creates a very tight dependency.
We have at the very least 2 versions of picolibc (the one in the module and the one in the SDK we point to). But users or CI may try to use other sdk versions (certainly newer versions in the future).
As such I would try to avoid having code like this in the tree.

[cfriedt]

@keith-packard - is there a better method of initializing a suitable Picolibc FILE object here?

[aescolar]

should fixmes not be fixed before submitting?

[cfriedt]

I would suggest no, since it's a far larger change. This is just left here as a reminder to do so.

[aescolar]

Major: This assumes the internals of the implementation of K_MEM_SLAB_DEFINE_STATIC, assuming private implementation details of other components would be quite fragile

[cfriedt]

@fkokosinski - I think what Alberto is hinting at is that there needs to be an non-fragile interface that supports the same functionality. Perhaps it would be a good idea to add a K_MEM_SLAB_BUFFER() macro for that purpose.

[aescolar]

what is this comment?

[aescolar]

there is a macro for this

[aescolar]

Major: The size of structures tends to depends on target, compile options, and other configuration (not just kconfig).
Getting this kind of information thru kconfig seems like a quite manual and fragile way.

[cfriedt]

If there is a better alternative, please feel free to suggest one.

[aescolar]

unrelated change?

[cfriedt]

In version 2 of this change set, it was required to avoid pulling in unnecessary dependencies which resulted in some issue on some platform.

In any case, it's a more accurate refinement of the dependency.

[de-nordic]

In spite of the blanket generalization above

Translating to factually and historically correct with lack of argumentation to counter, so I will just discard them in bulk.

, there are some who would like to see Zephyr become a POSIX conformant RTOS, and to be competitive in industries previously dominated by one or two companies.

OK, we may have different priorities here.

Keeping the POSIX layer as thin as possible is a goal of the subsystem, as is keeping it optional.

That isn't the case when your file support, like in case of close implemented here, is not implemented on native Zephyr API, but instead on additional fd id based level? That is not how the libc file operations should work, these should be natively implemented on the system API.
POSIX may be on top of one or another, does not matter.
Additionally we do not follow our own guidelines here, as we recommend avoiding memory allocations (https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html, 14), and here we rely on it.

I do not know the best approach here, but imho that should be to keep the system working on the most resource constrain devices we can, if that lefts out some of the libc things, then let that be the case.

[cfriedt]

Perhaps this would be more appropriate here, so that POSIX is not unintentionally pulled into the libc matters.

Suggested change

	if (close(fd)) {
	if (zvfs_close(fd)) {

[cfriedt]

Suggested change

#include <unistd.h>

[cfriedt]

These might be better defined at the libc level. The files stdin, stdout, and stderr are from C89 / ANSI.

https://en.cppreference.com/w/c/io/std_streams.html

[cfriedt]

In spite of the blanket generalization above

Translating to factually and historically correct with lack of argumentation to counter, so I will just discard them in bulk.

@de-nordic - not really looking for an argument.

OK, we may have different priorities here.

Exactly. Different users of Zephyr have different priorities.

That isn't the case when your file support, like in case of close implemented here

Do you mean fclose()? COMMON_LIBC_FCLOSE is not mandatory

is not implemented on native Zephyr API, but instead on additional fd id based level?

When using both FILE* and integer file descriptors in the same system, and especially when a FILE* or integer fd can represent more than just a file on a file system, one does expect there to be something tying the two representations together for consistency. Also, it does use a native Zephyr API?

[nashif]

Here we are adding to a component that claims to be central and "must use" for all,

where is this claim being made, I can't find it, can you provide a link?

[aescolar]

Here we are adding to a component that claims to be central and "must use" for all,

where is this claim being made, I can't find it, can you provide a link?

@nashif
Based on the kconfig description here:
https://github.com/zephyrproject-rtos/zephyr/pull/96278/files#diff-53ca56b02dc0627bf11c339bbecb7e927512bd4f2154c09cbfc525861d442d3b

"ZVFS is a central, Zephyr-native library that provides a common interoperable API for all
types of file descriptors such as those from the non-virtual FS, sockets, eventfds, FILE *'s
and more. It is designed to be used by all Zephyr subsystems that need to work with files."

[de-nordic]

When using both FILE* and integer file descriptors in the same system, and especially when a FILE* or integer fd can represent more than just a file on a file system, one does expect there to be something tying the two representations together for consistency. Also, it does use a native Zephyr API?

With this implementation you always have a nix or POSIX layer under C lib. Whether you enable POSIX or not.

Native implementation would basically mean that fopen allocates FILE object and gives back the pointer, and the object directly holds pointers to native system objects and dispatch structure for operations like read, write, seek, close, etc.;
fread, fwrite and so on would call these function through FILE * object held dispatch table; which means that prototypes of these functions would literally take FILE * as a file descriptor to work with, instead of the int id, like it seems to be currently planned.
In short natively written struct FILE would be closer to something:

struct FILE {
    size_t (*write)(struct FILE *f, ...)
    size_t (*read)(struct FILE *f, ...)
    ....
    /* other callbacks and needed buffers */
    void *pointer_to_native_system_object;
}

The wrapper to system object, would have to provide the proper implementation of the callbacks that would work correctly with the native pointer, when given the FILE * type object.

But that is not happens here. Here we have implemented fd table, fclose is literal prove that this is not intended to be native implementation, because nearly first operation that happens is zvfs_libc_file_get_fd to get integer id representing file object from FILE *, which is not native system object:

zephyr/lib/libc/common/source/stdio/fclose.c

Lines 10 to 22 in 6d6489c

	int fclose(FILE *stream)
	{
	int fd;
	fflush(stream);
	fd = zvfs_libc_file_get_fd(stream);
	if (!fd) {
	return EOF;
	}
	if (close(fd)) {
	return EOF;
	}

The fileno, which is here part of os services in lib C is literally POSIX specific function; to be sure I have checked my overpaid C99 spec and latest C drafts, and I am quite sure file functions are implemented on FILE * abstract object and know nothing about how system identifies devices.

What is being implemented here is this

 +-----------------------------+-----------------------------+
 |                             |              POSIX          |
 |            lib C            |                             |
 |      over int fid           |                             |
 +-----------------------------+ - - - - - - - - - - - - - - +
 |                                                           |
 |              Unix type/POSIX int file descriptors         |
 |              via fd table                                 |
 |                                                           |
 +-----------------------------------------------------------+ 
 |                                                           |
 |              Native Zephyr APIs and objects               |
 |                                                           |
 +-----------------------------------------------------------+

Which means that fdtable is and entire FILE * to int id mapping is needed whether you have POSIX enabled or not. Basically when you use C lib f-named functions, you switch on top of what is Zephyr Unix or POSIX subsystem.

What we should have instead is

 +------------------------------+----------------------------+
 |                             <->   POSIX and POSIX/nix     |
 |      lib C over native      <->      int ID table         |
 |      system API/objects     <->                           |
 +-----------------------------------------------------------+
 |                                                           |
 |                 Native Zephyr API and objects             |
 +-----------------------------------------------------------+

where C lib is aware to register and hold POSIX type file ids, only when POSIX is enabled at the same time.

Integer file descriptors are neither C not Zephyr thing.

Additionally, the implementation here, and prior work, basically tries to mimic how nix type process env is created immediately creating always existing stdin, stder and stdout descriptors that may not even be used by anything.
Yes, I know that crt may automatically create FILE * object for these, but unless you use them they do not have to be linked. Here you will get them always, and on embedded system they are basically useless, because you have to intentionally connect them to something you are expecting them to work with, open something you want them to work with.

[cfriedt]

In short natively written struct FILE would be closer to something:

struct FILE {
    size_t (*write)(struct FILE *f, ...)
    size_t (*read)(struct FILE *f, ...)
    ....
    /* other callbacks and needed buffers */
    void *pointer_to_native_system_object;
}

Yet it is the C library that ultimately defines what a FILE object is, as I have been reminded countless times.

But that is not happens here. Here we have implemented fd table, fclose is literal prove that this is not intended to be native implementation, because nearly first operation that happens is zvfs_libc_file_get_fd to get integer id representing file object from FILE *, which is not native system object:

ISO C FILE objects are defined by the respective C library implementors.

The filesystem objects that Zephyr provides are used (when dealing with filesystems). Similarly, the socket objects that Zephyr provides are used when dealing with sockets.

zephyr/lib/libc/common/source/stdio/fclose.c

Lines 10 to 22 in 6d6489c

	int fclose(FILE *stream)
	{
	int fd;
	fflush(stream);
	fd = zvfs_libc_file_get_fd(stream);
	if (!fd) {
	return EOF;
	}
	if (close(fd)) {
	return EOF;
	}

The fileno, which is here part of os services in lib C

I am in agreement (and have commented on exactly that already). That should not use fileno or close (which are POSIX).

What we should have instead is

 +------------------------------+----------------------------+
 |                             <->   POSIX and POSIX/nix     |
 |      lib C over native      <->      int ID table         |
 |      system API/objects     <->                           |
 +-----------------------------------------------------------+
 |                                                           |
 |                 Native Zephyr API and objects             |
 +-----------------------------------------------------------+

Fully agreed.

[de-nordic]

Here we are adding to a component that claims to be central and "must use" for all,

where is this claim being made, I can't find it, can you provide a link?

@nashif Based on the kconfig description here: https://github.com/zephyrproject-rtos/zephyr/pull/96278/files#diff-53ca56b02dc0627bf11c339bbecb7e927512bd4f2154c09cbfc525861d442d3b

"ZVFS is a central, Zephyr-native library that provides a common interoperable API for all types of file descriptors such as those from the non-virtual FS, sockets, eventfds, FILE *'s and more. It is designed to be used by all Zephyr subsystems that need to work with files."

That, and we already have Native API, getting C lib f-functions implemented, there is also POSIX. What is the ZVFS thing for? Why is POSIX being implemented over ZVFS not native Zephyr services directly? ZFVS is already basically nix/POSIX.
This entire thing is strange; I have tracked lseek POSIX implementation and call chain looks like this:

lseek->zvfs_lseek->(inline)zvfs_lseek_wrap-> fdtable[fd].vtable->ioctl->fs_seek

So it is not like POSIX natively calls subsystems, there is ZVFS between it and the target. POSIX lseek has only one call:

zephyr/lib/posix/options/fd_mgmt.c

Lines 44 to 47 in 727c15a

	off_t lseek(int fd, off_t offset, int whence)
	{
	return zvfs_lseek(fd, offset, whence);
	}

There is no documentation on the ZVFS, it does not appear on POSIX page in Zephyr, it is marked EXPERIMENTAL in Kconfig, yet POSIX, that is not marked experimental, selects if for only existing POSIX implementation of device_io.

Shouldn't we just rename the ZVFS posix and drop one thing?