Re: [Minios-devel] [UNIKRAFT PATCH v2 5/6] lib/ukswrand: Clean-up: devfs nodes are independent of MWC algorithm

[Costin Lupu <costin.lup@xxxxxxxxx>]

Hi Simon,

Please see inline.

On 9/6/19 3:03 PM, Simon Kuenzer wrote:
> This patch cleans up the devfs integration of ukswrand:
> - The config option is properly namespaced.
> - mwc_dev.c is actually independent of the random number generator MWC.
>   We move this file to dev.c
> - Crash the system when registration failed within the constructor.
>   This behavior can be changed as soon as we introduce Unikraft init
>   functions that can return error codes.
> 
> Signed-off-by: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> ---
>  lib/ukswrand/Config.uk            |  5 ++---
>  lib/ukswrand/Makefile.uk          |  6 ++----
>  lib/ukswrand/{mwc_dev.c => dev.c} | 26 ++++++++++++++++++--------
>  3 files changed, 22 insertions(+), 15 deletions(-)
>  rename lib/ukswrand/{mwc_dev.c => dev.c} (83%)
> 
> diff --git a/lib/ukswrand/Config.uk b/lib/ukswrand/Config.uk
> index a1a84bc5..40d22c86 100644
> --- a/lib/ukswrand/Config.uk
> +++ b/lib/ukswrand/Config.uk
> @@ -18,9 +18,8 @@ config LIBUKSWRAND_INITIALSEED
>       int "Initial random seed"
>       default 23
>  
> -config DEV_RANDOM
> -     bool "/dev/random device"
> +config LIBUKSWRAND_DEVFS
> +     bool "Register random and urandom to devfs"
>       select LIBDEVFS
>       default n
> -
>  endif
> diff --git a/lib/ukswrand/Makefile.uk b/lib/ukswrand/Makefile.uk
> index 25247474..b19094e0 100644
> --- a/lib/ukswrand/Makefile.uk
> +++ b/lib/ukswrand/Makefile.uk
> @@ -3,7 +3,5 @@ $(eval $(call addlib_s,libukswrand,$(CONFIG_LIBUKSWRAND)))
>  CINCLUDES-$(CONFIG_LIBUKSWRAND)      += -I$(LIBUKSWRAND_BASE)/include
>  CXXINCLUDES-$(CONFIG_LIBUKSWRAND) += -I$(LIBUKSWRAND_BASE)/include
>  
> -LIBUKSWRAND_SRCS-$(CONFIG_LIBUKSWRAND_MWC) += $(LIBUKSWRAND_BASE)/mwc.c
> -ifdef CONFIG_DEV_RANDOM
> -LIBUKSWRAND_SRCS-$(CONFIG_LIBUKSWRAND_MWC) += $(LIBUKSWRAND_BASE)/mwc_dev.c
> -endif
> +LIBUKSWRAND_SRCS-$(CONFIG_LIBUKSWRAND_MWC)   += $(LIBUKSWRAND_BASE)/mwc.c
> +LIBUKSWRAND_SRCS-$(CONFIG_LIBUKSWRAND_DEVFS) += $(LIBUKSWRAND_BASE)/dev.c
> diff --git a/lib/ukswrand/mwc_dev.c b/lib/ukswrand/dev.c
> similarity index 83%
> rename from lib/ukswrand/mwc_dev.c
> rename to lib/ukswrand/dev.c
> index 5a4cb100..c5ce5f87 100644
> --- a/lib/ukswrand/mwc_dev.c
> +++ b/lib/ukswrand/dev.c
> @@ -101,19 +101,29 @@ static struct driver drv_urandom = {
>       .name = DEV_URANDOM_NAME
>  };
>  
> -__constructor_prio(102) static void _uk_dev_swrand_ctor(void)
> +/*
> + * NOTE: We register the device nodes as application constructor
> + * because at that point of time we can expect that a memory allocator
> + * is available.
> + */
> +/*
> + * TODO: Move this registration to an Unikraft init table as soon we have it
> + * available. Application constructors may require random and urandom already
> + * being available when they get called.
> + */
> +__constructor_prio(101) static void _uk_dev_swrand_ctor(void)

I don't understand why you change the priority here. Let's change it
properly in another patch when we'll introduce a file/list of priorities
(e.g. with macros like '#define CONSTRUCTOR_PRIO_SWRAND 102).

>  {
>       struct device *dev;
>  
> -     uk_pr_info("Add /dev/random and /dev/urandom\n");
> +     uk_pr_info("Register random and urandom to devfs\n");
>  
> -     /* register /dev/urandom */
> +     /* register urandom */
>       dev = device_create(&drv_urandom, DEV_URANDOM_NAME, D_CHR);
> -     if (dev == NULL)

Please keep the changes to a minimum, i.e. leave these `if (dev ==
NULL)` checks as they are. It does bother me that these changes are not
objectively better, but these checks are in no way better than before.

> -             uk_pr_info("Failed to register /dev/urandom\n");
> +     if (!dev)
> +             UK_CRASH("Failed to register urandom to devfs\n");
>  
> -     /* register /dev/random */
> +     /* register random */
>       dev = device_create(&drv_random, DEV_RANDOM_NAME, D_CHR);
> -     if (dev == NULL)

Please keep the changes to a minimum, i.e. leave these `if (dev ==
NULL)` checks as they are.

> -             uk_pr_info("Failed to register /dev/random\n");
> +     if (!dev)
> +             UK_CRASH("Failed to register random to devfs\n");
>  }
> 

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