[Gluster-devel] Query regarding GF_ASSERT macro

[Vijay Bellur]

On 01/03/2014 03:33 PM, Harshavardhana wrote:
> On Fri, Jan 3, 2014 at 1:03 AM, Xavier Hernandez <xhernandez at datalab.es> wrote:
>>
>> El 03/01/14 07:02, Harshavardhana ha escrit:
>>
>
>>> But from what i gather GF_ASSERT in GlusterFS context is much like
>>> BUG_ON for Linux kernel - assert is only necessary during debugging -
>>> a backtrace is valid in case of a crash for production where we get
>>> something called gluster dump synonymous with 'Kernel dump'
>>
>> Probably it's only a matter of semantics, but I think that an assert should
>> be something that must be true under any circumstance. Even more, many times
>> the program will crash soon if an assert fails and it's not handled (i.e.
>> program aborted). If the program does not crash, something may get corrupted
>> which is worse. If some condition can be false under some rare circumstances
>> or it's possible to recover from a failed test, it would be better to use
>> GF_VALIDATE_xxx macros.
>>
>> I agree that on production builds, where (most) bugs have been killed, these
>> macros could be a NOOP for performance reasons, however GF_ASSERT() checks
>> the condition even in production builds. For this reason I think it would be
>> better to force the program to abort even in production, or the macro be
>> completely converted to a NOOP.
>>
>>
>
> There were similar discussions in the past there wasn't any clear
> understanding of what
> should be done here. I guess GF_ASSERT should be replaced with  GF_VALIDATE_xxx
> macros perhaps for all functions which need input validation (or which
> function doesn't? :-))

I think we need to do the following:

1. Change GF_ASSERT to be a simple wrapper over assert() and get rid of 
the -DDEBUG dependency.

2. Add -DNDEBUG for GA releases. qa releases to be built with assert() 
enabled.

3. Change GF_ASSERT to GF_VALIDATE_xxx wherever it is being used 
defensively and not as an assertion.

Might be a good idea to log a bug and track this activity if everybody 
is fine with the guidelines.

-Vijay

>
> GF_ASSERT is used extensively everywhere and there are many apparent
> NULL deferences down
> the line after such a check.
>
> Agree with GF_ASSERT being completely NOOP if going by the books, it
> might just improve run-time
> performance :-)
>