Thanks for your work. I've no good understanding of "eio", but doesn't that provide tasks / threads or effects and timers? Wouldn't it be more sensible to integrate a continuous (periodic) reseeding approach, as done in lwt and mirage and async implementations?

Indeed eio does have task,fibers and timers. Let me have a look at lwt implementation.

And while at it, please describe in more detail what

  let rand_source = Eio.Stdenv.secure_random env in
  let got = Eio.Flow.read rand_source buf in

Does -- i.e. where does it take its entropy from? Does it work for arbitrary big buffers buf? What is the failure semantics thereof? For the unix implementation, getrandom is used (see mc_getrandom_stubs.c) and the respective failure modes (i.e. handline of EINTR, calling getrandom again if a smaller size was returned).

Also, the CI seems to fail "cannot find package eio".

For eio_linux, the entropy is same as mirage-crypto-rng.unix - getrandom https://github.com/ocaml-multicore/eio/blob/main/lib_eio_linux/eio_stubs.c#L33.

For other OS, the entropy source is as defined in libuv lib - http://docs.libuv.org/en/v1.x/misc.html#c.uv_random

The buffers can be abitrary/user-defined size.

EINTR is not handled in the eio lib itself - at least for eio_linux. I need to check if libuv handles EINTR or not. Let me come back to you on this later.

The eio package is here - https://opam.ocaml.org/packages/eio/. Perhaps the opam repo needs to be updated?

Hmm, looks like there are multiple issues:

For eio_linux, the entropy is same as mirage-crypto-rng.unix - getrandom https://github.com/ocaml-multicore/eio/blob/main/lib_eio_linux/eio_stubs.c#L33.

There are differences between these two C implementations, esp. if getrandom returned a short read.

For other OS, the entropy source is as defined in libuv lib - http://docs.libuv.org/en/v1.x/misc.html#c.uv_random

shrug maybe

The buffers can be abitrary/user-defined size.

That sounds good.

EINTR is not handled in the eio lib itself - at least for eio_linux. I need to check if libuv handles EINTR or not. Let me come back to you on this later.

Ok - please have a look and report back (this is a blocker for merging).

The eio package is here - https://opam.ocaml.org/packages/eio/. Perhaps the opam repo needs to be updated?

Sorry, I don't understand "perhaps the opam repo needs to be updated". The "ocaml-ci" run here now only executes on the "debian-11-4.12+domains" (and reutrns (failed: Library "eio" not found.)). This is a regression to earlier runs, where lots of compilers and platforms were used. Maybe an issue with ocaml-ci (with multiple opam packages in a ssingle repository), and definitively a blocker to merge this PR.

I'm sorry I won't have time to look into further details before mid May (or even early June),

@talex5 Thanks for the review.

it might be convenient to have a Mirage_crypto_rng_eio.run function, to avoid the need for a switch here. e.g.

Do you mean I do Eio.Switch.run inside Mirage_crypto_rng_eio.run?

EDIT: I have addressed the review points by @talex5.

@talex5 with regards to handling EINTR for getrandom, this has to be done separately in both eio_linux and eio_luv right?

Do you mean I do Eio.Switch.run inside Mirage_crypto_rng_eio.run?

I was thinking of something like this:

let run ?(sleep = Duration.of_sec 1) env fn =
  if !running then (
    Log.debug
      (fun m -> m "Mirage_crypto_rng_eio.initialize has already been called, \
                   ignoring this call.");
    fn ()
  ) else (
    running := true;
    Fun.protect ~finally:(fun () -> running := false) @@ fun () ->
    (try
      let _ = default_generator () in
      Log.warn (fun m -> m "Mirage_crypto_rng.default_generator has already \
                            been set, check that this call is intentional");
     with 
      No_default_generator -> ());
    let seed = 
      let init = 
        Entropy.[ bootstrap ; whirlwind_bootstrap ; bootstrap ; getrandom_init env ] in
      List.mapi (fun i f -> f i) init |> Cstruct.concat
    in 
    let rng = create ~seed ~time:Mtime_clock.elapsed_ns (module Fortuna) in
    set_default_generator rng;
    let source = Entropy.register_source "getrandom" in (* xxx: no way to unregister? *)
    let feed_entropy () = periodically_feed_entropy env (Int64.mul sleep 10L) source in
    Eio.Fiber.any (rdrand_tasks env sleep @ [feed_entropy; fn])
  )

I used any instead of a switch so that the rng will stop when the main function exits.

Do you mean I do Eio.Switch.run inside Mirage_crypto_rng_eio.run?

I was thinking of something like this:

