Skip to content
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.

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

cpu/native: add host fs access via VFS #19315

Merged
merged 9 commits into from Apr 26, 2023
Merged

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Feb 24, 2023

Contribution description

It is often quite handy to access files from Linux on RIOT and the other way round.

With native this is very straightforward as native is just a Linux process.

This adds a VFS driver to access the host file system from native.

Testing procedure

Run e.g. tests/vfs_default

USEMODULE += fs_native

This will create a folder nvm0 in the current working directory that is also accessible from RIOT.

To change the exported directory to the current dir, set

CFLAGS += -DFS_NATIVE_DIR=\"\"

You should now be able to access the directory of the example from withing RIOT:

main(): This is RIOT! (Version: 2023.04-devel-487-gaf076-cpu/native_fs)
mount points:
	/nvm0

data dir: /nvm0
> ls /nvm0
./
Makefile.ci	252 B
nvm0/
main.c	858 B
Makefile	431 B
MEMORY.bin	8388608 B
../
bin/
total 4 files

> vfs r /nvm0/Makefile 64
00000000: 696e 636c 7564 6520 2e2e 2f4d 616b 6566  include ../Makef
00000010: 696c 652e 7465 7374 735f 636f 6d6d 6f6e  ile.tests_common
00000020: 0a0a 5553 454d 4f44 554c 4520 2b3d 2076  ..USEMODULE += v
00000030: 6673 5f64 6566 6175 6c74 0a55 5345 4d4f  fs_default.USEMO

Maybe this should be the default on native instead of littlefs2?

Issues/PRs references

@github-actions github-actions bot added Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: sys Area: System Platform: native Platform: This PR/issue effects the native platform labels Feb 24, 2023
@benpicco benpicco force-pushed the cpu/native_fs branch 2 times, most recently from c86c65c to 4fc5c3c Compare February 24, 2023 21:38
Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

Looks straightforward enough to me, with minor comments, and no testing yet.

On a remark / future-evolvability level, I hope that we'll on the long run alter our VFS away from the highly libc dependent structs (stat etc) we have now into something suitably small and understand- and implementable (most flags and modes are just ignored by most file systems). How we use POSIX concepts does make this file system very straightforward, but if and when we change the VFS, we'll have to change a lot of implementations anyway, and this would be just one more.

cpu/native/fs/native_fs.c Outdated Show resolved Hide resolved
boards/native/include/board.h Outdated Show resolved Hide resolved
cpu/native/fs/native_fs.c Show resolved Hide resolved
cpu/native/fs/native_fs.c Outdated Show resolved Hide resolved
@benpicco benpicco added this to Backlog / Proposals in RIOT Sprint Day via automation Feb 27, 2023
@benpicco
Copy link
Contributor Author

benpicco commented Mar 3, 2023

Shall I squash?

@benpicco benpicco requested a review from kaspar030 as a code owner March 3, 2023 12:41
@benpicco
Copy link
Contributor Author

benpicco commented Mar 3, 2023

I added a subfolder inside the native/ directory with the _native_id to prevent clashes when running multiple native instances at once.

To avoid cluttering the directory too much, empty folders are removed when native exits (via unmount).

@chrysn
Copy link
Member

chrysn commented Mar 3, 2023

As all old comments are addressed, yes, please squash and rebase.

Maybe this should be the default on native instead of littlefs2?

I'd appreciate that. Unless a file system is explicitly configured, it's a toss-up whether the embedded target board's file system would even match what's used on native, so let's rather go for something that's obviously native-only instead of using something that looks like it might be what'd be used on the board but isn't.

_native_id

I'm not so happy with how these paths are composed. Per the original description, -DFS_NATIVE_DIR=\"\" would mount things to the directory I'm in. Now, I have to create a native directory in there, and if I want to prepopulate the file system with any data, I'd have to figure out that I don't have to guess the PID but there's an otherwise unrelated parameter I can set.

