Re: [Gluster-devel] Query regarding GF_ASSERT macro

[Vijay Bellur]

Adding gluster-devel back.

On 01/03/2014 09:48 AM, Atin Mukherjee wrote:
> Hi Vijay,
>
> I have one question on the behaviour of GF_ASSERT macro in case of production 
> build. The way it has been designed, for production build this macro will not 
> abort but at the same point of time it does not behave the way assert should 
> behave, so we are always in danger of having null pointer dereferencing.

assert() macro also behaves in the same manner. It is an usual practice 
to have assert() be a NOOP in production builds. assert()'s normal usage 
is to assert that the expression is TRUE and it _should_ not be 
considered as a replacement for NULL pointer checks. If we want to avoid 
NULL pointer dereferences, it is a good idea to handle such scenarios 
and bail out gracefully.
> A typical example of this scenario is :
>
> GF_ASSERT (shandle);
> GF_ASSERT (shandle->path);
>
> My question is why don't we abort in case of production build? Any specific 
> reason?

abort should be done only in abnormal situations when the program cannot 
gracefully continue.Enabling an assertion macro is useful in test 
environments. However, for production it would be ideal for 
daemons/programs to continue running till they encounter a fatal error.

If you need to handle potential NULL pointer dereferences gracefully, 
you can consider using macros like GF_ASSERT_AND_GOTO_WITH_ERROR, 
GF_VALIDATE_OR_GOTO etc.

HTH,
Vijay

> ----- Original Message -----
> From: "Vijay Bellur" <address@hidden>
> To: "Atin Mukherjee" <address@hidden>, address@hidden
> Sent: Thursday, January 2, 2014 7:15:44 PM
> Subject: Re: [Gluster-devel] Query regarding GF_ASSERT macro
>
> On 01/02/2014 11:44 AM, Atin Mukherjee wrote:
> > I think GF_ASSERT macro should be modified such that in case of no debug build 
> > if x is NULL, after logging an error message it should abort i.e. by calling 
> > assert(). This fix is required as there are places where consecutive GF_ASSERT 
> > macro has been called like :
>
> I think the more appropriate thing to do is to remove dependency on
> DEBUG. assert() macro inherently relies on NDEBUG not being defined.
> The intention behind the current GF_ASSERT macro is to prevent abort()s
> from happening in non-debug/production builds.
>
> We can change GF_ASSERT to remove dependency on DEBUG and add NDEBUG to
> CFLAGS by default in configure.ac.
>
> -Vijay