let run ?(sleep = Duration.of_sec 1) env fn =
  if !running then (
    Log.debug
      (fun m -> m "Mirage_crypto_rng_eio.initialize has already been called, \
                   ignoring this call.");
    fn ()
  ) else (
    running := true;
    Fun.protect ~finally:(fun () -> running := false) @@ fun () ->
    (try
      let _ = default_generator () in
      Log.warn (fun m -> m "Mirage_crypto_rng.default_generator has already \
                            been set, check that this call is intentional");
     with 
      No_default_generator -> ());
    let seed = 
      let init = 
        Entropy.[ bootstrap ; whirlwind_bootstrap ; bootstrap ; getrandom_init env ] in
      List.mapi (fun i f -> f i) init |> Cstruct.concat
    in 
    let rng = create ~seed ~time:Mtime_clock.elapsed_ns (module Fortuna) in
    set_default_generator rng;
    let source = Entropy.register_source "getrandom" in (* xxx: no way to unregister? *)
    let feed_entropy () = periodically_feed_entropy env (Int64.mul sleep 10L) source in
    Eio.Fiber.any (rdrand_tasks env sleep @ [feed_entropy; fn])
  )

I used any instead of a switch so that the rng will stop when the main function exits.

I was thinking more in terms of API ergonomics not having to do the following:

Eio_main.run @@ fun env ->
Eio.Switch.run @@ fun _sw ->
...

Perhaps, a version of Eio_main.run as Eio_main.run : ?sw:Switch.t -> (Eio.Stdenv.t -> Eio.Switch.t -> unit) -> unit ? could be useful in such scenarios?

You need to know when the user's main function returns so that the rng fiber can be stopped too. Fiber.any will do this, but a switch won't. There's shouldn't be any need for the user to create a switch here, they just do:

let () =
  Eio_main.run @@ fun env ->
  Mirage_crypto_rng_eio.run env @@ fun () ->
  main ()     (* Can use mirage_crypto in here *)

You need to know when the user's main function returns so that the rng fiber can be stopped too. Fiber.any will do this, but a switch won't. There's shouldn't be any need for the user to create a switch here, they just do:

let () =
  Eio_main.run @@ fun env ->
  Mirage_crypto_rng_eio.run env @@ fun () ->
  main ()     (* Can use mirage_crypto in here *)

Ah, yes. I get it now. The API looks nice

@hannesm

EINTR is not handled in the eio lib itself - at least for eio_linux. I need to check if libuv handles EINTR or not. Let me come back to you on this later.

Ok - please have a look and report back (this is a blocker for merging).

EINTR is now handled correctly by eio. ocaml-multicore/eio#212

Thanks for your work. I did not follow the discusion, but as you may see the CI is not happy. And as remarked earlier, please make sure that the CI continues to run on x86_32 etc. (see https://github.com/mirage/mirage-crypto/runs/5740484133 for what the CI should run on).

EINTR is now handled correctly by eio. ocaml-multicore/eio#212

You may need to cut a release of eio and add some lower bounds in the dependencies here.

In the current state this is not mergeable. I will try to look into this PR again (likely) early June.

please remove the trailing whitespaces all over (I started to comment on them, but there are too many).

I took the liberty to (a) remove the trailing whitespaces (b) unset the default generator when run finishes executing (c) merge the main branch -- which I pushed to your branch @bikallem.

What is left to do:

• minor work on the docstring inconsistency with implementation
• documentation in Mirage_crypto_rng.mli (and rng.ml) to talk about eio (and its special semantics)
• why is it that "eio" is directly at the top level (with the source in eio/src), I'd expect it as rng/eio; and the tests (in case they can't for some reason be in tests, in rng/eio/tests.

why is it that "eio" is directly at the top level (with the source in eio/src), I'd expect it as rng/eio; and the tests (in case they can't for some reason be in tests, in rng/eio/tests.

eio package sources and tests have to be in separate top level dir for dune build and dune runtest to successfully work. This is primarily because some of the packages(core, async) used in rng folder do not build in eio OCaml version (>= 5.0). Which is also the reason why we suffix the folder name to dune build and dune runtest in our CI scripts. Conversely, the eio package doesn't run in older OCaml compilers (<= 4.14).

@bikallem uhm, but isn't the dune build -p <lots-of-packages> in the CI scripts select exactly the packages to be build, and the dune files contain package lines to put them into specific packages?

@hannesm I have moved eio/src and eio/tests to rng/eio and tests respectively and have updated the ci scripts accordingly. I have also updated the documentation in the noted places accordingly.

@hannesm Are there any more issues to be addressed?

Thanks, I merged this. Release will happen shortly.