lib/vfscore: Implement individual volume automounting #979

mogasergiu · 2023-07-13T06:57:01Z

Provide a command-line argument vfs.fstab that is meant to contain a list of whitespace separated strings with the following format:

	vfs.fstab=[
		"<src_dev>:<mntpoint>:<fsdriver>[:<flags>:<opts>]"
		"<src_dev>:<mntpoint>:<fsdriver>[:<flags>:<opts>]"
		...
	]

The core will parse the provided strings as volume information and attempt to mount them accordingly.

Prerequisite checklist

Read the contribution guidelines regarding submitting new changes to the project;
Tested your changes against relevant architectures and platforms;
Ran the checkpatch.uk on your commit series before opening this PR;
Updated relevant documentation.

Base target

Architecture(s): [e.g. x86_64 or N/A]
Platform(s): [e.g. kvm, xen or N/A]
Application(s): [e.g. app-python3 or N/A]

Additional configuration

Description of changes

lib/vfscore/Makefile.uk

lib/vfscore/volumes.c

StefanJum

This seems to work fine @mogasergiu, I'll add the tag after you resolve Simon's comments.

RaduNichita

Great work, @mogasergiu 💪 . I left some minor nitpicks.
I will approve after that!

lib/vfscore/volumes.c

StefanJum

I retested this and everything works fine, thanks!
Reviewed-by: Stefan Jumarea stefanjumarea02@gmail.com

RaduNichita

All good now. Thanks, @mogasergiu 🥇

Reviewed-by: Radu Nichita radunichita99@gmail.com

lib/vfscore/automount.c

lib/vfscore/Config.uk

In preparation for individual volume auto-mounting to Unikraft[0], introduce a helper library to serialize command-line arguments representing the in-kernel mount path for a device and its options. [0]: unikraft/unikraft#979 Signed-off-by: Alexander Jung <alex@unikraft.io>

lib/vfscore/automount.c

unikraft-bot · 2023-08-07T14:29:28Z

✅ Checkpatch passed

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request and it all looks good!

SHA	commit	checkpatch
`f44ee4b`	lib/vfscore: Implement individual volume automounting	✅
`3a616bc`	lib/vfscore: Discontinue `vfs.{rootfs, rootdef, rootops, rootflags}`	✅

skuenzer · 2023-08-09T12:48:49Z

I just noticed, that in your second commit that removes the vfs.roofs parameter, etc. you also need to update Config.uk:

 config LIBVFSCORE_AUTOMOUNT_ROOTFS
 bool "Automatically mount a root filesysytem (/)"
 default n
-imply LIBUKLIBPARAM
 help
-	Automatically mounts '/' during boot. If `libuklibparam` is
-	compiled in, the default root filesystem and mount options can
-	be changed with the following library parameters:
-	'vfs.rootfs', 'vfs.rootdev', 'vfs.rootflags', and 'vfs.rootopts'
+	Automatically mounts '/' during boot. The parameters are
+	compiled-in and cannot be changed at runtime.

This is a minor comment: Instead of keeping the parameters set from config options from line 76 to 98, you could directly fill the struct in line 226 with those.

I aslo jsut notice that you are using #ifdef CONFIG_ABC although you can use #if CONFIG_ABC which is slightly more powerful because it catches the case when you manually do #define CONFIG_ABC 0. If you use #if on an undefined pre-processor macro, it will return false.

lib/vfscore/automount.c

mogasergiu · 2023-08-09T13:35:07Z

I aslo jsut notice that you are using #ifdef CONFIG_ABC although you can use #if CONFIG_ABC which is slightly more powerful because it catches the case when you manually do #define CONFIG_ABC 0. If you use #if on an undefined pre-processor macro, it will return false.

Is this just general advice? The problem is if you do #if on a string you are going to be met with a preprocessor error. So I think I will stick to #ifdef's in the CONFIG_LIBVFSCORE_ROOT* macro's 🤔.

Provide a command-line argument `vfs.fstab` that is meant to contain a list of whitespace separated strings with the following format: ``` vfs.fstab=[ "<src_dev>:<mntpoint>:<fsdriver>[:<flags>:<opts>]" "<src_dev>:<mntpoint>:<fsdriver>[:<flags>:<opts>]" ... ] ``` The core will parse the provided strings as volume information and attempt to mount them accordingly, if `CONFIG_LIBVFSCORE_FSTAB` is provided. Furthermore, add a configurable `CONFIG_LIBVFSCORE_FSTAB_SIZE` option that will be used as the maximum amount of automatically mounted volumes, excluding the `rootfs` and modularize `initrd` extraction and mounting functionality into a standalone function to be used by both `fstab` and implicit `rootfs`. As a last change, since the file's functionality drastically changed, rename `rootfs.c` to `automount.c`. Signed-off-by: Sergiu Moga <sergiu@unikraft.io>

Now that we can dynamically mount any kind of volume through a more generic format through the commandline, there is an unnecessary overlap between `vfs.{rootfs, rootdef, rootops,rootflags}` and defining `rootfs` related arguments through `vfs.fstab` volumes. Therefore, deprecate `vfs.{rootfs, rootdef, rootops,rootflags}` in favor of `vfs.fstab`. Signed-off-by: Sergiu Moga <sergiu@unikraft.io>

skuenzer

Thanks a lot for the work!

Reviewed-by: Simon Kuenzer simon@unikraft.io
Approved-by: Simon Kuenzer simon@unikraft.io

