Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()

[Chen Gang <gang.chen@asianux.com>]

On 09/17/2013 12:16 AM, KOSAKI Motohiro wrote:
> (9/15/13 10:55 PM), Chen Gang wrote:
>> On 09/14/2013 12:50 AM, KOSAKI Motohiro wrote:
>>>> ---
>>>>    mm/shmem.c |    2 +-
>>>>    1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>>> index 8612a95..3f81120 100644
>>>> --- a/mm/shmem.c
>>>> +++ b/mm/shmem.c
>>>> @@ -890,7 +890,7 @@ static void shmem_show_mpol(struct seq_file *seq,
>>>> struct mempolicy *mpol)
>>>>        if (!mpol || mpol->mode == MPOL_DEFAULT)
>>>>            return;        /* show nothing */
>>>>
>>>> -    mpol_to_str(buffer, sizeof(buffer), mpol);
>>>> +    VM_BUG_ON(mpol_to_str(buffer, sizeof(buffer), mpol) < 0);
>>>
>>> NAK. VM_BUG_ON is a kind of assertion. It erase the contents if
>>> CONFIG_DEBUG_VM not set.
>>> An argument of assertion should not have any side effect.
>>
>> Oh, really it is. In my opinion, need use "BUG_ON(mpol_to_str() < 0)"
>> instead of "VM_BUG_ON(mpol_to_str() < 0);".
> 
> BUG_ON() is safe. but I still don't like it. As far as I heard, Google
> changes BUG_ON as nop. So, BUG_ON(mpol_to_str() < 0) breaks google.
> Please treat an assertion as assertion. Not any other something.
> 

Hmm... in kernel wide, BUG_ON() is 'common' 'standard' assertion, and
"mm/" is a common sub-system (not architecture specific), so when we
use BUG_ON(), we already 'express' our 'opinion' enough to readers.

And some architectures/users really can customize/config 'BUG/BUG_ON'
(they can implement it by themselves, or 'nop').

If they choose 'nop', they can let code size smaller (also may faster),
but they (not we) also have duty to face related risk: "when we find OS
is continuing blindly, we do not let it stop".



Related information for BUG in "init/Kconfig" (which BUG_ON based on):

config BUG
        bool "BUG() support" if EXPERT
        default y
        help
          Disabling this option eliminates support for BUG and WARN, reducing
          the size of your kernel image and potentially quietly ignoring
          numerous fatal conditions. You should only consider disabling this
          option for embedded systems with no facilities for reporting errors.
          Just say Y.



Thanks.
-- 
Chen Gang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>