Sean Christopherson <seanjc@xxxxxxxxxx> writes:
On Tue, Oct 11, 2022, Colton Lewis wrote:Sean Christopherson <seanjc@xxxxxxxxxx> writes:
> On Mon, Sep 12, 2022, Colton Lewis wrote: > > diff --git a/tools/testing/selftests/kvm/include/test_util.h > > b/tools/testing/selftests/kvm/include/test_util.h > > index 99e0dcdc923f..2dd286bcf46f 100644 > > --- a/tools/testing/selftests/kvm/include/test_util.h > > +++ b/tools/testing/selftests/kvm/include/test_util.h > > @@ -143,4 +143,6 @@ static inline void *align_ptr_up(void *x, size_t > > size) > > return (void *)align_up((unsigned long)x, size); > > }
> > +void guest_random(uint32_t *seed);
> This is a weird and unintuitive API. The in/out param exposes the gory> details of the pseudo-RNG to the caller, and makes it cumbersome to use, > e.g. to create a 64-bit number or to consume the result in a conditional.
To explain my reasoning:
It's simple because there is exactly one way to use it and it's short so anyone can understand how the function works at a glance. It's similar to the API used by other thread-safe RNGs like rand_r and random_r. They also use in/out parameters. That's the only way to buy thread safety. Callers would also have to manage their own state in your example with an in/out parameter if they want thread safety.
I disagree the details are gory. You put in a number and get a new number.
Regardless of whether or not the details are gory, having to be aware of those details unnecessarily impedes understanding the code. The vast, vast majority of folks that read this code won't care about how PRNGs work. Even if the reader is familiar with PRNGs, those details aren't at all relevant to understanding what the guest code does. The reader only needs to know "oh, this is randomizing the address". How the randomization works is completely irrelevant for that level ofunderstanding.
It is relevant if the reader of the guest code cares about reentrancy and thread-safety (good for such things as reproducing the same stream of randoms from the same initial conditions), because they will have to manage some state to make that work. Whether that state is an integer or an opaque struct requires the same level of knowledge to use the API.
It's common knowledge PRNGs work this way.
For people that are familiar with PRNGs, yes, but there will undoubtedly be peoplethat read this code and have practically zero experience with PRNGs.
I understand you are thinking about ease of future extensions, but this strikes me as premature abstraction. Additional APIs can always be addedlater for the fancy stuff without modifying the callers that don't need it.
I agree returning the value could make it easier to use as part of expressions, but is it that important?
Yes, because in pretty much every use case, the random number is going to be immediately consumed. Readability and robustness aside, returning the value cutsthe amount of code need to generate and consume a random number in half.
Ok. This is trivial to change (or implement on top of what is
there). Would you be happy with something like the following?
uint32_t guest_random(uint32_t *seed)
{
*seed = (uint64_t)*seed * 48271 % ((uint32_t)(1 << 31) - 1);
return *seed;
}