Now that we can dynamically mount any kind of volume through a more generic format through the commandline, there is an unnecessary overlap between `vfs.{rootfs, rootdef, rootops,rootflags}` and defining `rootfs` related arguments through `vfs.fstab` volumes. Therefore, deprecate `vfs.{rootfs, rootdef, rootops,rootflags}` in favor of `vfs.fstab`. Signed-off-by: Sergiu Moga <sergiu@unikraft.io> Reviewed-by: Radu Nichita <radunichita99@gmail.com> Reviewed-by: Razvan Virtan <virtanrazvan@gmail.com> Reviewed-by: Stefan Jumarea <stefanjumarea02@gmail.com> Reviewed-by: Simon Kuenzer <simon@unikraft.io> Approved-by: Simon Kuenzer <simon@unikraft.io> Tested-by: Unikraft CI <monkey@unikraft.io> GitHub-Closes: #979

mogasergiu requested a review from a team as a code owner July 13, 2023 06:57

mogasergiu force-pushed the smoga/multi-volume branch from cf7ce1b to a84192a Compare July 13, 2023 07:46

skuenzer reviewed Jul 13, 2023

View reviewed changes

lib/vfscore/Makefile.uk Outdated Show resolved Hide resolved

lib/vfscore/volumes.c Outdated Show resolved Hide resolved

lib/vfscore/volumes.c Outdated Show resolved Hide resolved

unikraft-bot added area/lib Internal Unikraft Microlibrary lang/c Issues or PRs to do with C/C++ lib/vfscore VFS Core Interface labels Jul 14, 2023

unikraft-bot assigned nderjung Jul 14, 2023

razvand assigned skuenzer, RaduNichita, razvanvirtan and StefanJum and unassigned nderjung and skuenzer Jul 14, 2023

razvand requested review from RaduNichita, razvanvirtan and StefanJum July 14, 2023 07:32

razvand assigned skuenzer and unassigned RaduNichita, razvanvirtan and StefanJum Jul 14, 2023

razvand added this to the v0.14.0 (Prometheus) milestone Jul 14, 2023

StefanJum reviewed Jul 14, 2023

View reviewed changes

RaduNichita suggested changes Jul 14, 2023

View reviewed changes

lib/vfscore/volumes.c Outdated Show resolved Hide resolved

lib/vfscore/volumes.c Outdated Show resolved Hide resolved

mogasergiu force-pushed the smoga/multi-volume branch 3 times, most recently from 0b15cc1 to 3d1e652 Compare July 14, 2023 13:33

StefanJum reviewed Jul 14, 2023

View reviewed changes

RaduNichita self-requested a review July 14, 2023 15:30

RaduNichita approved these changes Jul 14, 2023

View reviewed changes

skuenzer reviewed Jul 14, 2023

View reviewed changes

lib/vfscore/automount.c Outdated Show resolved Hide resolved

lib/vfscore/Config.uk Outdated Show resolved Hide resolved

mogasergiu force-pushed the smoga/multi-volume branch 5 times, most recently from 3b76779 to 27e582a Compare July 17, 2023 16:20

mogasergiu commented Jul 21, 2023

View reviewed changes

lib/vfscore/automount.c Outdated Show resolved Hide resolved

nderjung mentioned this pull request Jul 25, 2023

feat: Package directory volumes when using OCI unikraft/kraftkit#676

Draft

4 tasks

skuenzer reviewed Aug 1, 2023

View reviewed changes

lib/vfscore/automount.c Outdated Show resolved Hide resolved

mogasergiu force-pushed the smoga/multi-volume branch from 27e582a to 926f080 Compare August 1, 2023 15:57

skuenzer reviewed Aug 4, 2023

View reviewed changes

lib/vfscore/automount.c Outdated Show resolved Hide resolved

skuenzer reviewed Aug 4, 2023

View reviewed changes

lib/vfscore/automount.c Show resolved Hide resolved

mogasergiu force-pushed the smoga/multi-volume branch from 926f080 to 3a616bc Compare August 5, 2023 11:12

razvand requested a review from skuenzer August 9, 2023 06:24

skuenzer reviewed Aug 9, 2023

View reviewed changes

lib/vfscore/automount.c Show resolved Hide resolved

nderjung added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 9, 2023

mogasergiu added 2 commits August 9, 2023 16:42

mogasergiu force-pushed the smoga/multi-volume branch from 3a616bc to 48caaa5 Compare August 9, 2023 13:45

skuenzer approved these changes Aug 9, 2023

View reviewed changes

unikraft-bot closed this in 24be5b0 Aug 9, 2023

unikraft-bot added the ci/merged Merged by CI label Aug 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib/vfscore: Implement individual volume automounting #979

lib/vfscore: Implement individual volume automounting #979

mogasergiu commented Jul 13, 2023 •

edited

Loading

StefanJum left a comment

RaduNichita left a comment •

edited

Loading

StefanJum left a comment

RaduNichita left a comment

unikraft-bot commented Aug 7, 2023

skuenzer commented Aug 9, 2023 •

edited

Loading

mogasergiu commented Aug 9, 2023

skuenzer left a comment

lib/vfscore: Implement individual volume automounting #979

lib/vfscore: Implement individual volume automounting #979

Conversation

mogasergiu commented Jul 13, 2023 • edited Loading

Prerequisite checklist

Base target

Additional configuration

Description of changes

StefanJum left a comment

Choose a reason for hiding this comment

RaduNichita left a comment • edited Loading

Choose a reason for hiding this comment

StefanJum left a comment

Choose a reason for hiding this comment

RaduNichita left a comment

Choose a reason for hiding this comment

unikraft-bot commented Aug 7, 2023

skuenzer commented Aug 9, 2023 • edited Loading

mogasergiu commented Aug 9, 2023

skuenzer left a comment

Choose a reason for hiding this comment

mogasergiu commented Jul 13, 2023 •

edited

Loading

RaduNichita left a comment •

edited

Loading

skuenzer commented Aug 9, 2023 •

edited

Loading