W_ERROR_HAVE_NO_MEMORY and friends

[Andrew Bartlett]

On Wed, 2012-05-30 at 13:32 +0200, Michael Adam wrote:
> Andrew Bartlett wrote:
> > On Wed, 2012-05-30 at 12:35 +0200, Kai Blin wrote:
> > > On 2012-05-30 07:59, Volker Lendecke wrote:
> > > 
> > > >>        via  6a1ad76 s4-dns: Use W_ERROR_HAVE_NO_MEMORY in create_response_rr
> > > > 
> > > > Where is the rule laid down that we must use those macros?
> > > 
> > > I'm not aware of any rule, but the rest of the DNS server code is using
> > > that macro, so I thought consistency was a good thing.
> > > 
> > > > I find them completely obnoxious to use. Doing a return or
> > > > goto from something that looks like a function call is just
> > > > wrong to me. To save precious screen space, I would rather
> > > > go and introduce a special rule to say
> > > > 
> > > > if (ptr == NULL) { return WERR_NOMEM };
> > > 
> > > This is not about screen space for me, it's about having to write less
> > > boilerplate code. Code completion works better on the macro.
> > 
> > I've written code that uses it, and code that does not.  One of the
> > advantages is that it promotes efficient and correct error handling
> > (that is, checking every error, perhaps freeing a temp mem context). 
> > 
> > I tend to find that code that uses this has correct error handling,
> > while code that doesn't (and so has the manually expanded version) tends
> > to have poorer handling, often missing details. 
> 
> Some additional thoughts.
> 
> It is true that the code using the macros is more compact,
> and hence possibly easier to read and grasp at first sight.
> I also liked the macros quite a lot. But while it might be
> easier to start off with a better error handling, this
> might also be deceiving safety in some situations, since
> the automatic use of these macros might make you forget
> some special error handling (freeing of memory, whatnot)
> that needs to be done. So currently I tend to agree with
> Volker that the values in the compactness of code do not
> make up for the additional sublte dangers and possible
> misunderstandings in the code.

One of the great problems is the great power of a macro like:

#define NT_STATUS_HAVE_NO_MEMORY_AND_FREE(x, ctx) do {	\
	if (!(x)) {\
		talloc_free(ctx); \
		return NT_STATUS_NO_MEMORY;\
	}\
} while (0)

#define NT_STATUS_NOT_OK_RETURN_AND_FREE(x, ctx) do {	\
	if (!NT_STATUS_IS_OK(x)) {\
		talloc_free(ctx); \
		return x;\
	}\
} while (0)

These 'simple' macros do a lot, but also save a lot of repeated code
that we tend to get wrong.  But it is easy to look down a page of code
and confirm that all the error handling macros look the same. 

Perhaps this is why I enjoy python and exceptions...

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org