On 27/11/2023 16:46, Willem de Bruijn wrote:
> Matthieu Baerts wrote:
>> Hi Willem,
>>
>> (+ cc MPTCP list)
>>
>> On 24/11/2023 18:15, Willem de Bruijn wrote:
>>> From: Willem de Bruijn <willemb@xxxxxxxxxx>
>>>
>>> Same init_rng() in both tests. The function reads /dev/urandom to
>>> initialize srand(). In case of failure, it falls back onto the
>>> entropy in the uninitialized variable. Not sure if this is on purpose.
>>> But failure reading urandom should be rare, so just fail hard. While
>>> at it, convert to getrandom(). Which man 4 random suggests is simpler
>>> and more robust.
>>>
>>> mptcp_inq.c:525:6:
>>> mptcp_connect.c:1131:6:
>>>
>>> error: variable 'foo' is used uninitialized
>>> whenever 'if' condition is false
>>> [-Werror,-Wsometimes-uninitialized]
>>
>> Thank you for the patch!
>>
>> It looks good to me:
>>
>> Reviewed-by: Matthieu Baerts <matttbe@xxxxxxxxxx>
>>
>>> Fixes: 048d19d444be ("mptcp: add basic kselftest for mptcp")
>>> Fixes: b51880568f20 ("selftests: mptcp: add inq test case")
>>> Cc: Florian Westphal <fw@xxxxxxxxx>
>>> Signed-off-by: Willem de Bruijn <willemb@xxxxxxxxxx>
>>>
>>> ----
>>>
>>> When input is randomized because this is expected to meaningfully
>>> explore edge cases, should we also add
>>> 1. logging the random seed to stdout and
>>> 2. adding a command line argument to replay from a specific seed
>>> I can do this in net-next, if authors find it useful in this case.
>>
>> I think we should have done that from the beginning, otherwise we cannot
>> easily reproduce these edge cases. To be honest, I don't think this
>> technique helped to find bugs, and it was probably used here as a good
>> habit to increase the coverage. But on the other hand, we might not
>> realise some inputs are randomised and can cause instabilities in the
>> tests because we don't print anything about that.
>>
>> So I would say that the minimal thing to do is to log the random seed.
>> But it might not be that easy to do, for example 'mptcp_connect' is used
>> a lot of time by the .sh scripts: printing this seed number each time
>> 'mptcp_connect' is started will "flood" the logs. Maybe we should only
>> print that at the end, in case of errors: e.g. in xerror() and
>> die_perror() for example, but I see 'exit(1)' is directly used in other
>> places...
>>
>> That's more code to change, but if it is still OK for you to do that,
>> please also note that you will need to log this to stderr: mptcp_connect
>> prints what has been received from the other peer to stdout.
>>
>> Because it is more than just adding a 'printf()', I just created a
>> ticket in our bug tracker, so anybody can look at that and check all the
>> details about that:
>>
>> https://github.com/multipath-tcp/mptcp_net-next/issues/462
>
> Thanks for the detailed feedback, Matthieu!
>
> Another option to avoid flooding the logs might be to choose a pseudo
> random number in the script and pass the explicit value mptcp_connect.
Good idea!
If I understood correctly, from the .c file, we can check if an env var
is set (e.g. `MPTCP_RND_SEED`) and use it. If not, we generate a random
one like before. The .sh scripts should generate a random number if the
env var is not already set. In any case, this seed should be printed by
the scripts.
> I haven't looked closely, but for transport layer tests it is likely
> that the payload is entirely ignored. Bar perhaps checksum coverage.
> If it does not increase code coverage, randomization can also just be
> turned off.
Here the randomisation is used to change the length of the data that are
exchanged or to do some actions in different orders. I think it still
makes sense to have randomisation. But in case of issues around that, it
might not be clear what the userspace was exactly doing. That's what can
be improved later in net-next.
Cheers,
Matt
