Re: [Minios-devel] [UNIKRAFT PATCH] lib/ukswrand: Remove private functions from public API

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

Hey Roxana,

On 11.07.2018 17:36, Roxana Nicolescu wrote:
> By making the private uk_swrand_init_r and uk_swrand_randr_r functions visible 
> to users, they can call them when they are not allowed to.
It was not intended to have uk_swrand_init_r() and uk_swrand_randr_r() 
private. A use case is to initialize and operate with your own (and 
maybe multiple random) number generators. Each struct uk_swrand is used 
as storage to its current state. You may want this to reduce contention 
(e.g., SMP environments).
For convenience, the library initializes one random number generator 
already during boot.
Of course, there are always cases that you should not do with an 
interface. A clear interface design (e.g., meaningful parameters and 
fitting data types) and sometimes also description is required. We 
usually prevent most of this mis-usage with UK_ASSERT() statements. But 
even here in this particular case, whenever you re-initialize a the 
number generator, nothing bad happens: It just starts throwing out the 
same number sequence again.
> In order to stop this to happen, these should be removed from swrand.h
> Also, because uk_swrand_randr function is the only public one, the declaration 
> static inline should be changed.
>
> Signed-off-by: Roxana Nicolescu <nicolescu.roxana1996@xxxxxxxxx>
> ---
>   lib/ukswrand/include/uk/swrand.h | 18 +-----------------
>   lib/ukswrand/mwc.c               | 19 ++++++++++++++++---
>   2 files changed, 17 insertions(+), 20 deletions(-)
>
> diff --git a/lib/ukswrand/include/uk/swrand.h b/lib/ukswrand/include/uk/swrand.h
> index e2e43f9..7c4b35a 100644
> --- a/lib/ukswrand/include/uk/swrand.h
> +++ b/lib/ukswrand/include/uk/swrand.h
> @@ -42,26 +42,10 @@
>   extern "C" {
>   #endif
>   
> -struct uk_swrand;
> -extern struct uk_swrand uk_swrand_def;
> -
> -void uk_swrand_init_r(struct uk_swrand *r, __u32 seed);
> -__u32 uk_swrand_randr_r(struct uk_swrand *r);
> -
>   /* Uses the pre-initialized default generator  */
>   /* TODO: Add assertion when we can test if we are in interrupt context */
>   /* TODO: Revisit with multi-CPU support */
> -static inline __u32 uk_swrand_randr(void)
> -{
> -       unsigned long iflags;
> -       __u32 ret;
> -
> -       iflags = ukplat_lcpu_save_irqf();
> -       ret = uk_swrand_randr_r(&uk_swrand_def);
> -       ukplat_lcpu_restore_irqf(iflags);
> -
> -       return ret;
> -}
> +__u32 uk_swrand_randr(void);
>   
>   #ifdef __cplusplus
>   }
> diff --git a/lib/ukswrand/mwc.c b/lib/ukswrand/mwc.c
> index 60a5bf1..493c455 100644
> --- a/lib/ukswrand/mwc.c
> +++ b/lib/ukswrand/mwc.c
> @@ -37,6 +37,7 @@
>   #include <uk/print.h>
>   #include <uk/assert.h>
>   #include <uk/plat/ctors.h>
> +#include <stdlib.h>
>   
>   /* https://stackoverflow.com/questions/9492581/c-random-number-generation-pure-c-code-no-libraries-or-functions */
>   #define PHI 0x9e3779b9
> @@ -55,12 +56,12 @@ struct uk_swrand uk_swrand_def;
>    */
>   static void _uk_swrand_ctor(void) __constructor_prio(UK_SWRAND_CTOR_PRIO);
>   
> -void uk_swrand_init_r(struct uk_swrand *r, __u32 seed)
> +static void uk_swrand_init_r(struct uk_swrand *r, __u32 seed)
>   {
>         __u32 i;
>   
>   	UK_ASSERT(r);
> -
> +       
>         r->Q[0] = seed;
>         r->Q[1] = seed + PHI;
>         r->Q[2] = seed + PHI + PHI;
> @@ -71,7 +72,7 @@ void uk_swrand_init_r(struct uk_swrand *r, __u32 seed)
>         r->i = 4095;
>   }
>   
> -__u32 uk_swrand_randr_r(struct uk_swrand *r)
> +static __u32 uk_swrand_randr_r(struct uk_swrand *r)
>   {
>         __u64 t, a = 18782LL;
>         __u32 x, y = 0xfffffffe;
> @@ -101,3 +102,15 @@ static void _uk_swrand_ctor(void)
>         uk_printd(DLVL_INFO, "Initialize random number generator...\n");
>         uk_swrand_init_r(&uk_swrand_def, CONFIG_LIBUKSWRAND_INITIALSEED);
>   }
> +
> +__u32 uk_swrand_randr(void)
Btw, without static keyword, you can still link to this symbol.

> +{
> +        unsigned long iflags;
> +        __u32 ret;
> +
> +        iflags = ukplat_lcpu_save_irqf();
> +        ret = uk_swrand_randr_r(&uk_swrand_def);
> +        ukplat_lcpu_restore_irqf(iflags);
> +
> +        return ret;
> +}

Thanks,

Simon

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