KASSERT_WARN for asserting malloc(M_WAITOK) not in a non-sleepable thread

[Bryan Drewery]

On 9/25/2014 1:22 PM, Justin Hibbits wrote:
> On Thu, Sep 25, 2014 at 11:16 AM, Davide Italiano <davide at freebsd.org> wrote:
>> On Thu, Sep 25, 2014 at 11:09 AM, Ian Lepore <ian at freebsd.org> wrote:
>>> On Thu, 2014-09-25 at 10:51 -0700, Davide Italiano wrote:
>>>> On Thu, Sep 25, 2014 at 9:14 AM, Adrian Chadd <adrian at freebsd.org> wrote:
>>>>> Hi,
>>>>>
>>>>> Please bring in KASSERT_WARN().
>>>>>
>>>>> I'm grown up enough to use KASSERT_WARN() along with handling the
>>>>> invariant check myself in code. Having KASSERT_WARN() means I can add
>>>>> in this rather than printf()s or device_printf()'s with various knobs
>>>>> to remove it.
>>>>>
>>>>> (This is absolutely _not_ the "should KASSERT() optionally just log"
>>>>> argument. I'm not going to get into that a second time.)
>>>>>
>>>>>
>>>>
>>>> If you put a KASSERT() inside your code -- probably you should be
>>>> careful enough to put that iff you're sure that it should be always
>>>> verified. No exceptions.
>>>> People tend to be very lazy (including me). I don't expect everybody
>>>> diligently upgrading KASSERT_WARN to KASSERT. So KASSERT_WARN start
>>>> becoming more and more widespread, and people realize all of these
>>>> need to be upgraded to KASSERT or removed. This generally happens
>>>> after years. Yet. Another. Crusade.
>>>> There's a lot of work in the kernel to remove old/wrong/naive  KPI
>>>> from the kernel. jhb@ is looking at timeout()-> callout() conversion.
>>>> I'm personally looking at dev_clone() removal. There are a lot of
>>>> other examples.
>>>> Adding KASSERT_WARN is a step backward, not a step forward, IMHO.
>>>> That said, if you want to pollute the kernel, fine. I expressed my
>>>> opinion, and I'm personally not happy about this, but I never stated
>>>> I'm gonna stop you from doing that.
>>>>
>>>> Thanks,
>>>>
>>>> --
>>>
>>> IMO, this entire argument is ridiculous.  Some conditions are so insane
>>> that you've got to stop immediately rather than make things worse.
>>> Other conditions indicate problems, but the code can recover or
>>> otherwise continue to operate safely.  Trying to define every possible
>>> anomalous condition as either fatal or not worth mentioning is insane.
>>>
>>> Everyone is free to write code such as
>>>
>>> #ifdef INVARIANTS
>>>   if (some_condition)
>>>     printf("whatever warning\n");
>>> #endif
>>>
>>> So let's be clear here:  the objections are to spelling that code
>>> sequence KASSERT_WARN.  If you object, please explain what's wrong with
>>> that spelling and how you would prefer it to be spelled.
>>>
>>> -- Ian
>>>
>>>
>>
>> Take the assert out of the name. Call it DEBUG_WARN, or something else
>> if you like.
>> assert as a pretty *clear* and specific semantic, no need to mess
>> around with it.
>>
>> Thanks,
>>
>> --
>> Davide
> 
> I like my bikeshed a nice royal blue.  At a previous job we used
> ASSERT and VERIFY macros.  VERIFY was comparable to this (warn if
> condition not met, don't panic), so how about KVERIFY() (I'll also
> support KWARN, but I think KVERIFY() conveys a better message by
> name).
> 
> - Justin

I will commit it as KVERIFY tonight based on the majority consensus.
Even at work right now we are tracking down an odd bug where this could
be useful to have temporarily.

-- 
Regards,
Bryan Drewery

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freebsd.org/pipermail/freebsd-arch/attachments/20140926/e1dbdffb/attachment.sig>