Re: [Minios-devel] [UNIKRAFT PATCH v2 6/6] lib/ukboot: Root filesystem as library parameter

[Costin Lupu <costin.lup@xxxxxxxxx>]

Hi Simon,

Please see inline.

On 9/6/19 3:03 PM, Simon Kuenzer wrote:
> Enables support for setting fsname, device, flags, and mount data
> for the root filesystem. These options can configured through the menu
> configuration. In case libukparam is enabled, the default values can
> be overwritten with the kernel command line; for example:
>   init.root_fsname="ramfs"
> 
> Signed-off-by: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> ---
>  lib/ukboot/Config.uk   | 22 ++++++++++++++++++++--
>  lib/ukboot/Makefile.uk |  3 +++
>  lib/ukboot/boot.c      | 36 ++++++++++++++++++++++--------------
>  3 files changed, 45 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/ukboot/Config.uk b/lib/ukboot/Config.uk
> index 8420aae9..d9d57715 100644
> --- a/lib/ukboot/Config.uk
> +++ b/lib/ukboot/Config.uk
> @@ -22,11 +22,29 @@ if LIBUKBOOT
>       default y
>       select LIBUKALLOCBBUDDY
>  
> -     config LIBUKBOOT_VFSROOT
> -     bool "Mount ramfs to /"
> +     menuconfig LIBUKBOOT_VFSROOT
> +     bool "Mount a root filesysytem (/)"
>       default n
>       select LIBRAMFS

Please be careful with indentation next time.

>  
> +     if LIBUKBOOT_VFSROOT
> +             config LIBUKBOOT_VFSROOT_FSNAME
> +             string "Default root filesystem"
> +             default "ramfs"

Since we'll have a a next version, please indent the config attributes
for all these 4 new config options.

> +
> +             config LIBUKBOOT_VFSROOT_DEV
> +             string "Default root device"
> +             default ""
> +
> +             config LIBUKBOOT_VFSROOT_FLAGS
> +             hex "Default root mount flags"
> +             default 0x0
> +
> +             config LIBUKBOOT_VFSROOT_DATA
> +             string "Default root mount data"
> +             default ""
> +     endif
> +
>       config LIBUKBOOT_DEVFS
>       bool "Mount devfs to /dev"
>       default n
> diff --git a/lib/ukboot/Makefile.uk b/lib/ukboot/Makefile.uk
> index ea052019..fa5c1aea 100644
> --- a/lib/ukboot/Makefile.uk
> +++ b/lib/ukboot/Makefile.uk
> @@ -1,5 +1,8 @@
>  $(eval $(call addlib_s,libukboot,$(CONFIG_LIBUKBOOT)))
>  
> +# Register to uklibparam, sets "ini" as paramprefix
> +$(eval $(call addlib_paramprefix,libukboot,init))
> +
>  CINCLUDES-$(CONFIG_LIBUKBOOT)                += -I$(LIBUKBOOT_BASE)/include
>  CXXINCLUDES-$(CONFIG_LIBUKBOOT)      += -I$(LIBUKBOOT_BASE)/include
>  
> diff --git a/lib/ukboot/boot.c b/lib/ukboot/boot.c
> index 5e2cb757..4e34ef76 100644
> --- a/lib/ukboot/boot.c
> +++ b/lib/ukboot/boot.c
> @@ -60,14 +60,23 @@
>  #if CONFIG_LIBUKBUS
>  #include <uk/bus.h>
>  #endif /* CONFIG_LIBUKBUS */
> -#ifdef CONFIG_LIBUKLIBPARAM
>  #include <uk/libparam.h>
> -#endif /* CONFIG_LIBUKLIBPARAM */
>  #ifdef CONFIG_LIBUKBOOT_VFSROOT
>  #include <sys/stat.h>
>  #include <sys/mount.h>
>  #endif /* CONFIG_LIBUKBOOT_VFSROOT */
>  
> +#ifdef CONFIG_LIBUKBOOT_VFSROOT
> +static char *root_fsname = CONFIG_LIBUKBOOT_VFSROOT_FSNAME;
> +static char *root_dev    = CONFIG_LIBUKBOOT_VFSROOT_DEV;
> +static char *root_data   = CONFIG_LIBUKBOOT_VFSROOT_DATA;
> +static __u64 root_flags  = (__u64) CONFIG_LIBUKBOOT_VFSROOT_FLAGS;
> +
> +UK_LIB_PARAM_STR(root_fsname);
> +UK_LIB_PARAM_STR(root_dev);
> +UK_LIB_PARAM(root_flags,  __u64);
> +UK_LIB_PARAM_STR(root_data);
> +#endif

This is a general comment. We kind of bloat this boot code with
filesystem related code. Why don't we introduce a new file in vfscore,
e.g. init.c, which would do this - initializing a filesystem? And the
you would just call that function from here, if config option was enabled.

>  
>  int main(int argc, char *argv[]) __weak;
>  #ifdef CONFIG_LIBLWIP
> @@ -98,25 +107,24 @@ static void main_thread_func(void *arg)
>        * VFS initialization
>        */
>  #ifdef CONFIG_LIBUKBOOT_VFSROOT
> -     /*
> -      * TODO: Provide a boot parameter option to specify a custom
> -      * root mount (e.g., ramfs, initrd, 9pfs).
> -      */
> -     uk_pr_info("Mount root...\n");
> -     ret = mount("", "/", "ramfs", 0, NULL);
> +     uk_pr_info("Mount root (%s)...\n", root_fsname);
> +     ret = mount(root_dev, "/", root_fsname, root_flags, root_data);
>       if (ret != 0)
> -             UK_CRASH("Failed to mount ramfs to /\n");
> +             UK_CRASH("Failed to mount /\n");

Since we'll have a next version, please remove this extra space at the end.

>  
>       /*
> -      * TODO: We could place here code that extracts an archive
> -      * found as initrd to '/'
> +      * TODO: Alternatively we could extract an archive found
> +      * as initrd to a ramfs '/' if we have got fsname 'initrd'
>        */
>  
>  #ifdef CONFIG_LIBUKBOOT_DEVFS
>       uk_pr_info("Mount '/dev'...\n");
> -     ret =  mkdir("/dev", S_IRWXU);
> -     if (ret != 0)
> -             UK_CRASH("Failed to create directory '/dev'\n");
> +
> +     /*
> +      * Try to create the mount point.
> +      * We are ignoring errors because it could exist already.
> +      */
> +     mkdir("/dev", S_IRWXU);

mkdir() may fail because of multiple reasons, not only when the
directory exist. That's why we should always check the return code here,
but that's also universally true. You should check if the directory
exists before and if not, then you create it.

>  
>       ret = mount("", "/dev", "devfs", 0, NULL);
>       if (ret != 0)
> 

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel