Re: [PATCH mptcp-net] mptcp: avoid printing warning once on client side

[Mat Martineau <martineau@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2061 bytes --]

On Fri, 16 Feb 2024, Matthieu Baerts wrote:

> Hi Paolo,
>
> Thank you for your reply!
>
> On 16/02/2024 12:23, Paolo Abeni wrote:
>> On Fri, 2024-02-16 at 11:42 +0100, Matthieu Baerts wrote:
>>> On 16/02/2024 00:25, Mat Martineau wrote:
>>>> On Thu, 15 Feb 2024, Matthieu Baerts (NGI0) wrote:
>
> (...)
>
>>>>> Notes:
>>>>> - Should we convert this pr_warn_once() to a WARN_ONCE()? Or just in
>>>>>   our tree? Or just in DEBUG mode?
>>>>
>>>> I think it makes sense to keep this patch minimal for -net and stable
>>>> (just moving the label).
>>>>
>>>> Also given the consequences of panic_on_warn, would be better to make
>>>> any changes to WARN_ONCE() in mptcp-next/net-next. I don't see extra
>>>> complexity to modify this warning in our tree or debug mode as being
>>>> worth it, do you think it would be valuable?
>>>
>>> Sorry, I'm a bit confused by your reply. For the moment, our CI
>>> complains when a "Call Trace:" is printed, but it doesn't complain when
>>> there is a pr_warn().
>>>

Ok, I didn't realize our CI ignored pr_warn() output so didn't really see 
the motivation for switching to the WARN() macro - but did want to make 
sure (at a minimum) that you weren't proposing WARN() for a -net patch.

>>> If the warning here can be caused by interactions with a buggy host, it
>>> makes sense not to have a WARN() when used in production, but it would
>>> be good for our CI can catch that. If the warning can only be caused by
>>> an internal bug, then easier to use a WARN_ONCE().

This makes sense - I agree it's better for CI to catch it.

>>
>> We can trigger the warning due to either:
>> * local S/W bug
>> * bugged/malicious peer.
>>
>> I think we should avoid a WARN there.
>>
>>> So I think here, we could maybe convert it to a DEBUG_NET_WARN_ON_ONCE()
>>> and upstream that. WDYT?
>>
>> DEBUG_NET is quite lightweight and could be enabled even in production,
>
> Ah OK, I didn't know that!
>
>> that should be an export-branch only change.
>

+1

> Fine by me! I appreciate your suggestion.
>

Thanks Paolo and Matthieu


- Mat