PCG lacks a module in cmake/

[ringerc]

There's no FindPGC.cmake

$ cmake -DHUNTER_ENABLED=0 .
CMake Error at CMakeLists.txt:130 (find_package):
  By not providing "Findpcg.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "pcg", but
  CMake did not find one.

  Could not find a package configuration file provided by "pcg" with any of
  the following names:

    pcgConfig.cmake
    pcg-config.cmake

  Add the installation prefix of "pcg" to CMAKE_PREFIX_PATH or set "pcg_DIR"
  to a directory containing one of the above files.  If "pcg" provides a
  separate development package or SDK, be sure it has been installed.

[ringerc]

Per #1 I don't understand why PGC is used at all.

You don't use GRD_RANDOM so the only way the GRD_NONBLOCK flag is useful is if the urandom pool isn't initialized yet, and that's not likely to be an issue for the sorts of things Jaeger is used for. The nonblocking call is very suspect given that you don't retry or loop, anyway, so you might fail to initialize the seed at all. You also call the syscall directly instead of using the glibc wrapper where available.

On Linux it's sufficient for all but the strongest crypto purposes (and probably even then) to simply use random_r from <stdlib.h> and there's no need for PGC.

So it seems to me that PGC isn't found properly, and the seed isn't guaranteed safely initialized when using SYS_getrandom. Also, the macros aren't overly safe; better to use a static inline and accept the size arg separately.

I'll send a pull.

[ringerc]

OK, so the reason for the direct syscall is https://lwn.net/Articles/711013/ . Fair enough, glibc support came much later than kernel support for this syscall. However, it's unsafe to determine its availability at configure-time and not check for ENOSYS at runtime, because you don't know if your build kernel will be the same as your runtime kernel.

[ringerc]

Ok, and glibc's random_r etc are probably unsafe or at least hard to get right.

Edit: probably safe for Jaeger's apparent uses, but with care and the caveats in the docs considered.

So this comes down to "why can't you use the global RNG?"

Is it not wanting to rely on or contaminate the app's use of the RNG? That's fair enough if so, but most users won't care, and I'd rather cut a dependency most of the time instead.

[ringerc]

.... and PGC isn't packaged in Fedora - and I don't see it in Debian either. Nor is it documented in the README as required. So it seems the lib is already broken for !HUNTER builds.

Would you accept a fallback implementation to the system RNG?

[isaachier]

Honestly, it isn't a huge deal. I just wanted a "good random function," and this has a great website: http://www.pcg-random.org/. What would you recommend?

[ringerc]

I recommend using the platform randomness facilities whenever possible, because infrastructure libraries should minimise dependencies and use platform functionality when they can, and depend on lowest-common-denominator libraries and versions where they do need to depend on extra facilities. For Linux, if you want strong and pre-seeded pseudorandom values you'd typically use the getrandom syscall into a local random values pool you iterate through, then refill when it's low. But that's probably unnecessary complexity here, unless you need strong randomness you're fine with the platform C library for all but the very worst platforms. Just use rand(). Problem solved. Linux, FreeBSD, Windows, OS X, and prehistoric UNIXes from the dawn of time all have it, and in modern C libraries the quality is generally quite acceptable. It's only necessary to do otherwise if you have an app that depends on deterministic pseudo-random number generation, i.e. it expects the the same sequence of values once it seeds the generator. Any sensible app will use its own PRNG in that case. Or if you need near-crypto-quality randomness but more entropy than the system can give you, in which case you need a strong PRNG like PGC. I'll send a pull that just uses rand(). I'm inclined to remove the whole wrapper and randomness state stuff, but if you want I can keep it and make it stubs, so you can plug in PGC if it's available at build time.

[ringerc]

@isaachier Working on this now. Confused as to why non-hot paths for random init, etc are static inlines in src/jaegertracingc/internal/random.h but the hot code jaeger_random64 is in src/jaegertracingc/random.c. Seems odd to separate them out explicitly like that, especially since the compiler is plenty smart enough to inline a normal static function.

[ringerc]

@isaachier Please confirm if you want me to make PCG optional, or if you're OK with removing it entirely. I recommend removal unless you know you need very strong randomness.

[isaachier]

We can use system randomness. I'm not sure it is wise to put so much in the headers. As of now I followed your advice to keep it in the headers, but I wanted to benchmark this approach against a more opaque implementation. Link time optimization could help, and it would keep the library flexible. Bottom line, people shouldn't be using Jaeger functions directly if they can use OpenTracing calls instead.

[ringerc]

On 7 June 2018 at 19:13, isaachier ***@***.***> wrote: We can use system randomness. I'm not sure it is wise to put so much in the headers. As of now I followed your advice to keep it in the headers, but I wanted to benchmark this approach against a more opaque implementation. Link time optimization could help, and it would keep the library flexible. Bottom line, people shouldn't be using Jaeger functions directly if they can use OpenTracing calls instead.

Yeah, agreed. I'm testing the system randomness patch now and will send a PR soon.

…

-- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services

[isaachier]

I just pushed a commit that should fix this. Sorry to preempt your PR. Please let me know what you think.

[ringerc]

For reference of other readers that's 49513a5

[isaachier]

I'll try to pull in some of these changes. Thank you!

[ringerc]

Improvements for this are in PR #3