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

[Roxana Nicolescu <nicolescu.roxana1996@xxxxxxxxx>]

Hi Simon,

When I first counter the problem, I made a simple main.c where I generated my own private number generator. 

And this was the result:

error: ‘uk_swrand’ undeclared (first use in this function)

The problem is that before link time, the generated object needs to know the uk_swrand type.

I tried to change the definition of uk_swrand_init by passing a double reference to struct uk_swrand. But, the constructor for uk_swrand_def complained, and I presumed that this function should be private.

A solution for this would be to put the definition of uk_swrand structure in swrand.h. Another option is to use the global random generator uk_swrand_def and reinitialize it.

Thanks,

Roxana N.

On Fri, Jul 13, 2018 at 3:02 PM, Simon Kuenzer <simon.kuenzer@xxxxxxxxx> wrote:

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@xxxxxxxxm>
---
  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