Re: [PATCH] zsh/random module [UPDATED]

[dana <dana@dana.is>]

On Mon 7 Nov 2022, at 18:18, Clinton Bunch wrote:
> '(-r)-L+[specify integer lower bound (implies -i)]: :_numbers -d$lmin -l$lmin -m$max "integer lower bound"' \
> '(-r)-U+[specify integer upper bound (implies -i)]: :_numbers -d$umax -l$umin -m$max "integer upper bound"'

Was going to mention in my draft reply that the $max references here are
broken now

On Wed 23 Nov 2022, at 13:46, Daniel Shahaf wrote:
>   + Why should -c default to 8 in the "random bytes" case?  Why
>     shouldn't it default to some other value, or even be required to be
>     specified explicitly by the user? ...
> - Why should -c's default depend on anything at all?  You mentioned
>   downthread you consider making -c1 the default in all cases; that'd be
>   better.

The defaults with this API are kind of weird, because if you make them
dependent on the format (e.g. 8 for hex and 1 for everything else) it's kind
of arbitrary, but if you keep them all the same (e.g. 1 or 8 for everything)
they aren't generally useful — i think it's safe to assume that 'i would like
exactly 1 random hex digit' is not going to be the most common use case

Requiring the user to explicitly specify it would address that, though you
could say then that it goes the other way, e.g. again it's probably safe to
assume that 90% of the time you're only going to want one integer value, and
making people write that out every time, whilst expected in a lower-level API
like a C function, is maybe annoying in a convenience shell built-in

But annoying is probably better than confusing, if those are the options

On Wed 23 Nov 2022, at 13:46, Daniel Shahaf wrote:
>> +%test
>> +  getrandom -U 56
>> +1f:Checking if system has a kernel random source
>> +?(eval):1: No kernel random pool found.
>> +
>
> The test point's code has nothing to do with the test point's
> description and expected errput.

The assertion is backwards but i guess this is just testing to see if the
module works on the system. But should that even be a test? Shouldn't there
instead be a %prep that just leaves the whole file unimplemented in this case?
Unless we're expecting the module to be both available and usable on every zsh
build

On Wed 23 Nov 2022, at 13:46, Daniel Shahaf wrote:
> Oh, and bump that 16 to something 3 or 4 times as big, because a 1/65536
> chance isn't really enough in a world where automated builds (CI,
> distros' QA, etc.) is a thing.

I feel like it should be very nearly impossible for a test to fail just for
randomness reasons. Maybe it's over-kill but in my draft reply to the patch i
was going to suggest something like this:

  () {
    repeat $(( 10 ** 5 )); do
      getrandom -L4 -U5 -c64 -a tmpa
      [[ $tmpa[(r)5] == 5 ]] && return 0
    done
    return 1
  }

On Wed 23 Nov 2022, at 13:52, Daniel Shahaf wrote:
> Could have something like this:
>
>     "-f:specify format:(raw hex decimal)"

I kind of like that

On Wed 23 Nov 2022, at 14:33, Daniel Shahaf wrote:
> 4. Which approach generalizes better?

If we were to directly expose an underlying C API as in Matthew's proposed
arc4random_uniform(), i think it would make sense to remain consistent with
that API. But in any other case i would prefer inclusive, and i think that
fits nicely with other things in zsh as you said

dana