I see how multiple running instances can be an issue, especially given that RIOT will not expect file system content to "just change". But then, same is true for MTD based FSes, right?

I don't have a good way forward. In My Ideal World native FS (and probably also MTD) would use file locking to grab its backend, and failure would result in a printed message at startup a la "Unable to mount FS due to concurrent access; consider passing the RIOT_FS_NATIVE environment variable with a different directory to each instance". Would that work for you?

@benpicco
Copy link
Contributor Author

benpicco commented Mar 3, 2023

How about adding an --isolate-fs startup parameter?
Or a CONFIG_NATIVE_ISOLATE_FS?

@chrysn
Copy link
Member

chrysn commented Mar 3, 2023 via email

@benpicco
Copy link
Contributor Author

benpicco commented Mar 3, 2023

That's a bit more tricky as we get that from a XFA and I didn't want native to diverge too much from real boards here.

@chrysn
Copy link
Member

chrysn commented Mar 3, 2023 via email

@Teufelchen1
Copy link
Contributor

Worked like a charm! Compiled fine with clang & asan.

@benpicco benpicco modified the milestone: Release 2023.04 Mar 20, 2023
@Teufelchen1
Copy link
Contributor

Also tested it using #19401 - ofc this also tested #19401 :)

@benpicco
Copy link
Contributor Author

Rebased again to solve the merge conflict.

Copy link
Contributor

@Teufelchen1 Teufelchen1 left a comment

Choose a reason for hiding this comment

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

Just nits!

cpu/native/fs/native_fs.c Outdated Show resolved Hide resolved
sys/include/fs/native_fs.h Show resolved Hide resolved
RIOT Sprint Day automation moved this from Backlog / Proposals to Under Review Apr 25, 2023
Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

I've gone through the code, and have some comments. I'll give it a test run in the next iteration.

boards/native/board_init.c Outdated Show resolved Hide resolved
sys/include/fs/native_fs.h Outdated Show resolved Hide resolved
cpu/native/fs/native_fs.c Outdated Show resolved Hide resolved
cpu/native/fs/native_fs.c Outdated Show resolved Hide resolved

static int _rename(vfs_mount_t *mountp, const char *from_path, const char *to_path)
{
static char path_a[PATH_MAX];
Copy link
Member

Choose a reason for hiding this comment

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

I wonder whether it's warranted to have two PATH_MAX buffers around for an operation that can't be GC'd at link time because it's behind the file system driver -- but it's OK, we're on native, nothing in here will eat significant resources of the host anyway.

(If this were targetting embedded, I'd suggest to have static buffers per compilation unit and use them as needed).

cpu/native/fs/native_fs.c Outdated Show resolved Hide resolved
cpu/native/fs/native_fs.c Show resolved Hide resolved
cpu/native/fs/native_fs.c Show resolved Hide resolved
cpu/native/fs/native_fs.c Show resolved Hide resolved
sys/net/application_layer/gcoap/fileserver.c Outdated Show resolved Hide resolved
Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

My concerns as well as @Teufelchen1's have been addressed (thus dismissing the review).

LGTM and passes tests, let's ship it.

@chrysn chrysn dismissed Teufelchen1’s stale review April 25, 2023 22:27

It said "just nits" and the nits have been picked.

RIOT Sprint Day automation moved this from Under Review to Waiting For Murdock Apr 25, 2023
@benpicco
Copy link
Contributor Author

Thank you for the review!

bors merge

@bors
Copy link
Contributor

bors bot commented Apr 26, 2023

Build succeeded:

@bors bors bot merged commit 23f7087 into RIOT-OS:master Apr 26, 2023
25 checks passed
RIOT Sprint Day automation moved this from Waiting For Murdock to Done Apr 26, 2023
@benpicco benpicco deleted the cpu/native_fs branch April 26, 2023 07:54
@benpicco benpicco added this to the Release 2023.07 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: CoAP Area: Constrained Application Protocol implementations Area: cpu Area: CPU/MCU ports Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: native Platform: This PR/issue effects the native platform
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants