zvfs: move file ops from POSIX into ZVFS and move fdtable into ZVFS #98490
Conversation
fkokosinski
force-pushed
the
84656-rework_zvfs
branch
from
edd35b9 to
6f9de2a
Compare
lib/os/fdtable.c
Outdated
| #include <zephyr/fs/fs.h> | ||
|
|
||
| struct stat; | ||
| K_MEM_SLAB_DEFINE(file_desc_slab, sizeof(struct fs_file_t), CONFIG_ZVFS_OPEN_MAX, 4); |
There was a problem hiding this comment.
Note that after the commit 1cfdf7c, you need to use ZVFS_OPEN_SIZE define instead of CONFIG_ZVFS_OPEN_MAX (as the latter is set to 0 in typical use cases).
fkokosinski
force-pushed
the
84656-rework_zvfs
branch
5 times, most recently
from
8c36f05 to
5e6c7fa
Compare
This commit moves all operations on single files into ZVFS and makes the POSIX subsystem call into ZVFS to perform them. It was necessary to define a `struct zvfs_stat` to avoid a dependency cycle. Functions used internally for file i/o operations are publicised since they won't require any changes between various subsystems. This allows ZVFS to actually fulfill its purpose of facilitating cooperation of different file APIs. Signed-off-by: Jakub Klimczak <jklimczak@internships.antmicro.com>
The file descriptor table is used in every area that expects to work on files through descriptor indices. It can only be operated on through functions whose names indicate a relationship with ZVFS (`zvfs_*fd*`). The integer file descriptor mechanism shouldn't be separate from ZVFS. This will make cooperation between different file access APIs much simpler. This commit also makes preparations for the fdtable becoming optional. Signed-off-by: Jakub Klimczak <jklimczak@internships.antmicro.com>
This removes a function that created a new mutex and conditional variable and used memcpy() to compare them with ones in a given fdtable entry. Since those struct members are initialized statically, this test doesn't serve much of a purpose anymore. Moreover, padding bytes inside structs are technically not required to be zero, so these memcpy() calls caused SonarQube to complain. Signed-off-by: Jakub Klimczak <jklimczak@internships.antmicro.com>
fkokosinski
force-pushed
the
84656-rework_zvfs
branch
from
5e6c7fa to
d9d8b6d
Compare
|
zephyrbot
added
area: POSIX
zephyrbot
requested review from
Kludentwo,
MaochenWang1,
andyross,
cfriedt,
dcpleung,
kartben,
krish2718,
matt-rodgers and
nashif
zephyrbot
requested review from
pdgendt,
peter-mitsis,
rlubos,
ssharks and
ycsin
de-nordic
left a comment
de-nordic
left a comment
There was a problem hiding this comment.
Once again why are we investing into this?
I do not see any reason for ZVFS and POSIX to coexist in one system.
Has anyone noticed that this is basically requiring something like 4-5 stack frames from POSIX to FS to even work? Who does it make POSIX work for? Who is the ZVFS for?
|
Thanks a lot for taking a look at this PR.
The POSIX subsystem has its users (including us) - so I suppose it's natural that those users will try to contribute patches/fixes they're using downstream that they found useful. This PR is an attempt at addressing the comments we've received in the previous PR. We'd be happy to address any further technical comments you might have.
As you've noticed in the linked PR, ZVFS claims to be a central library, and "is designed to be used by all Zephyr subsystems that need to work with files." I'm not sure why the POSIX layer wouldn't fit into that description?
The POSIX layer is completely optional - it's up to the user whether they want to allocate the device's resources on it or not. Plus, there are features in Zephyr that are not targeted towards the more resource-constrained devices, such as SMP. Zephyr is very useful and works well on beefy RISC-V or Armv8-A machines. There are also features that straight up wouldn't work on a physical device, like VIRTIO, but are useful nevertheless.
At least according to the documentation, it's meant to be central, so for everyone. Let me reiterate that we're committed to making these changes of as little impact to the average user as possible, meaning - if there's a change introduced by this PR that can be made optional, we'd be more than happy making it disabled by default. |
| /** | ||
| * @brief Read from a file. | ||
| * | ||
| * See IEEE 1003.1 | ||
| */ |
There was a problem hiding this comment.
Seems that ZVFS is already POSIX.
There was a problem hiding this comment.
This is selectable by ZVFS_DEFAULT_FILE_VMETHODS, which is n-selected by default
There was a problem hiding this comment.
Still, it is already POSIX, does not matter how many Kconfigs are there needed to select this.
6 participants
This PR:
It was necessary to define a
struct zvfs_statto avoid a dependency cycle. Functions used internally for file i/o operations are publicised since they won't require any changes between various subsystems. This allows ZVFS to actually fulfill its purpose of facilitating cooperation of different file APIs.The file descriptor table is used in every area that expects to work on files through descriptor indices. It can only be operated on through functions whose names indicate a relationship with ZVFS (
zvfs_*fd*). The integer file descriptor mechanism shouldn't be separate from ZVFS.